Conversation
* Make geomeTRIC an option for both torsions and standard stores * Refactor the torsion and standard minimization functions to reduce duplication
Also pass n_processes
Codecov Report❌ Patch coverage is 🚀 New features to boost your workflow:
|
|
@fjclark just a curiosity question, did you check different values of tolerance for the torsion angle agreement, right now it is 5 degrees, I am guessing it is tight enough. |
|
@pavankum thanks for highlighting (thought I had set this tighter, but looks like I only did this for the tests/locally). Yes, I tried locally with much tighter checks but I've now updated so that a) the tolerance is much tighter (0.1 degrees) and b) an error is raised if the deviation is greater (rather than the previous warning). |
|
Thank you for the clarification @fjclark ! |
|
No problem! I've been playing around with this a bit today for the 3mer minimisations, and found that the very high force constant does slow down the minimisation by up to ~ 10x (up to ~ 50 ms). This is noticable for 2D scans where you avoid reparameterising/ recreating OpenMM systems and run 24 * 24 minimisations for the same system, but currently it doesn't matter much here as this is still faster than creating the systems, which is done once for every minimisation. |
mattwthompson
left a comment
mattwthompson
left a comment
There was a problem hiding this comment.
I've gone through everything except the most important changes - yammbs/torsion/_minimize.py and tests, which I will come back to later - with only minor suggestions for improvement. The refactors you put together look really slick
| if "Constraints" in force_field.registered_parameter_handlers: | ||
| logger.info("Deregistering Constraints handler from SMIRNOFF force field") | ||
| force_field.deregister_parameter_handler("Constraints") |
There was a problem hiding this comment.
We should document this (I forgot this was the default beavior)
| # TODO: Remove this? | ||
| context.computeVirtualSites() |
There was a problem hiding this comment.
Open to removing it as long as we're not running dynamics, but I think it's likely to be harmless most of the time
We lost a lot of time with this ~gotcha: openmm/openmm#3736 (comment)
| # Log initial positions | ||
| initial_positions = ( | ||
| context.getState(getPositions=True) | ||
| .getPositions( | ||
| asNumpy=True, | ||
| ) | ||
| .value_in_unit(openmm.unit.angstrom) | ||
| ) | ||
| logger.debug(f"Initial positions (Angstrom): {initial_positions}") | ||
|
|
There was a problem hiding this comment.
This looks handy but I'm liable to forget this exists - could you add some developer/poweruser-facing docs about what is and is not logged, and at what level?
I'm thinking a new docs/logging.md file would be the best place. No need to make it comprehensive or polished - better if you don't spend much time on it, really - I just want this information to not be buried
|
|
||
| return smirnoff_force_field.create_openmm_system(molecule.to_topology()) | ||
| def _smirnoff(molecule: Molecule, force_field_path: str) -> openmm.System: | ||
| from openff.toolkit import ForceField |
There was a problem hiding this comment.
Nit: probably not worth lazy-loading here
| # The toolkit does a poor job of distinguishing between a string | ||
| # argument being a file that does not exist and a file that it should | ||
| # try to parse (polymorphic input), so just have to clobber whatever |
There was a problem hiding this comment.
👍
For this project, I think making an effort to parse OFFXML as raw strings is a net negative. Hopefully we continue to use only files
| method : Literal["openmm_torsion_atoms_frozen", "openmm_torsion_restrained"] | ||
| Minimization method to use. OpenMM constrains the positions of all | ||
| atoms which define the torsion. |
There was a problem hiding this comment.
Please expand this docstring with a little more detail - it's fine in the context of developing this change, but in the future this will be the entry point for users deciding which one to use and there isn't enough detail for new users to understand what each does (or me to remember in a few months when I forget)
|
Thanks Matt! I'm planning to address these comments in ~ two weeks as I'm focussing on writing up some work at the moment. Another change I'll make in the minimisation code is avoiding creating an entire MDAnalysis universe, which is unnecessary just to get the torsion angles. |
4 participants
This addresses #229.
I've started from #195 but removed everything to do with geomeTRIC, because this contained a bit of refactoring to reduce duplication between the standard and torsion minimisation and setup.
Changes to default behaviour from before:
Thanks!