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 issues 213, 215, and 219 #220

Merged
merged 13 commits into from
Aug 1, 2024
Merged

Fix issues 213, 215, and 219 #220

merged 13 commits into from
Aug 1, 2024

Conversation

lohedges
Copy link
Contributor

This PR closes #219 by only excluding non-bonded interactions between from/to ghost atoms within the same molecule. This is one of several possible solutions (probably) the easiest. The issue in #219 can also be avoided by using a distance restraint between the alchemical ion and perturbable molecule, but this isn't used in somd1 and the implementation works without issues. Regardless, I think that the logic to exclude the non-bonded interactions was only intended for atoms within a perturbable molecule anyway.

In the fix I've just reused the existing start_indices vector to work out the molecule index. I'm happy to also create a atom_to_mol container or similar to avoid the double molecule index search. (This was just a quick fix, re-using what we had.)

Note that I've currently skipped the CI. I can trigger once we resolved issue #217. This PR supersedes #216.

  • 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 lohedges added bug Something isn't working cresset related to work with cresset labels Jul 29, 2024
@chryswoods
Copy link
Contributor

This looks good - I am happy with the code you've used to detect molecules. It is very simple. We can look to optimise it in the future if needed. I guess this would only possibly impact things when we have, e.g. perturbable proteins?

chryswoods
chryswoods previously approved these changes Jul 30, 2024
@lohedges
Copy link
Contributor Author

Yes, I doubt it's much of a performance issue. If needed, it's trivial to keep track of the molecule indices as the ghosts are detected. I just did this as the simple first fix and thought I'd optimise once we knew it fixed the problem. (And if optimisation was needed.)

@lohedges lohedges merged commit d120cf2 into devel Aug 1, 2024
5 checks passed
@lohedges lohedges deleted the fix_213_215_219 branch August 1, 2024 15:54
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.

[BUG] Undesired behaviour in systems with multiple perturbable molecules
2 participants