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

Add OpenMM module #482

Merged
merged 80 commits into from
Oct 4, 2024
Merged

Add OpenMM module #482

merged 80 commits into from
Oct 4, 2024

Conversation

joaomcteixeira
Copy link
Member

The code presented in this commit was entirely done by Ivar de Leeuw during his Master course: Major Research Project (GSLS), academic year 2021-2022.

I am pushing the code after Ivar's request and in its exact original form.

In the next commits, the team will work on Ivar's code to accommodate it fully to the HADDOCK3 project.

Thanks, Ivar, for all your great work and best luck with your future studies!

The code presented in this commit was enterily done by Ivar de Leeuw
during his Master course: Major Research Project (GSLS), academic year 2021-2022.

I am pushing the code after Ivar's request and in its exact original form.

In the next commits, the team will work on Ivar's code to accommodate it fully
to the HADDOCK3 project.

Thanks Ivar for all your great work and best luck on your future studies!
@joaomcteixeira joaomcteixeira self-assigned this Jun 20, 2022
@joaomcteixeira joaomcteixeira added the m|openmm Module to work with OpenMM label Jun 20, 2022
@joaomcteixeira
Copy link
Member Author

Made a general linting around files. Addition lint can still be done on function and variable names.

@mgiulini
Copy link
Contributor

Hi @joaomcteixeira, just had a quick chat with Ivar De Leeuw about the version of the dependencies he used: it's openmm v. 7.7.0 and pdbfixer 1.8.1. I'll modify the INSTALL.md accordingly

@joaomcteixeira
Copy link
Member Author

Okay. I strongly suggest having them defined as optional dependencies, same as lightdock and gdock.

@mgiulini
Copy link
Contributor

Okay. I strongly suggest having them defined as optional dependencies, same as lightdock and gdock.

I totally agree.

docs/INSTALL.md Outdated Show resolved Hide resolved
@mgiulini
Copy link
Contributor

mgiulini commented Jun 21, 2022

tried a run with the example file openmm-test.cfg and commit ID #c79f47a. It's already a great work. A few "style" improvements from my user experience:

  • we should clarify if the simulation uses implicit or explicit solvent at the beginning
  • we should report the difference between equilibration and the actual simulation
  • the logging during the actual simulation should be more frequent to allow the user to track the progress, we should inspect a bit the core openmm functions
  • we could split the function runOpenMM in two or more pieces for enhancing the readability
  • a warning appeared Warning: importing 'simtk.openmm' is deprecated. Import 'openmm' instead. probably we can safely ignore it, but I still have to check
  • the integrator seed should be added as a user-defined parameter and printed during the execution, so that we could obtain (almost) identical calculations

@joaomcteixeira
Copy link
Member Author

a warning appeared Warning: importing 'simtk.openmm' is deprecated. Import 'openmm' instead. probably we can safely ignore it, but I still have to check

I would say this is not ours and is something from openmm because we don't import from simtk. not in the original Ivar's code nor in the linted one.

@joaomcteixeira
Copy link
Member Author

@mgiulini, the pre-last commit solves the globals() problem. It was not necessary because the value was the same as the key, so we can simply avoid calling the globals() dictionary.

Also corrected some defaults that were lists to single strings

Undo linting from all previous commits. After linting, some deep bug
appeared and I found no way to reproduce the original results. So, we need to
lint block by block very carefully.

I left the other files: install.md and config_read untouched because
those changes will surely go through.
@mgiulini
Copy link
Contributor

mgiulini commented Jul 6, 2022

As for the reproducibility: it is possible to assign a seed to the integrators, but not to the modeller functions. Therefore, the solvation boxes specified at the beginning of the simulations are bound to differ.

@VGPReys VGPReys requested a review from a team September 23, 2024 09:47
rvhonorato
rvhonorato previously approved these changes Sep 23, 2024
Copy link
Member

@rvhonorato rvhonorato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Co-authored-by: Anna Engel <113177776+AljaLEngel@users.noreply.github.com>
AljaLEngel
AljaLEngel previously approved these changes Sep 24, 2024
mgiulini
mgiulini previously approved these changes Sep 30, 2024
src/haddock/core/exceptions.py Outdated Show resolved Hide resolved
amjjbonvin
amjjbonvin previously approved these changes Sep 30, 2024
Copy link
Member

@amjjbonvin amjjbonvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What will happen if a protein-ligand complex is passed to the openmm module?
Is there a check that only "pure" protein and nuclei acids are accepted?
Also I assume (did not check the openmm code) that the system is neutralised by adding counter ions. Is this true?

@VGPReys
Copy link
Contributor

VGPReys commented Oct 1, 2024

What will happen if a protein-ligand complex is passed to the openmm module? Is there a check that only "pure" protein and nuclei acids are accepted? Also I assume (did not check the openmm code) that the system is neutralised by adding counter ions. Is this true?

  • System is neutralized
  • No yet checks for ligands... must do it, good point !

Co-authored-by: Marco Giulini <54807167+mgiulini@users.noreply.github.com>
@VGPReys VGPReys dismissed stale reviews from amjjbonvin, mgiulini, and AljaLEngel via 7253d32 October 1, 2024 08:57
@VGPReys
Copy link
Contributor

VGPReys commented Oct 2, 2024

  • Added try/except logic to handle cases where topology cannot be generated due to unknown (by the force-field) residues.
  • Also fixed module when using implicit solvent !

@mgiulini mgiulini merged commit 8a1157b into main Oct 4, 2024
4 checks passed
@mgiulini mgiulini deleted the openmm branch October 4, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature request m|openmm Module to work with OpenMM
Projects
Development

Successfully merging this pull request may close these issues.

7 participants