Fix validation in AHFE Protocol #1572
Conversation
IAlibay
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1572 +/- ##
==========================================
- Coverage 95.05% 93.40% -1.66%
==========================================
Files 172 174 +2
Lines 14492 14537 +45
==========================================
- Hits 13776 13578 -198
- Misses 716 959 +243
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:
|
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay , this looks great! Only left two small comments.
Should we adapt the ABFE and SepTop protocols as well?
| # Check that there are no protein components | ||
| if stateA.contains(ProteinComponent) or stateB.contains(ProteinComponent): | ||
| errmsg = ("Protein components are not allowed for " | ||
| "absolute solvation free energies") |
There was a problem hiding this comment.
Maybe are not allowed in the AbsoluteSolvationProtocol to be more specific?
There was a problem hiding this comment.
Unless you feel strongly about this, I would prefer not, couple of reasons:
- This entire thing gets subclassed by the ASFEProtocol in pontibus. So I expect it to be shared.
- The error will stem from execution of the transformation, so it should (hopefully) be obvious what Protocol is emitting the error.
- There are a lot of other warnings/errors all over the place that aren't that specific, especially if the above is untrue, that might need a rethink of how we do errors.
The ABFE Protocol also implements For SepTop and HybridTop, yes eventually but not for v1.7. As-in, I don't consider not having it a blocker for SepTop, and the HybridTop one would require more effort / rewritting (validation there is messy because it was our first Protocol). |
hannahbaumann
left a comment
hannahbaumann
left a comment
There was a problem hiding this comment.
Thanks @IAlibay , lgtm!
| * Solvent must always be present in both end states. | ||
| """ | ||
| # Check that there are no protein components | ||
| if stateA.contains(ProteinComponent) or stateB.contains(ProteinComponent): |
There was a problem hiding this comment.
Love to see the new contains methods out in the wild!
There was a problem hiding this comment.
Both the contains and diff methods are very convenient!
|
|
||
| def test_create_default_settings(): | ||
| settings = AbsoluteSolvationProtocol.default_settings() | ||
| assert settings |
There was a problem hiding this comment.
Should we be checking some of these settings in case they are changed accidentally?
There was a problem hiding this comment.
Field deletion gets checked via serialization tests (under the results check and also the tokenization ones). Field addition (if non-optional) are also checked that way.
Field changes or new defaults aren't (they used to be via testing the GufeTokenizable key, but that wasn't stable). I'm not against testing some settings, but that probably should be done else on a settings by settings manner.
|
No API break detected ✅ |
validatemethod.Checklist
newsentryDevelopers certificate of origin