OpenFreeEnergy / gufe

grand unified free energy by OpenFE
https://gufe.readthedocs.io
MIT License
32 stars 9 forks source link

change signature of Protocol.create to have a list of ComponentMappings #260

Closed richardjgowers closed 10 months ago

richardjgowers commented 11 months ago

was previously a dict[str, ComponentMapping] where the strings were arbitrary labels. These arbitrary labels were superfluous. Instead can use the arbitrary labels on ChemicalSystems to label components. (ComponentMappings can (and should) be matched against components in ChemicalSystem using the Component eq operations.)

This now accepts either a single ComponentMapping (covers most cases), or None (covers ABFE), or a list (handles future things?)

also affects Transformation object, which was essentially wrapper around Protocol + ChemicalSystem

codecov[bot] commented 11 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (a426dce) 99.21% compared to head (88c3efb) 99.21%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #260 +/- ## ======================================= Coverage 99.21% 99.21% ======================================= Files 36 36 Lines 1907 1911 +4 ======================================= + Hits 1892 1896 +4 Misses 15 15 ```

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

pep8speaks commented 10 months ago

Hello @richardjgowers! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 115:75: E252 missing whitespace around parameter equals Line 115:76: E252 missing whitespace around parameter equals Line 115:80: E501 line too long (80 > 79 characters) Line 175:75: E252 missing whitespace around parameter equals Line 175:76: E252 missing whitespace around parameter equals Line 175:80: E501 line too long (80 > 79 characters) Line 516:80: E501 line too long (86 > 79 characters)

Line 56:80: E501 line too long (89 > 79 characters) Line 58:80: E501 line too long (90 > 79 characters)

Line 31:80: E501 line too long (82 > 79 characters) Line 71:80: E501 line too long (83 > 79 characters)

Comment last updated at 2024-01-22 14:49:43 UTC
richardjgowers commented 10 months ago

@dwhswenson I think I'm going the other way, I'll make the mapping argument take (edit: either a list of mappings or) a single instance (since this is currently all cases?) and not have to explain that technically we've thought about the idea that multiple mappings might one day be possible/necessary.

IAlibay commented 10 months ago

Especially as we move towards multi-state perturbations, multiple mappings may be important here. I guess the question is - is flexibility better than not?

richardjgowers commented 10 months ago

@IAlibay sorry should've been clearer, it's taking either a single Mapping or many, so zero, one or more. Then you don't have to (but can) pass a list of a single mapping whenever you do a RFE.

IAlibay commented 10 months ago

Ah so Optional[Union[Mapping, List[Mapping]]? That would be great!

IAlibay commented 10 months ago

I should have just read the code instead of reading my emails, sorry...