search

choderalab / perses

Experiments with expanded ensembles to explore chemical space
http://perses.readthedocs.io
MIT License
179 stars 50 forks source link

Fix spectator support #1233

Closed ijpulidos closed 10 months ago

ijpulidos commented 10 months ago

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.
codecov[bot] commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

Can you mark it for GPU or for slow?

mikemhenry commented 10 months ago

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

ijpulidos commented 10 months ago

@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 commented 10 months ago

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