Fix/issue 1194 #1230

mikemhenry · 2023-08-23T19:02:26Z

Description

Motivation and context

Resolves #1194

How has this been tested?

Change log

Correctly set cutoff for `CustomNonbondedForce` in `HybridTopologyFactory` when using a non-default cutoff distance.

ijpulidos

The changes look good.

I think we do require a test for this, I guess it would involve extracting the custom nonbonded forces from the openmm system and manually checking its cutoff distance.

After that we should also check the effect of this on a full tyk2 benchmark calculation, just to be on the safe side.

ijpulidos · 2023-08-23T19:31:46Z

perses/annihilation/relative.py

@@ -800,8 +800,10 @@ def _add_nonbonded_force_terms(self, add_custom_sterics_force=True):
        sterics_custom_nonbonded_force = openmm.CustomNonbondedForce(total_sterics_energy)
        if self._softcore_LJ_v2:
            sterics_custom_nonbonded_force.addGlobalParameter("softcore_alpha", self._softcore_LJ_v2_alpha)
+            sterics_custom_nonbonded_force.setCutoffDistance(self.r_cutoff)


I just realized that this should probably exist right after line 800 where the object is instantiated in a single statement, instead of being inside the conditional.

Also, I don't think there's a instance attribute for r_cutoff, we probably want to use the one defined in lines 767 or 775, that in turn don't need to exist separately, these could be computed outside that conditional.

Oh yeah, that's why the CI tests aren't passing https://github.com/choderalab/perses/actions/runs/5955395011/job/16153949168?pr=1230

yes much better!

mikemhenry

Looks good! Thanks!

ijpulidos

LGTM!

ijpulidos · 2023-08-23T23:06:56Z

Actually, I first want to run a tyk2 validation simulation with these changes before merging them

codecov · 2023-08-23T23:11:58Z

Codecov Report

Merging #1230 (b6c29f7) into 0.10.x (aca858f) will increase coverage by 0.02%.
The diff coverage is 76.19%.

ijpulidos · 2023-08-23T23:12:50Z

@zhang-ivy Since you have worked with the topology factories in detail, can you please take a look into these changes? Just to make sure we didn't miss anything. Thanks!

zhang-ivy · 2023-08-24T13:18:43Z

perses/annihilation/relative.py

            standard_nonbonded_force.setReactionFieldDielectric(epsilon_solvent)
            standard_nonbonded_force.setCutoffDistance(r_cutoff)
        elif self._nonbonded_method in [openmm.NonbondedForce.PME, openmm.NonbondedForce.Ewald]:
            _logger.info("\t_add_nonbonded_force_terms: nonbonded_method is PME or Ewald")
            [alpha_ewald, nx, ny, nz] = self._old_system_forces['NonbondedForce'].getPMEParameters()
            delta = self._old_system_forces['NonbondedForce'].getEwaldErrorTolerance()
-            r_cutoff = self._old_system_forces['NonbondedForce'].getCutoffDistance()
-            sterics_energy_expression = self._nonbonded_custom(self._softcore_LJ_v2)
            standard_nonbonded_force.setPMEParameters(alpha_ewald, nx, ny, nz)
            standard_nonbonded_force.setEwaldErrorTolerance(delta)
            standard_nonbonded_force.setCutoffDistance(r_cutoff)


Noting that the cutoff distance was set properly for the standard_nonbonded_force but not the sterics_custom_nonbonded_force

Nothing to change here, just making a note.

zhang-ivy · 2023-08-24T13:26:02Z

perses/tests/test_relative.py

+            # Assert that the nb cutoff distance is different compared to the custom nb forces
+            for custom_force in hybrid_custom_nonbonded_forces:
+                assert custom_force.getCutoffDistance() != \
+                       force.getCutoffDistance(), "Expected different cutoff distances between NB and custom NB forces."


Are these nested for loops necessary? There should only be one standard nonbonded force and one custom nonbonded force in the HybridTopologyFactory objects: https://github.com/choderalab/perses/blob/main/perses/annihilation/relative.py#L37-L38

Seems a bit confusing/unnecessary to check each custom force with respect to each standard nonbonded force.

I did it in this way to make it general in case we change the test system to something that has more than one nonbonded force and also more than one custom nonbonded forces. The assumption here is that all of them should share the same cutoff.

I understand what you mean that the HTF only has one of them (and should only have one of them), but if in the future we want to inherit from this test class, say for example to test other kind of HTFs that may have more than one of the nonbonded/custom forces, then we can use the same code to test them. I hope that makes sense.

zhang-ivy · 2023-08-24T13:28:48Z

Looks good to me! Just added a non-blocking suggestion for the test. Feel free to ignore if you disagree.

mikemhenry · 2023-08-24T13:54:23Z

Thanks!

ijpulidos · 2023-08-24T15:19:43Z

Thanks for the review! Waiting for the tyk2 benchmark results to confirm that these changes are not changing anything else along the way.

ijpulidos · 2023-08-25T05:29:33Z

Results seem correct (unaffected by these changes), we could use this for the release as well.

* run CI on new 0.10.x branch (#1212) * run CI on new 0.10.x branch * Update .github/workflows/CI.yaml Co-authored-by: Iván Pulido <[email protected]> --------- Co-authored-by: Iván Pulido <[email protected]> * print perses version on startup (#1176) * Update comments in `RESTCapableHybridTopologyFactory` (#1189) * update comments * bump ci --------- Co-authored-by: Iván Pulido <[email protected]> Co-authored-by: Mike Henry <[email protected]> * use python executable from env (#1174) * use python executable from env * run tests using the shell * parmed 4 seems to be giving us some issues * actually I think we want the newer parmed * pin pymbar for now * ooof, parmed != pymbar * add some more debugging to figure out how we are getting old parmed * Update devtools/conda-envs/test_env.yaml * see if shell=True fixes it * removed parmed by mistake * Support pymbar 4 (#1173) * switch to using pymbar 3 & 4 support from openmmtools * fix typo on import * fix yaml * switch to using pymbar 4 * missed a pymbar import * missed another one * bump ci * missed a import * go back to how it was * Update devtools/conda-envs/test_env.yaml --------- Co-authored-by: Iván Pulido <[email protected]> * Remove example testing (#1214) * Store initial and final topologies for all phases -- small molecule pipeline (#1210) * Initial and final topologies serialized per phase. * Using properties instead of private attrs. * Fix test * Remove uneeded code/attributes * bump ci * Store phase topologies separately * Fix tests. vacuum topologies expected. * Better docstring. --------- Co-authored-by: Mike Henry <[email protected]> * Improve docker building (#1200) * Added note about example for adding oe license * make docker file much simpler * build images in CI * forgot to add conda-forge * fix permissions on a step * get oe_license file mounted in docker container * mount path must be absolute * setup singulairty to test + fix testing on docker image * add some testing deps to the image * add -v for tests, fix envar * tests are failing, want to test rest of pipeline * use latest openmm version * hardcode perses version for now * bump ci * make sure we can make openeye dir * support a dev build as well * don't hardcode value * fix name clash * forgot to add conda-forge * bump ci * test docker image and fix missing deps * install ps * also push latest tag * don't build on tag since the conda-forge package won't exist yet * don't test the examples * Remove docker deb build, we can do these ourselves * better document container use * build a latest version for apptainer * build with 11.2 to make things more compatable * skip docker test to see if other bits build okay * see if I can get the step to fail if there is an error * skip docker tests to make sure apptainer builds okay * Add mpiplus and mpi4py to docker image * give correct path to oe license * add mpi stuff to docker * clean up diskspace before build * skip tests for singularity now that the only failures come from a package bug * Clean examples -- CLI protein-ligand example for Tyk2 (#1223) * Improving examples dir structure and readme * Adding Tyk2 CLI example * removing new-cli/ripk2 example (deprecated) * fix typo for link * Clarify tyk2 cli example docs * Realtime analysis interval to default to checkpoint interval (#1227) * CI miscellaneous fixes (#1217) * CI minor fixes. Allow codecov to fail. * bump ci --------- Co-authored-by: Mike Henry <[email protected]> * Changing offline freq default to checkpoint interval * Fixing input yaml for example * commenting offline-freq param --------- Co-authored-by: Mike Henry <[email protected]> * MPI example with dipeptide mutation (#1228) * peptide mutation MPI example added * better documentation of motivation * Fix/issue 1194 (#1230) * set cutoff distance in sterics_custom_nonbonded_force * matching cutoff for custom forces. Improving logic. * test for HTF nonbonded cutoff --------- Co-authored-by: Iván Pulido <[email protected]> * Fix/issue 1196 (#1229) * CI miscellaneous fixes (#1217) * CI minor fixes. Allow codecov to fail. * bump ci --------- Co-authored-by: Mike Henry <[email protected]> * added dels to contexts * Update perses/app/setup_relative_calculation.py --------- Co-authored-by: Iván Pulido <[email protected]> * Fix spectator support (#1233) * fix spectator support. Enabling test. * Test to run on GPU CI. * pin <4 for pymbar on GPU --------- Co-authored-by: Iván Pulido <[email protected]> Co-authored-by: Ivy Zhang <[email protected]>

@ijpulidos

From @ijpulidos

@ijpulidos

* Add fix from choderalab/perses#1230 From @ijpulidos * fix empty space issue * Update relative.py

set cutoff distance in sterics_custom_nonbonded_force

f00dc29

mikemhenry changed the base branch from main to 0.10.x August 23, 2023 19:03

mikemhenry requested a review from ijpulidos August 23, 2023 19:04

ijpulidos requested changes Aug 23, 2023

View reviewed changes

ijpulidos reviewed Aug 23, 2023

View reviewed changes

ijpulidos added 2 commits August 23, 2023 15:48

matching cutoff for custom forces. Improving logic.

b5aef9f

test for HTF nonbonded cutoff

b6c29f7

ijpulidos self-requested a review August 23, 2023 22:55

mikemhenry commented Aug 23, 2023

View reviewed changes

ijpulidos approved these changes Aug 23, 2023

View reviewed changes

ijpulidos enabled auto-merge August 23, 2023 23:06

ijpulidos disabled auto-merge August 23, 2023 23:06

ijpulidos requested a review from zhang-ivy August 23, 2023 23:12

zhang-ivy reviewed Aug 24, 2023

View reviewed changes

ijpulidos added this pull request to the merge queue Aug 25, 2023

Merged via the queue into 0.10.x with commit f6ee190 Aug 25, 2023

ijpulidos deleted the fix/issue_1194 branch August 25, 2023 06:02

ijpulidos mentioned this pull request Sep 11, 2023

[HybridTopologyFactory] CustomNonbondedForce cutoff doesn't match NonBondedForce #1194

Closed

IAlibay added a commit to OpenFreeEnergy/openfe that referenced this pull request Jan 1, 2024

Add fix from choderalab/perses#1230

1c900a9

From @ijpulidos

IAlibay mentioned this pull request Jan 1, 2024

Properly set the cutoff distance in HTF nonbonded forces OpenFreeEnergy/openfe#675

Merged

1 task

richardjgowers pushed a commit to OpenFreeEnergy/openfe that referenced this pull request Jan 19, 2024

Properly set the cutoff distance in HTF nonbonded forces (#675)

45a6d28

* Add fix from choderalab/perses#1230 From @ijpulidos * fix empty space issue * Update relative.py

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/issue 1194 #1230

Fix/issue 1194 #1230

mikemhenry commented Aug 23, 2023

ijpulidos left a comment

ijpulidos Aug 23, 2023

ijpulidos Aug 23, 2023

ijpulidos Aug 23, 2023

mikemhenry Aug 23, 2023

mikemhenry left a comment

ijpulidos left a comment

ijpulidos commented Aug 23, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading

ijpulidos commented Aug 23, 2023

zhang-ivy Aug 24, 2023

zhang-ivy Aug 24, 2023

zhang-ivy Aug 24, 2023

zhang-ivy Aug 24, 2023

ijpulidos Aug 24, 2023

ijpulidos Aug 24, 2023

zhang-ivy commented Aug 24, 2023 •

edited

Loading

mikemhenry commented Aug 24, 2023

ijpulidos commented Aug 24, 2023

ijpulidos commented Aug 25, 2023

Fix/issue 1194 #1230

Fix/issue 1194 #1230

Conversation

mikemhenry commented Aug 23, 2023

Description

Motivation and context

How has this been tested?

Change log

ijpulidos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikemhenry left a comment

Choose a reason for hiding this comment

ijpulidos left a comment

Choose a reason for hiding this comment

ijpulidos commented Aug 23, 2023

codecov bot commented Aug 23, 2023 • edited Loading

Codecov Report

ijpulidos commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhang-ivy commented Aug 24, 2023 • edited Loading

mikemhenry commented Aug 24, 2023

ijpulidos commented Aug 24, 2023

ijpulidos commented Aug 25, 2023

codecov bot commented Aug 23, 2023 •

edited

Loading

zhang-ivy commented Aug 24, 2023 •

edited

Loading