Skip to content
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

Fix spectator support #1233

Merged
merged 2 commits into from
Sep 12, 2023
Merged

Fix spectator support #1233

merged 2 commits into from
Sep 12, 2023

Conversation

ijpulidos
Copy link
Contributor

Description

These changes remove an unnecessary call to RelativeFEPSetup._setup_complex_phase method, fixing the support for spectator molecules.

Motivation and context

Resolves #1232
Resolves #1219

How has this been tested?

Re-enabling previously skipped test for setting up bace system with spectator molecule.

Change log

Regression error when setting up systems with spectator molecules has been fixed by avoiding early and unnecessary calls to setup methods.

@ijpulidos ijpulidos added this to the 0.10.3 - Bugfix release milestone Sep 8, 2023
@ijpulidos ijpulidos changed the base branch from main to 0.10.x September 8, 2023 20:24
@codecov
Copy link

codecov bot commented Sep 8, 2023

Codecov Report

Merging #1233 (c34c734) into 0.10.x (ff55126) will decrease coverage by 0.01%.
The diff coverage is 33.33%.

Additional details and impacted files

@ijpulidos
Copy link
Contributor Author

The current test takes a considerable amount of time.
https://github.com/choderalab/perses/actions/runs/6126127300/job/16629487704?pr=1233#step:9:349

@mikemhenry
Copy link
Contributor

Can you mark it for GPU or for slow?

@mikemhenry
Copy link
Contributor

@ijpulidos have you ran this locally or should I run this PR on our GPU runner?

@ijpulidos
Copy link
Contributor Author

ijpulidos commented Sep 11, 2023

@ijpulidos have you ran this locally or should I run this PR on our GPU runner?

The test already ran before on CPU (check my previous comment). And yes, I have run it locally with GPU. Though it shouldn't really use the GPU, it would just set the contexts and such, since it isn't running any iterations.

We are just putting it in the GPU CI because it's a slow test, but I guess we could also mark it as slow and have some other CI flow that run the slow tests?

@ijpulidos
Copy link
Contributor Author

By the way, I also made sure that starting an example protein-ligand simulation wasn't affected by this.

@mikemhenry mikemhenry added this pull request to the merge queue Sep 12, 2023
Merged via the queue into 0.10.x with commit 7fb8306 Sep 12, 2023
6 checks passed
@mikemhenry mikemhenry deleted the fix/1232-spectator-support branch September 12, 2023 14:35
github-merge-queue bot pushed a commit that referenced this pull request Sep 14, 2023
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duplicate instances of self._setup_complex_phase() in relative_setup.py Error with spectator ligands
2 participants