Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added new rescaling method ":vinversion" for SurfaceHopping #323

Merged
merged 2 commits into from
Dec 13, 2023

Conversation

reinimaurer1
Copy link
Member

@reinimaurer1 reinimaurer1 commented Dec 9, 2023

during a frustrated hop in FSSH or IESH, the velocity along the NAC is inverted.
currently, only rescaling=:off or rescaling=:standard are implemented. "Standard" refers to the 1990 Tully definition, where during a frustrated hop, the velocities are not changed.

rescaling=:vinversion inverts velocity along the NAC vector during a frustrated hop. This is standard practice in most modern surface hopping implementations as it helps with overcoherence in certain scenarios. It is also a crucial element for another SurfaceHopping method I plan to implement soon.

…ethod. during a frustrated hop, the velocity along the NAC is inverted
@reinimaurer1
Copy link
Member Author

this still needs more testing, which is why it's a "draft"

Copy link
Member

@jamesgardner1421 jamesgardner1421 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, seems like it should work as intended

# Frustrated hop with insufficient kinetic energy
# perform inversion of the velocity component along the nonadiabatic coupling vector
frustrated_hop_invert_velocity!(sim, DynamicsUtils.get_velocities(u), d)
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In hindsight it would have been nice to implement this option using a struct instead of a symbol so we could use dispatch instead of an if statement but it's fine for now with only a few possible options.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did try to do that, but couldn't quite figure out how. If you have a moment, maybe you could post an Issue that describes the change and what benefits it might offer. Then it becomes something we could pick up at some point as we will surely run into limitations with the current approach as more methods get introduced.

Copy link
Member

@Alexsp32 Alexsp32 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with James' suggestions. Tests pass on my machine. We should consider adding a test case to check the frustrated hopping algorithm works as well.

Co-authored-by: James Gardner <38594562+jamesgardner1421@users.noreply.github.com>
@reinimaurer1 reinimaurer1 marked this pull request as ready for review December 13, 2023 15:13
@reinimaurer1 reinimaurer1 merged commit 1d04c94 into main Dec 13, 2023
2 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants