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.

Sign up for GitHub

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

Jump to bottom

HTF migration #24

Merged
merged 27 commits into from
Mar 8, 2024
Merged

HTF migration #24

merged 27 commits into from
Mar 8, 2024

Conversation

ijpulidos
Copy link
Contributor

@ijpulidos ijpulidos commented Jan 22, 2024 •
edited
Loading

Changes to adopt the HybridTopologyFactory class/object from openfe.

The idea is migrating the class and also adapt it to meet the work from Fleck et al

These changes are a starting point where we can test our implementation of HTF, at least minimally to what perses was testing, and gradually improving upon that.

@ijpulidos
Copy link
Contributor Author

This is still in a very WIP status. I plan to work on this in the following days.

Currently I created a hybrid_topology.py module in utils, but I'm not sure that's the best place to put it. We could probably think about having an alchemy module or relative or something similar.

Currently I copied/pasted the test_relative.py module that perses uses to test the HTF. We would need to drop all the geometry engine and topology proposal things. And adapt the current code to use the feflow object itself.

richardjgowers and others added 23 commits February 27, 2024 15:47
@richardjgowers
remove flatten torsions option
a3be504
@richardjgowers
reinclude fix from openfe #675 (perses #1230)
e604cb6
@richardjgowers
HTF: simplify _add_nonbonded_force_terms
352227e
@ijpulidos
Merge pull request #26 from choderalab/htf-migration_openfe_patches
643b640 
Htf migration openfe patches
@richardjgowers
port openfe version of LambdaProtocol into feflow
c0a12ef 
perses version hasn't been touched since April '22
@richardjgowers
fixup attempt to be clever...
e57eaf4
@richardjgowers
port tests from perses to feflow
c46b0ba 
and modernise to pytest
@richardjgowers
Merge pull request #28 from choderalab/include_LambdaProtocol
f13f272 
port openfe version of LambdaProtocol into feflow
@ijpulidos
Upgraded badges and acknowledgements
dfa67a9
@ijpulidos
Removing non-relevant tests (protein mut., REST, etc.). Style formatt…
68a4dd0 
…ing.
@ijpulidos
using gufe tests path fixture
7649d35
@ijpulidos
Comments on why lists objs are needed
70402ca
@ijpulidos
Start of organized module for HTF testing -- WIP
2941436
@ijpulidos
Renaming to lambda_protocol.py
819b8b4
@ijpulidos
Ingesting tests/utils from perses
f1f3092
@ijpulidos
Need openeye for testing
d247cb2
@ijpulidos
old positions with old system. Fix callable.
bcfc3a8
@ijpulidos
Utility function to extract HTF data from perses top proposal
b804428
@ijpulidos
Adapt HTF signature. Skipping difficult ones.
100e51c
@ijpulidos
Need to pin minimum openfe version for CI
7d59d63
@ijpulidos
Testing convergence difficult overlap
277a10d
@ijpulidos
Modular testing. Software vs science test modules.
a1850b2
@ijpulidos
Skip hard to converge test.
e78d16e
@ijpulidos
Copy link
Contributor Author

Temporarily we will be relying on perses to test many aspects of the HTF. Eventually we want to move away from depending on perses for the tests. Issues will be raised to follow up on this.

I tried organizing/modularizing things a bit, but not sure if this is what we want. I'm imagining this as having one module for more integration/science tests in test_relative.py and another more for the actual code unit tests for the HTF class in test_hybrid_topology.py, if that makes any sense. Thoughts?

For now the LambdaProtocol tests are failing since our implementation doesn't add the default functions automatically as it was done in perses. Do we want to leave it like this?

@ijpulidos ijpulidos marked this pull request as ready for review February 29, 2024 04:34
@ijpulidos ijpulidos changed the title [WIP] HTF migration HTF migration Feb 29, 2024
@ijpulidos ijpulidos mentioned this pull request Feb 29, 2024
Open
IAlibay
IAlibay requested changes Mar 5, 2024
View reviewed changes
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very first glance - I believe we're missing the tests for the virtual sites?

IAlibay
IAlibay reviewed Mar 5, 2024
View reviewed changes
feflow/tests/test_hybrid_topology.py

sys_gen_config = {}
sys_gen_config["forcefields"] = ["amber/ff14SB.xml",
"amber/tip3p_standard.xml",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the best place to deal with environment virtual sites is here - I can probably spend the hour tomorrow porting over a set of tests that will deal with this (it's mostly atom counting and ensuring coordinates & particle properties are set properly).

IAlibay
IAlibay reviewed Mar 5, 2024
View reviewed changes
feflow/utils/hybrid_topology.py
raise ValueError(errmsg)
else:
virtual_site = self._old_system.getVirtualSite(
particle_idx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a random thought - if you like autoformatters @ijpulidos, now might be a good time to pass this file through black or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is whatever my IDE uses (I think it's not black), but you are right, maybe we should set up a pre-commit hooks bot here for formatting the code. Hopefully one that doesn't get in the way so much, I'll try setting that up.

IAlibay
IAlibay reviewed Mar 5, 2024
View reviewed changes
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple more things, omm topology is kinda new / fixed and probably needs at least a smoke test, and we should maybe add the one or two more tests for lambda protocol.

I can add those to my todo list tomorrow morning.

feflow/utils/hybrid_topology.py Show resolved Hide resolved
feflow/utils/hybrid_topology.py Show resolved Hide resolved
feflow/tests/test_hybrid_topology.py Show resolved Hide resolved
feflow/utils/lambda_protocol.py Show resolved Hide resolved
feflow/tests/test_lambdaprotocol.py Show resolved Hide resolved
feflow/tests/test_lambdaprotocol.py Show resolved Hide resolved
@dotsdl dotsdl added this to the 0.1.0 release milestone Mar 5, 2024
This was referenced Mar 6, 2024
Merged
Closed
This was referenced Mar 6, 2024
Open
Merged
@IAlibay
Migrate tests for "new" HTF features (#32)
f3fcf7a 
* Vsites tests

* temporarily try to get CI working

* Add lambda schedule tests

* add basic tests for mdt and omm topology attributes

* try to fix scope issues

* more scope limiting

* fix lambda_schedule check

* missing self entry

* make gufe_data_dir session scoped

* fix to_openmm calls

* Add gitignore and fix tests

* Update ci.yaml
IAlibay
IAlibay approved these changes Mar 8, 2024
View reviewed changes
Copy link
Member

@IAlibay IAlibay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@ijpulidos ijpulidos merged commit 2e300ed into main Mar 8, 2024
4 checks passed
@ijpulidos ijpulidos deleted the htf-migration branch March 8, 2024 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@richardjgowers richardjgowers richardjgowers approved these changes

@IAlibay IAlibay IAlibay approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.1.0 release
Development

Successfully merging this pull request may close these issues.
4 participants
@ijpulidos @richardjgowers @IAlibay @dotsdl