-
Notifications
You must be signed in to change notification settings - Fork 51
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 more padding to fix issue #949 #953
Conversation
Let's hold off on merging this until we decide on openmm/openmm#3502 |
Note from dev sync today: Mike says that some of the tests are still failing because the padding is still not large enough, but increasing the padding even more for all tests will slow the tests down. They are already taking > 45 min. Beforehand, they took ~15 min. |
Good news! I've bumped up the padding and the tests don't run really any slower 😎 I'm very happy to be wrong about the increased test times, which were due to some bad luck (see openmm/openmm#3502 (comment)) However, we now have two new issues:
From and
From I'm going to investigate this and see where it is coming from, but that fix may be in a different PR or upstream. |
So it looks like I propose we make a new issue and merge this PR in to fix the issue with solvent padding. One open question, do we change the default padding in other parts of the code? For larger systems, the padding seems to work fine. |
This is ready for review! The nightly tests that are still failing are then fixed in #976 (there is one energy test failing but I think that one is just flaky (since it doesn't fail on both python versions) but if it is a problem I will address it later in another PR) |
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.
perses/tests/utils.py
Outdated
@@ -522,7 +522,7 @@ def generate_solvated_hybrid_test_topology(current_mol_name="naphthalene", prop | |||
hs = [atom for atom in modeller.topology.atoms() if atom.element.symbol in ['H'] and atom.residue.name not in ['MOL','OLD','NEW']] | |||
modeller.delete(hs) | |||
modeller.addHydrogens(forcefield=system_generator.forcefield) | |||
modeller.addSolvent(system_generator.forcefield, model='tip3p', padding=9.0*unit.angstroms) | |||
modeller.addSolvent(system_generator.forcefield, model='tip3p', padding=20.0*unit.angstroms) |
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.
Why is it 20 A here instead of 11 A as it was with the other tests?
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.
Good catch! I will double check that one!
So since those systems are "real" they seem to work fine, since we run some of these examples through CI. |
Can you clarify why the test times were previously longer? |
I mention that here: I hadn't merged my fix to the multistate samplers which were causing the context cache to get a miss every time which slowed everything down. |
@mikemhenry : I don't see the fix in the issue comment you linked, could you point me to the PR? |
@zhang-ivy Here you go! https://github.com/choderalab/perses/pull/961/files Because of this line https://github.com/choderalab/perses/blob/0.9.5/perses/samplers/multistate.py#L10 |
@ijpulidos I was able to reduce the padding in that one util from 20 A to 16 A but anything smaller fails. |
Noting that Peter has attempted to help resolve this with this PR: openmm/openmm#3537 that John mentioned yesterday. I'm surprised that we need to increase padding at all, given this new PR.. |
So, from what I can see the problem is what Peter mentions here. Is it the case that Openmm is now making smaller boxes so we have the problem with the cutoff being greater than half of the box size. Then that's why increasing the padding works for these tests, we are just making larger boxes such that cutoff is smaller than half of its size. I think this is fine for the tests since they are actually fast tests (not a bottleneck), as far as I can see. |
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, extra padding doesn't seem to affect the time for tests. Thanks!
Yes, I agree with everything you said.. but check out this new PR from peter. The description for the PR says: "First, it revises the method for selecting box size. When building a water box around a very small solute, it will never make the box width less than 2*padding." My comment earlier today was referencing this revision. Peter's change should fix our problem, meaning we no longer need to increase the padding. If there is no difference in the speed of the tests, then I guess it doesn't hurt to bump the padding, but in principle, we shouldn't have to, with Peter's revision. |
@zhang-ivy Ah! I see what you mean. Yes, I agree. We probably just need the nightly builds to catch these changes then. |
Last night's CI run is still throwing padding issues, and I really like to get our CI passing again. Given the low impact this has on our CI times, is this okay to merge @zhang-ivy @jchodera @ijpulidos ? |
@mikemhenry : I'm still not sure why we need to increase the padding. Happy to be overrridden on this decision, but see my comment here: #953 (comment) |
I'll confirm that the nightly getting pulled in has the PR that is supposed to fix this |
@zhang-ivy is right that we shouldn't need to change the padding. The OpenMM nightlies should be the latest The issue must be that openmm/openmm#3537 is an incomplete fix. Perhaps we can open another PR starting from the perses |
I double checked our CI that runs on |
@mikemhenry Can you give me an example of a simple test that is failing due to the box size issues? I can dive into this in more detail and propose further fixes to the OpenMM feature. |
import numpy as np
from openmm import app, unit
from perses.tests.test_topology_proposal import (
generate_atp, generate_dipeptide_top_pos_sys)
new_res = "ASP"
charge_diff = 1
# Make a vacuum system
atp, system_generator = generate_atp(phase="vacuum")
# Make a solvated system/topology/positions with modeller
modeller = app.Modeller(atp.topology, atp.positions)
modeller.addSolvent(
system_generator.forcefield,
model="tip3p",
padding=9 * unit.angstroms,
ionicStrength=0.15 * unit.molar,
)
solvated_topology = modeller.getTopology()
solvated_positions = modeller.getPositions()
# Canonicalize the solvated positions: turn tuples into np.array
atp.positions = unit.quantity.Quantity(
value=np.array(
[
list(atom_pos)
for atom_pos in solvated_positions.value_in_unit_system(unit.md_unit_system)
]
),
unit=unit.nanometers,
)
atp.topology = solvated_topology
atp.system = system_generator.create_system(atp.topology)
# Make a topology proposal and generate new positions
top_proposal, new_pos, _, _ = generate_dipeptide_top_pos_sys(
topology=atp.topology,
new_res=new_res,
system=atp.system,
positions=atp.positions,
system_generator=system_generator,
conduct_geometry_prop=True,
conduct_htf_prop=False,
validate_energy_bookkeeping=True,
) This fails with the currently nightly openmm build, let me know if you need this trimmed down more @jchodera |
Thanks, @mikemhenry! Filed a simpler version of this as openmm/openmm#3502 (comment) |
Description
Fix issue #949
Motivation and context
Due to a change in openmm, we need to increase the padding for our simulation boxes.
Resolves #949
How has this been tested?
Tested in CI
Change log
I'm trying to increase the padding as little as possible in our tests/defaults to pass without increasing CI time significantly. So it is a bit slow/some trial and error to find that sweet spot.