RE-EDS with OpenMM

[SalomeRonja]

Description

Adding the proof of concept implementation of RE-EDS with OpenMM, as well as the input files for the paper documentation.

Todos

• [x] add OpenMM RE-EDS module
• [x] add input files for datasets in GROMOS/OpenMM for paper documentation

Status

• [x] Ready to go

[lgtm-com[bot]]

This pull request introduces 1 alert when merging aa2e3ecdf6a760aa4017d977b275995fc282f760 into 302eca14e42ef7babc45db405e93d54671ce4e76 - view on LGTM.com

new alerts:

• 1 for Unused import

[candidechamp]

I had a look at everything and it looks good to me. I just had a few comments/questions.

1. In the Reeds class, you define the following variables:

self.s_values = reeds_simulation_variables.s_values self.energy_offsets = reeds_simulation_variables.energy_offsets self.temperature = reeds_simulation_variables.temperature self.beta = (1/(reeds_simulation_variables.kb*self.temperature)).value_in_unit(u.mole/(u.joule))*1000 # mol/kJ self.initial_time = reeds_simulation_variables.initial_time

Wouldn't it be simpler to just call these variables directly as self.reeds_simulation_variables.s_values instead of making another copy? Maybe there is a reason I am missing but not having two variables referencing the same thing might avoid problems. If it is to have a lighter syntax, the variable reeds_simulation_variables could be shortened to params.

2. Functions in the analysis.py script are not documented. Could you please add some basic info (mostly so we know the expected format for the input).

3. This is maybe more of a long term idea, but all of the code in the run() function could be broken down into smaller functions (e.g. run_md(), calc_exchanges() etc.)

Overall the code is good to be merged as it is anyways. Let me know if you want to have a look at these quick fixes before the merge or in a next merge request. Then we should try to think of making a release tag which we can associate to your publication.

[SalomeRonja]

@candidechamp thanks a lot for having a look!

Wouldn't it be simpler to just call these variables directly as self.reeds_simulation_variables.s_values instead of making another copy?

Yes, you're absolutely right. I think the self.beta makes sense, as it avoids having to recalculate the value, but the other variables are all available through reeds_simulation_variables. It was indeed just to have a shorter syntax. I would still leave it as is for now, but I'll keep this in mind for a (near) future cleanup! Thanks for pointing it out.

Functions in the analysis.py script are not documented.

good point - I will add that before merging

but all of the code in the run() function could be broken down into smaller functions

Yes, definitely! I also don't like that the initialization function is so long, that one should also be broken down. Probably also the creation of the four separate custom forces in the custom_reaction_field function. The code will still be heavily modified and refactored in the future to be (a) cleaner and (b) more efficient. This is definitely on my list.

Thanks again for your feedback. I will merge after adding the documentation to the example analysis script.

[lgtm-com[bot]]

This pull request introduces 2 alerts when merging 5a124dd2569e1e4a8b6adcc7973c6231a052cf71 into 579516f01ed82f1b75f988478fd08ad569f703e5 - view on LGTM.com

new alerts:

• 1 for Unused local variable
• 1 for Unused import