OpenBioSim / sire

Sire Molecular Simulations Framework
https://sire.openbiosim.org
GNU General Public License v3.0
39 stars 11 forks source link

Fix issues 213, 215, and 219 #220

Closed lohedges closed 1 month ago

lohedges commented 1 month ago

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.

Suggested reviewers:

@chryswoods

chryswoods commented 1 month ago

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?

lohedges commented 1 month ago

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.)