Add more padding to fix issue #949 #953

mikemhenry · 2022-03-04T04:14:36Z

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

Increase default padding in XXXX classes

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.

jchodera · 2022-03-04T04:26:50Z

Let's hold off on merging this until we decide on openmm/openmm#3502

codecov · 2022-03-04T04:58:25Z

Codecov Report

Merging #953 (a58953d) into main (06bb0f4) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

zhang-ivy · 2022-03-29T20:09:41Z

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.

… fix/issue_949

mikemhenry · 2022-03-31T01:25:15Z

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:

 >           for i in range(periodic_torsion.getNumTorsions()):
E           AttributeError: 'NonbondedForce' object has no attribute 'getNumTorsions'

From test_flattenedHybridTopologyFactory_energies in perses/tests/test_relative.py

and

>       top_proposal._old_system.getForce(3).setUseDispersionCorrection(False)
E       AttributeError: 'HarmonicAngleForce' object has no attribute 'setUseDispersionCorrection'

From test_HybridTopologyFactory_energies in perses/tests/test_relative.py

I'm going to investigate this and see where it is coming from, but that fix may be in a different PR or upstream.

mikemhenry · 2022-03-31T01:40:37Z

So it looks like setUseDispersionCorrection is only a property of non-bonded forces (makes sense) and likewise it makes sense that AttributeError: 'NonbondedForce' object has no attribute 'getNumTorsions' which means that someone (on nightly) our forces are getting scrambled. So periodic_torsion = system.getForce(2) doesn't pull in a periodic torsion, but instead a non-bonded force.

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.

mikemhenry · 2022-03-31T16:02:06Z

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)

ijpulidos

I wonder if we should also change the default padding options in other places, for example here or here or here

ijpulidos · 2022-03-31T18:19:21Z

perses/tests/utils.py

@@ -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)


Why is it 20 A here instead of 11 A as it was with the other tests?

Good catch! I will double check that one!

mikemhenry · 2022-04-01T00:27:55Z

So since those systems are "real" they seem to work fine, since we run some of these examples through CI.

zhang-ivy · 2022-04-04T13:38:24Z

@mikemhenry :

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

Can you clarify why the test times were previously longer?

mikemhenry · 2022-04-04T20:00:19Z

@mikemhenry :

Good news! I've bumped up the padding and the tests don't run really any slower sunglasses I'm very happy to be wrong about the increased test times, which were due to some bad luck

Can you clarify why the test times were previously longer?

I mention that here:
openmm/openmm#3502 (comment)

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.

zhang-ivy · 2022-04-04T20:37:51Z

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?

mikemhenry · 2022-04-04T20:42:37Z

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
Importing that module caused weird behavior.

mikemhenry · 2022-04-05T12:33:29Z

@ijpulidos I was able to reduce the padding in that one util from 20 A to 16 A but anything smaller fails.
Ready for re-review

zhang-ivy · 2022-04-05T13:35:26Z

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..

ijpulidos · 2022-04-05T18:45:03Z

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.

ijpulidos

This looks good, extra padding doesn't seem to affect the time for tests. Thanks!

zhang-ivy · 2022-04-05T18:49:32Z

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.

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.

ijpulidos · 2022-04-05T19:01:31Z

@zhang-ivy Ah! I see what you mean. Yes, I agree. We probably just need the nightly builds to catch these changes then.

mikemhenry · 2022-04-07T12:32:39Z

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 ?

zhang-ivy · 2022-04-07T13:26:54Z

@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)

mikemhenry · 2022-04-07T13:29:19Z

I'll confirm that the nightly getting pulled in has the PR that is supposed to fix this

jchodera · 2022-04-07T16:03:26Z

@zhang-ivy is right that we shouldn't need to change the padding. The OpenMM nightlies should be the latest master build, which includes openmm/openmm#3537.

The issue must be that openmm/openmm#3537 is an incomplete fix. Perhaps we can open another PR starting from the perses main, figure out what tests are failing and why, and then give some feedback about how openmm/openmm#3537 should be further refined?

mikemhenry · 2022-04-07T16:53:19Z

I double checked our CI that runs on main https://github.com/choderalab/perses/runs/5872047384?check_suite_focus=true and we are running the latest nightly that was built last night, so openmm/openmm#3537 is not a complete fix.

jchodera · 2022-04-07T18:19:16Z

@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.

mikemhenry · 2022-04-19T22:40:33Z

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

jchodera · 2022-04-20T12:46:33Z

Thanks, @mikemhenry! Filed a simpler version of this as openmm/openmm#3502 (comment)

fixes a few tests by increasing the padding

823af3d

mikemhenry added 2 commits March 4, 2022 09:46

bump padding, but realy slows down tests

6131b47

Merge branch 'main' into fix/issue_949

893c4bc

jchodera mentioned this pull request Mar 30, 2022

Address change in padding behavior for addSolvent openmm/openmm#3502

Closed

mikemhenry added 3 commits March 29, 2022 22:14

Merge remote-tracking branch 'refs/remotes/origin/fix/issue_949' into…

8552a2d

… fix/issue_949

fix protein_counterion_topology tests

72242cd

bump padding a bit in test 'fixture'

26ad3f1

mikemhenry requested review from ijpulidos and jchodera March 31, 2022 16:00

ijpulidos reviewed Mar 31, 2022

View reviewed changes

see if 11 A works here

c1dcfe6

mikemhenry added 2 commits April 4, 2022 13:47

needed a bit more padding

9b2e415

test_generate_endpoint_thermodynamic_states needs 16 A padding

49c60d5

mikemhenry requested a review from ijpulidos April 5, 2022 12:31

ijpulidos approved these changes Apr 5, 2022

View reviewed changes

Merge branch 'main' into fix/issue_949

65ef29b

Merge branch 'main' into fix/issue_949

a58953d

mikemhenry enabled auto-merge (squash) May 3, 2022 16:38

mikemhenry merged commit 285205d into main May 3, 2022

mikemhenry deleted the fix/issue_949 branch May 3, 2022 16:58

zhang-ivy mentioned this pull request Jun 14, 2022

Examine source of large error bars in 0.10.0 release #1041

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more padding to fix issue #949 #953

Add more padding to fix issue #949 #953

mikemhenry commented Mar 4, 2022

jchodera commented Mar 4, 2022

codecov bot commented Mar 4, 2022 •

edited

Loading

zhang-ivy commented Mar 29, 2022 •

edited

Loading

mikemhenry commented Mar 31, 2022

mikemhenry commented Mar 31, 2022 •

edited

Loading

mikemhenry commented Mar 31, 2022

ijpulidos left a comment

ijpulidos Mar 31, 2022

mikemhenry Apr 1, 2022

mikemhenry commented Apr 1, 2022

zhang-ivy commented Apr 4, 2022

mikemhenry commented Apr 4, 2022

zhang-ivy commented Apr 4, 2022 •

edited

Loading

mikemhenry commented Apr 4, 2022

mikemhenry commented Apr 5, 2022 •

edited

Loading

zhang-ivy commented Apr 5, 2022

ijpulidos commented Apr 5, 2022

ijpulidos left a comment

zhang-ivy commented Apr 5, 2022 •

edited

Loading

ijpulidos commented Apr 5, 2022

mikemhenry commented Apr 7, 2022

zhang-ivy commented Apr 7, 2022

mikemhenry commented Apr 7, 2022

jchodera commented Apr 7, 2022

mikemhenry commented Apr 7, 2022 •

edited

Loading

jchodera commented Apr 7, 2022

mikemhenry commented Apr 19, 2022 •

edited

Loading

jchodera commented Apr 20, 2022

Add more padding to fix issue #949 #953

Add more padding to fix issue #949 #953

Conversation

mikemhenry commented Mar 4, 2022

Description

Motivation and context

How has this been tested?

Change log

jchodera commented Mar 4, 2022

codecov bot commented Mar 4, 2022 • edited Loading

Codecov Report

zhang-ivy commented Mar 29, 2022 • edited Loading

mikemhenry commented Mar 31, 2022

mikemhenry commented Mar 31, 2022 • edited Loading

mikemhenry commented Mar 31, 2022

ijpulidos left a comment

Choose a reason for hiding this comment

ijpulidos Mar 31, 2022

Choose a reason for hiding this comment

mikemhenry Apr 1, 2022

Choose a reason for hiding this comment

mikemhenry commented Apr 1, 2022

zhang-ivy commented Apr 4, 2022

mikemhenry commented Apr 4, 2022

zhang-ivy commented Apr 4, 2022 • edited Loading

mikemhenry commented Apr 4, 2022

mikemhenry commented Apr 5, 2022 • edited Loading

zhang-ivy commented Apr 5, 2022

ijpulidos commented Apr 5, 2022

ijpulidos left a comment

Choose a reason for hiding this comment

zhang-ivy commented Apr 5, 2022 • edited Loading

ijpulidos commented Apr 5, 2022

mikemhenry commented Apr 7, 2022

zhang-ivy commented Apr 7, 2022

mikemhenry commented Apr 7, 2022

jchodera commented Apr 7, 2022

mikemhenry commented Apr 7, 2022 • edited Loading

jchodera commented Apr 7, 2022

mikemhenry commented Apr 19, 2022 • edited Loading

jchodera commented Apr 20, 2022

codecov bot commented Mar 4, 2022 •

edited

Loading

zhang-ivy commented Mar 29, 2022 •

edited

Loading

mikemhenry commented Mar 31, 2022 •

edited

Loading

zhang-ivy commented Apr 4, 2022 •

edited

Loading

mikemhenry commented Apr 5, 2022 •

edited

Loading

zhang-ivy commented Apr 5, 2022 •

edited

Loading

mikemhenry commented Apr 7, 2022 •

edited

Loading

mikemhenry commented Apr 19, 2022 •

edited

Loading