search

michellab / Sire

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

Roadmap to merge QUBE implementation in devel #329

Closed jmichel80 closed 3 years ago

jmichel80 commented 3 years ago

Hi @lohedges ,

@SofiaBariami is finalising a collaboration with the team of Daniel Cole at Newcastle University to implement the QUBE forcefield in SOMD. A preprint describing the work is available here

Sofia made modifications to Sire to implement OPLS combining rules in SOMD, and to support parsing of QUBE input files. She also made modifications to BioSimSpace to support inclusion of virtual sites in SOMD's pert files. I believe the most of up to date branch for QUBE in Sire is this one. @SofiaBariami please confirm if this is so, and which other branches in Sire are deprecated and could be safely deleted (there were several failed branching attempts initially).

@SofiaBariami is writing up her thesis over the coming months as a part of this I would like the code changes she worked on to be merged in devel so they can eventually make it into a Sire 2021 release. As things stand a pull request would probably be messy owing to other mods you and I did to SOMD in the meantime. It would also be really useful if you could help review @SofiaBariami 's code, advise her how to refactor (lightly) the code for a cleaner merge, and review unit tests to make sure we do not break support for QUBE in the future. Please let us know how you think this could be progressed further.

lohedges commented 3 years ago

Hi @jmichel80 and @SofiaBariami,

I'll take a look at the edits and let you know how best to proceed. As you say, a direct merge is not possible. Since you are more familar with the QUBE edits (to Sire) it might be worth your while merging the latest devel into a local qube-jm_feature-vSites branch to see what conflicts there are. (I could do this, but you might have a better idea of what sections of the code have changed.) I typically do a merge this way before submitting a pull-request from a feature branch so I know what the likely issues are ahead of time, and can try to fix them myself if possible. There were quite a few changes to enable triclinic box support for SOMD, but these were isolated to specific sections of the SOMD code. There will most certainly be conflicts between the wrappers, but these can be ignored. There is no point fixing the conflicts by hand and it's easiest to simply re-create the wrappers after the merge, then committing the updated files.

I've also had a quick look at the BioSimSpace edits and it looks like some of the changes are specific to QUBE, e.g. terms in the non-bonded intrascale matrix. Before merging it would be good to work out a general way of handling all of the forecefields that we support. This could be done by adding a Sire forcefield object for QUBE, or just adding some kind of if/else logic in the BioSImSpace code depending on the forcefield that we're dealing with, e.g. one branch taken if the molecule has virtual sites. There are also a few other things, such as publicly exposing utility functions, but I'll comment on these in the pull thread.

Cheers.

SofiaBariami commented 3 years ago

Hi everybody, Thank you for all the help with this project. I confirm that qube-jm_feature-vSites is the branch that is most up to date. Feel free to let me know what changes you would like me to make at the code to have a clean merge. Cheers!

jmichel80 commented 3 years ago

Hi @SofiaBariami please follow @lohedges suggestions to start the merging process. Also delete the deprecated branches. It's better if you take the lead and ask questions on github when you hit a stumbling block.

lohedges commented 3 years ago

The qube-jm branch is now merged. If there will be additional code to add, e.g. relating to implementation of virtual sites, then we keep this open to discuss progress. Otherwise, feel free to close this issue when you are happy.