consequences of gufe #260 change #666

richardjgowers · 2023-12-18T18:02:46Z

all mappings to Protocol.create/Transformation.init are now lists of mappings, not a dict

Developers certificate of origin

I certify that this contribution is covered by the MIT License here and the Developer Certificate of Origin at https://developercertificate.org/.

richardjgowers · 2023-12-18T18:03:19Z

relies on this change: OpenFreeEnergy/gufe#260

this PR being the downstream consequences of this API change

codecov · 2023-12-18T18:09:31Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (48b31c4) 91.33% compared to head (1c9ee3b) 91.33%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #666      +/-   ##
==========================================
- Coverage   91.33%   91.33%   -0.01%     
==========================================
  Files         132      132              
  Lines        9328     9324       -4     
==========================================
- Hits         8520     8516       -4     
  Misses        808      808

Flag	Coverage Δ
fast-tests	`91.33% <100.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

IAlibay

Overall this looks reasonable, although I'm not convinced on the method return change, in fact I think we should go more the other way and remove the "single mapping" error raising. This is especially because I know that Ivan wanted to re-use some of this tooling and maybe it's just overly restrictive for no reason?

IAlibay · 2023-12-28T00:00:04Z

openfe/protocols/openmm_rfe/equil_rfe_methods.py

@@ -243,6 +247,8 @@ def _validate_alchemical_components(
                logger.warning(wmsg)
                warnings.warn(wmsg)  # TODO: remove this once logging is fixed

+    return mapping[0]  # type: ignore


The reason why this by default was not returning mapping[0] was so that a future protocol could re-use this method without enforcing the same behaviour. I.e. a requirement that only a single mapping is necessary is specific to this specific Protocol. If we're enforcing that specific protocols' behaviour then maybe we should just make thsi a class method?

https://github.com/OpenFreeEnergy/openfe/pull/666/files#diff-1ab9cb2879dcf6337e58cc145548646e494814de8535f1c23b56c13ee9162632R209-R213

currently this function only allows a single mapping, so the function returns the only allowable input.

I'm not sure this code is sufficiently complex/interesting that it can't just be rewritten for new protocols, it's just a bunch of checks specific to this Protocol. Trying to make it general/useful for many different cases is probably more effort than just rewriting these checks. The choice of function/staticmethod is not that different?

I'd prefer to keep the function called _validate_X purely for validation (i.e., a void / return None function). I don't expect a function with that name to return something.

It's also better if it is reusable. Personally, I'd recommend splitting the warning check off, but that's out of scope for this PR. The stuff before that, which raises ValueErrors for bad mappings, is extremely reusable, and your typical PhD-student dev might forget some of those checks. Additionally, reusing code that generates error messages is extremely helpful for users searching for the error message (maybe we'll get it documented; even less likely that some random PhD student will make the effort).

richardjgowers · 2024-01-01T15:24:06Z

I can change the function return back, but trying to share very simple functions like this feels like false-sharing where the cost of sharing is actually worse than just rewriting some simple code that is specific for each case.

dwhswenson

My main thing is to turn the validate method back into something that only does validation. If, in OpenFreeEnergy/gufe#260, you take my recommendation to change mapping -> mappings, this PR will obviously need to update for that, too.

trying to share very simple functions like this feels like false-sharing where the cost of sharing is actually worse than just rewriting some simple code that is specific for each case.

I'm not sure how this has any analogy with false sharing. In that case, you're trying NOT to share a resource and are surprised by the fact that you are, and that leads to performance issues. If I'm missing a connection here, please clarify.

If someone doesn't want to use our validator, they don't need to. If they know it exists and it meets their needs, they use it. If they don't know it exists, then they write their own — that seems like the worst loss of time, and I'm not sure how having something potentially reusable in our makes that worse. It isn't even public API now, so there's no real issue here.

In any case, my argument is that the validator should only validate.

dwhswenson · 2024-01-09T20:23:22Z

openfe/protocols/openmm_rfe/equil_rfe_methods.py

@@ -243,6 +247,8 @@ def _validate_alchemical_components(
                logger.warning(wmsg)
                warnings.warn(wmsg)  # TODO: remove this once logging is fixed

+    return mapping[0]  # type: ignore


I'd prefer to keep the function called _validate_X purely for validation (i.e., a void / return None function). I don't expect a function with that name to return something.

It's also better if it is reusable. Personally, I'd recommend splitting the warning check off, but that's out of scope for this PR. The stuff before that, which raises ValueErrors for bad mappings, is extremely reusable, and your typical PhD-student dev might forget some of those checks. Additionally, reusing code that generates error messages is extremely helpful for users searching for the error message (maybe we'll get it documented; even less likely that some random PhD student will make the effort).

IAlibay

blocking on upstream decision

IAlibay · 2024-01-19T11:37:37Z

openfe/protocols/openmm_afe/equil_solvation_afe_method.py

@@ -521,7 +521,7 @@ def _create(
        self,
        stateA: ChemicalSystem,
        stateB: ChemicalSystem,
-        mapping: Optional[Dict[str, gufe.ComponentMapping]] = None,
+        mapping: list[gufe.ComponentMapping],


As discussed on the gufe PR, I would very much like this not to be how we do things going forward - it's very unintuitive from a Pythonic sense.

pep8speaks · 2024-01-19T12:30:22Z

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

In the file openfe/protocols/openmm_afe/equil_solvation_afe_method.py:

Line 605:80: E501 line too long (92 > 79 characters)

In the file openfe/protocols/openmm_rfe/equil_rfe_methods.py:

Line 474:80: E501 line too long (85 > 79 characters)
Line 486:80: E501 line too long (92 > 79 characters)

Comment last updated at 2024-02-07 10:34:48 UTC

IAlibay

So the current approve in gufe 260 is to do Optional[mapping, list[mapping]], which is convenient from a user stand point.

My original take is that definitely "no mapping" == None (because that's what I'd expect in Python), but I don't have any strong opinions on list[mapping] vs Union[mapping, list[mapping]]. We should however make changes here to account for the single mapping case.

IAlibay · 2024-01-22T15:36:02Z

openfe/protocols/openmm_rfe/equil_rfe_methods.py

@@ -203,14 +203,13 @@ def _validate_alchemical_components(
    """
    # Check mapping
    # For now we only allow for a single mapping, this will likely change
-    if mapping is None or len(mapping.values()) > 1:
+    if mapping is None or len(mapping) > 1:


What happens if you do len on a single ComponentMapping?

all mappings to Protocol.create/Transformation.__init__ are now lists of mappings, not a dict

Just be safe and check against ComponentMapping

Just be safe and check against ComponentMapping also guard against len 0 list case

# Conflicts: # openfe/protocols/openmm_afe/equil_solvation_afe_method.py # openfe/protocols/openmm_rfe/equil_rfe_methods.py # openfe/tests/protocols/test_openmm_equil_rfe_protocols.py

handle when mapping is single ComponentMapping not list

IAlibay · 2024-02-08T08:42:23Z

@dwhswenson - I believe your concerns have been addressed so I'll go ahead with a merge. If they haven't please do raise an issue and we can tackle further changes in a follow-up PR.

concerns addressed

Fix devtools for #666 changes

richardjgowers requested review from dwhswenson and IAlibay December 18, 2023 18:02

richardjgowers added this to the 1.0.0 milestone Dec 18, 2023

IAlibay reviewed Dec 28, 2023

View reviewed changes

richardjgowers mentioned this pull request Jan 1, 2024

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

Merged

dwhswenson previously requested changes Jan 9, 2024

View reviewed changes

IAlibay requested changes Jan 19, 2024

View reviewed changes

IAlibay reviewed Jan 22, 2024

View reviewed changes

richardjgowers force-pushed the gufe_260_consequences branch 2 times, most recently from 6ddaac9 to de909f6 Compare January 24, 2024 09:49

richardjgowers added 7 commits January 26, 2024 13:55

consequences of gufe #260 change

0ba9345

all mappings to Protocol.create/Transformation.__init__ are now lists of mappings, not a dict

final fixups for new _create/create gufe API

57fc2fe

allow None for mappings in _create

e1b1c9c

allow None for mappings in _create

a0651bf

fix case where mapping is a single ComponentMapping

14d4fe6

what if someone used a tuple of mappings?

b224d71

Just be safe and check against ComponentMapping

what if someone used a tuple of mappings?

7a89fb2

Just be safe and check against ComponentMapping also guard against len 0 list case

richardjgowers force-pushed the gufe_260_consequences branch from cf9d449 to 7a89fb2 Compare January 26, 2024 13:55

richardjgowers requested review from IAlibay and dwhswenson January 30, 2024 09:58

IAlibay and others added 3 commits January 30, 2024 10:46

Merge branch 'main' into gufe_260_consequences

47a6850

Merge branch 'main' into gufe_260_consequences

58af8fe

# Conflicts: # openfe/protocols/openmm_afe/equil_solvation_afe_method.py # openfe/protocols/openmm_rfe/equil_rfe_methods.py # openfe/tests/protocols/test_openmm_equil_rfe_protocols.py

fixups following merge

1c9ee3b

handle when mapping is single ComponentMapping not list

richardjgowers assigned IAlibay Feb 7, 2024

IAlibay approved these changes Feb 8, 2024

View reviewed changes

IAlibay merged commit 0815897 into main Feb 8, 2024
9 checks passed

IAlibay deleted the gufe_260_consequences branch February 8, 2024 08:42

IAlibay added a commit that referenced this pull request Feb 8, 2024

Fix devtools for #666 changes

8b7d340

richardjgowers added a commit that referenced this pull request Feb 8, 2024

Merge pull request #707 from OpenFreeEnergy/fix-devtools

1917f5a

Fix devtools for #666 changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

consequences of gufe #260 change #666

consequences of gufe #260 change #666

richardjgowers commented Dec 18, 2023

richardjgowers commented Dec 18, 2023

codecov bot commented Dec 18, 2023 •

edited

Loading

IAlibay left a comment •

edited

Loading

IAlibay Dec 28, 2023

richardjgowers Jan 1, 2024

dwhswenson Jan 9, 2024

richardjgowers commented Jan 1, 2024

dwhswenson left a comment

dwhswenson Jan 9, 2024

IAlibay left a comment

IAlibay Jan 19, 2024

pep8speaks commented Jan 19, 2024 •

edited

Loading

IAlibay left a comment

IAlibay Jan 22, 2024

IAlibay commented Feb 8, 2024

consequences of gufe #260 change #666

consequences of gufe #260 change #666

Conversation

richardjgowers commented Dec 18, 2023

Developers certificate of origin

richardjgowers commented Dec 18, 2023

codecov bot commented Dec 18, 2023 • edited Loading

Codecov Report

IAlibay left a comment • edited Loading

Choose a reason for hiding this comment

IAlibay Dec 28, 2023

Choose a reason for hiding this comment

richardjgowers Jan 1, 2024

Choose a reason for hiding this comment

dwhswenson Jan 9, 2024

Choose a reason for hiding this comment

richardjgowers commented Jan 1, 2024

dwhswenson left a comment

Choose a reason for hiding this comment

dwhswenson Jan 9, 2024

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Jan 19, 2024

Choose a reason for hiding this comment

pep8speaks commented Jan 19, 2024 • edited Loading

Comment last updated at 2024-02-07 10:34:48 UTC

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay Jan 22, 2024

Choose a reason for hiding this comment

IAlibay commented Feb 8, 2024

codecov bot commented Dec 18, 2023 •

edited

Loading

IAlibay left a comment •

edited

Loading

pep8speaks commented Jan 19, 2024 •

edited

Loading