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

Fix form of null bonded terms #43

Merged
merged 2 commits into from
May 2, 2024
Merged

Fix form of null bonded terms #43

merged 2 commits into from
May 2, 2024

Conversation

lohedges
Copy link
Contributor

@lohedges lohedges commented May 2, 2024

This PR fixes the form of null bonded terms used during the somd1 compatibility mode. By not adding the terms in the end state in which they should be nulled, sire will correctly add a null term that zeros the force constant but preserves the other components, e.g. r0, theta0, phase, periodicity, from the opposite state. When adding an explicit null term, conversion to a CAS expression creates a fully nulled potential, so all components are zero, i..e. a bond would have its force constant and bond size perturbed to zero. I don't think this causes issues in practice, since it's just a different pathway through lambda space, but it is good to be consistent with what is done for somd1.

@lohedges lohedges requested a review from mb2055 May 2, 2024 08:46
@lohedges
Copy link
Contributor Author

lohedges commented May 2, 2024

Just to add that this doesn't solve the ethane<-->methanol reversibility problem ;-)

mb2055
mb2055 previously approved these changes May 2, 2024
Copy link
Contributor

@mb2055 mb2055 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 👍 Well spotted

@lohedges
Copy link
Contributor Author

lohedges commented May 2, 2024

I'll hold off on this for a sec. From the emle side I am finding that I sometimes get errors such as:

Have a 1-4 scaling factor (0.833333/0.5) between atoms N:6 and HH31:0 despite there being no physical dihedral between these two atoms. All
1-4 scaling factors MUST be associated with physical dihedrals. The shortest path is [  ]

when dihedrals are removed from an end state. I would have assumed that a null would be added so that the term would be there.

@lohedges
Copy link
Contributor Author

lohedges commented May 2, 2024

Thanks, I just want to double check that nulling terms in this way doesn't cause issues for dihedrals that don't only contain ghost atoms. This was what was causing my issue with sire-emle above.

@lohedges lohedges merged commit 09d276f into main May 2, 2024
@lohedges lohedges deleted the fix_null_bonded_terms branch May 2, 2024 12:14
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.

2 participants