search

michellab / Sire

Sire Molecular Simulations Framework
http://siremol.org
GNU General Public License v3.0
95 stars 26 forks source link

Fix issues Atom Ordering Issues with centerSolute and setupDistanceRestraints #374

Closed fjclark closed 2 years ago

fjclark commented 2 years ago

Hi,

As a result of the 2022.1.0 changes to atom ordering, my ABFE calculations with SOMD using multiple distance restraints were failing. There were issues with both centerSolute (molecule number not in system) and setupDistanceRestraints (which used molecule number 1 to store restraint information).

To fix these issues, I've added a function - getSolute - which returns the molecule matching the perturbed residue number. This is then used by both centerSolute and setupDistanceRestraints. The distance restraint information is then retrieved in openmmfrenergyst.cpp by looping over all molecules and checking for the "linkbonds" property. I've tested this with some of my previously working inputs, both with and without multiple distance restraints, and it seems to be working as previously.

Thanks.

lohedges commented 2 years ago

Hi @fjclark. Many thanks for this, it's much appreciated. I'm glad the fix was easy and the issue wasn't too pervasive. I'll add some review comments to suggest a few edits to the code. Let me know if you want to do these yourself, otherwise I would be happy to merge as-is and edit at my end. (One suggestion would involve updating the Sire.Mol wrappers.)

Cheers.

fjclark commented 2 years ago

No problem. If you could merge as-is and edit, that would be ideal - thanks very much.

lohedges commented 2 years ago

No problem. I've still added review comments so you can see what I would change and why. Let me know if you have any thoughts or suggestions. I'll merge over a bit later and edit. Once things are rebuilt could you test that things still work as expected.

Cheers.

fjclark commented 2 years ago

Thanks very much for the comments - it's very helpful to see this. Yes, we should break after that line. Brilliant, I'll test again after that.

Cheers

lohedges commented 2 years ago

It looks like there's lots of other loops over all of the molecules in the initialise method, i.e. rather than using the solute directly. I'll leave your implementation as-is, other than adding a break statement once the molecule is found. I think most of this will be refactored with @halx's PME-FEP work, anyway.

Cheers.

halx commented 2 years ago

A refactoring is not the immediate goal and so may take quite some time before that happens. At the moment I am working on a proof-of-concept and as such try to minimise code changes as much as possible.