search

choderalab / feflow

Recipes and protocols for molecular free energy calculations using the openmmtools/perses and Open Free Energy toolkits
MIT License
10 stars 1 forks source link

Support version 1.0 for openfe and gufe #38

Closed ijpulidos closed 1 month ago

ijpulidos commented 3 months ago

This PR supersedes #33.

Solves #31 Solves #22

It aims to add the following changes in order to support the 1.0 version/rc of gufe and openfe.

TODO:

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 79.54545% with 9 lines in your changes are missing coverage. Please review.

Project coverage is 85.84%. Comparing base (05ab4bc) to head (7b107c1). Report is 4 commits behind head on main.

Files Patch % Lines
feflow/settings/integrators.py 74.28% 9 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #38 +/- ## ========================================== - Coverage 92.06% 85.84% -6.23% ========================================== Files 7 10 +3 Lines 416 1399 +983 ========================================== + Hits 383 1201 +818 - Misses 33 198 +165 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

ijpulidos commented 3 months ago

This one should be now pretty much ready to go.

The failing tests are the ones with "bad" mappings. Now the LigandAtomMapping implementation checks for boundaries, so the way we were forcing a "failed dag execution" is no longer working. We would have to think about better ways to test a failed dag execution (I think it's desired to have a "negative" test). Is the openfe package doing a similar test?

On the other hand, we should probably wait for a 1.0 release of both gufe and openfe before merging this into main.

IAlibay commented 3 months ago

Awesome work here @ijpulidos ! I'll try to have a look tomorrow morning ahead of our call.

dotsdl commented 1 month ago

@IAlibay I will add the partial charge setting (and not setting) approach used in the OpenFE RelativeHybridTopologyProtocol; that will be critical for users of alchemiscale-fah.

dotsdl commented 1 month ago

@IAlibay can you work with @ijpulidos to get the settings changes that you have identified into this PR? That will avoid painful migrations later on, especially if we already know the changes we want made.

ijpulidos commented 1 month ago

As discussed in the dev meetings, I think we should aim this PR to be the minimal set of changes that would support the released 1.0 versions of openfe and gufe. This is just a convenient way to be able to handle the scope of each PR in a more granular way and avoid trying to change too much in each.

I have created issues for the remaining changes suggested in https://github.com/choderalab/feflow/pull/38#pullrequestreview-2093798032 and added them to the next milestone

This should be ready for a new review, considering some of the suggestions in previous reviews are to be solved in subsequent PRs, as discussed.

ijpulidos commented 1 month ago

Please note that the CI tests are failing due to the temporary drop of support for GAFF (that's also why it's succeeding for older python 3.9 version). These fails should be independent of the set of changes in this PR as far as I can tell.