-
Notifications
You must be signed in to change notification settings - Fork 19
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
OpenMMSolvation: Support for non-cubic boxes and defined box properties #673
Conversation
Hello @IAlibay! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-08-27 13:21:57 UTC |
environment.yml
Outdated
@@ -26,6 +26,7 @@ dependencies: | |||
- click | |||
- typing-extensions | |||
- lomap2>=2.3.0 | |||
- openmm>=8.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These solvation settings need OpenMM 8.0. Overall supporting multiple major versions of OpenMM is probably unwise as we'll need to test them all so I would suggest bumping up the minimum version?
Otherwise we can add a version check to the validator.
This is low priority, but it would be amazing if we could get it merged sooner than later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Just out of curiosity (might be a dumb question): how would you specify the density of molecules in the box?
Co-authored-by: Hannah Baumann <[email protected]>
I don't think it's a dumb question. Looks like.. you can't? http://docs.openmm.org/development/api-python/generated/openmm.app.modeller.Modeller.html#openmm.app.modeller.Modeller.addSolvent 🙈 I suspect the answer for setting a given density would be to do it manually, selecting a given box size / vectors and then setting the number of waters added manually? |
No... that doesn't work because it's not allowed. Good question indeed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Two questions, nothing blocking
@@ -27,7 +27,7 @@ dependencies: | |||
- openfe-analysis>=0.2.0 | |||
- click | |||
- typing-extensions | |||
- openmm !=8.1.0 | |||
- openmm >=8.0.0,!=8.1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we document the min version of OpenMM we support? Or which versions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not, but we should!
""" | ||
settings = openmm_rfe.RelativeHybridTopologyProtocol.default_settings() | ||
settings.solvation_settings.solvent_padding = 1.5 * unit.nanometer | ||
settings.solvation_settings.box_shape = 'dodecahedron' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this become our new recommendation? (dodecahedron boxes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Long term yes, but we'll need to do some performance benchmarks.
@hannahbaumann if you have time could you have another look at this? I'd like to get to a point where we can merge this but I definitely want your stamp of approval before we do! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm, thanks @IAlibay !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could use a changelog entry, otherwise it is good to go!
Good call, done! |
Fixes #585
In this PR:
Developers certificate of origin