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 AtomMapping::alignTo when mapping an ion to a molecule #211

Merged
merged 2 commits into from
Jul 13, 2024

Conversation

lohedges
Copy link
Contributor

This PR fixes the AtomMapping::alignTo methods to handle the case where one molecule is a monatomic ion. This allows sire.morph.merge to handle the creation of alchemical ions by merging a water molecule with an ion. The existing alignTo methods fail since they attempt a RMSD alignment when there are too few degrees of freedom.

  • I confirm that I have merged the latest version of devel into this branch before issuing this pull request (e.g. by running git pull origin devel): [y]
  • I confirm that I have added a changelog entry to the changelog (we will add a link to this PR as part of the review): [y]
  • I confirm that I have permission to release this code under the GPL3 license: [y]

Suggested reviewers:

@chryswoods

@lohedges
Copy link
Contributor Author

Note that this is just assuming a single ion is mapped, which is the use case in question. I guess we could generalise this to fix when the AtomMapping contains multiple molecules that are mapped to ions.

@lohedges lohedges added bug Something isn't working cresset related to work with cresset labels Jul 11, 2024
@lohedges
Copy link
Contributor Author

I just need to add a platform skipif decorator for Windows. I'll add this tomorrow.

Copy link
Contributor

@chryswoods chryswoods left a comment

Choose a reason for hiding this comment

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

Thanks - nice fix :-)

@lohedges
Copy link
Contributor Author

Thanks. I'll merge this in so that we can work on the somd2 alchemical ion implementation next week.

@lohedges lohedges merged commit 762f244 into devel Jul 13, 2024
@lohedges lohedges deleted the fix_ion_merge branch July 13, 2024 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cresset related to work with cresset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants