CLI adaptive settings for charge change transformations #1053
Conversation
|
🚨 API breaking changes detected! 🚨 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1053 +/- ##
==========================================
- Coverage 94.55% 92.84% -1.72%
==========================================
Files 134 135 +1
Lines 10008 10061 +53
==========================================
- Hits 9463 9341 -122
- Misses 545 720 +175
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🚨 API breaking changes detected! 🚨 |
atravitz
left a comment
atravitz
left a comment
There was a problem hiding this comment.
Just a few questions/comments
| with pytest.warns(UserWarning, match="Charge changing transformation between ligands lig_40 and lig_41"): | ||
| result = runner.invoke(plan_rbfe_network, args) | ||
|
|
||
| print(result.output) |
There was a problem hiding this comment.
I don't think we need this print statement here.
There was a problem hiding this comment.
Good catch, this is what happens when I copy and paste a test!
There was a problem hiding this comment.
I still see the print statement, just putting it here in case you forgot to remove it, if not, please ignore this comment!
openfe/setup/alchemical_network_planner/relative_alchemical_network_planner.py
Outdated
Show resolved
Hide resolved
openfe/setup/alchemical_network_planner/relative_alchemical_network_planner.py
Outdated
Show resolved
Hide resolved
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @jthorton , lgtm, just the few comments!
openfe/setup/alchemical_network_planner/relative_alchemical_network_planner.py
Show resolved
Hide resolved
|
|
||
| yield pdb_path, lig_path | ||
|
|
||
| def test_plan_rbfe_network_charge_changes(cdk8_files): |
There was a problem hiding this comment.
I wonder if it would make sense to have a test with three ligands and two edges, one with charge change and one without to test the "mixed" networks.
There was a problem hiding this comment.
Good idea I was hoping the other tests would catch any breaks in the settings but they don't seem to be checking for it.
| with pytest.warns(UserWarning, match="Charge changing transformation between ligands lig_40 and lig_41"): | ||
| result = runner.invoke(plan_rbfe_network, args) | ||
|
|
||
| print(result.output) |
There was a problem hiding this comment.
I still see the print statement, just putting it here in case you forgot to remove it, if not, please ignore this comment!
|
No API break detected ✅ |
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @jthorton , lgtm!
Fixes #1000 by adding adaptive settings based on if the transformation involves a net charge change, for the default RBFE protocol only.
Checklist
newsentryDevelopers certificate of origin