Test cofactor networks generated with the CLI #1048
Conversation
|
🚨 API breaking changes detected! 🚨 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1048 +/- ##
==========================================
- Coverage 94.50% 92.80% -1.70%
==========================================
Files 134 134
Lines 9986 9995 +9
==========================================
- Hits 9437 9276 -161
- Misses 549 719 +170
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Python 3.11/12 tests are failing as the name of the cofactor component is not what we expect, see here.
|
|
🚨 API breaking changes detected! 🚨 |
| @@ -112,7 +112,7 @@ def __call__( | |||
| RFEComponentLabels.PROTEIN: self.protein, | |||
| } | |||
| for i, c in enumerate(self.cofactors): | |||
| components.update({f'{RFEComponentLabels.COFACTOR}{i+1}': c}) | |||
| components.update({f'{RFEComponentLabels.COFACTOR.value}{i+1}': c}) | |||
There was a problem hiding this comment.
what is the reason for using the value here instead of using the RFEComponentLabels object itself?
There was a problem hiding this comment.
For some reason using it like this gives an unexpected label, the cofactor is called RFEComponentLabels.COFACTOR1 whereas the ligand and protein components are labelled ligand and protein as expected. See the error for how this looks, I guess its not super important to change as I don't think these labels are used for anything but it doesn't look nice and its not easy to pull out the cofactor with the inconsistent naming.
There was a problem hiding this comment.
within abstract_chemicalsystem_generator.py, if you make the class inherit from StrEnum instead of both str and Enum (i.e.RFEComponentLabels(StrEnum)), it'll solve this!
It looks like StrEnum has some additional handling for string representation, but I don't quite understand the details.
There was a problem hiding this comment.
Ahhh thanks, though I think that the component keys should all be strings going by the type hint in gufe so maybe they should all be using .value?
|
🚨 API breaking changes detected! 🚨 |
Extend the CLI network tests with cofactors to explicitly check that they are included in the transformations.
Checklist
newsentryDevelopers certificate of origin