Allow multiple mappings #90
Conversation
|
Hello @jthorton! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2024-09-24 15:20:12 UTC |
| @@ -997,6 +997,8 @@ def _create( | |||
| if extends: | |||
| raise NotImplementedError("Can't extend simulations yet") | |||
|
|
|||
| mapping = mapping[0] if isinstance(mapping, list) else mapping # type: ignore | |||
There was a problem hiding this comment.
Doing this silently is, in my opinion, a really bad idea when it comes to the user experience. I.e. imagine passing in a list of mappings hoping you'd get multiple transformations, and then you end up with just one (and you're not even sure which because you didn't get told!).
If we're doing this then we should put in something that raises an error if the list is > 1. That's what we do in openfe - and short-medium term I want to spend time to merge the two protocol's setups (adding in all the validation from openfe), but in a more immediate case it would be best to just have the one line here.
There was a problem hiding this comment.
Agreed! Would you recommend just using the openfe validate function rather than writing something similar again?
There was a problem hiding this comment.
went with the validate method in 82447f0
There was a problem hiding this comment.
I think that should work, we probably should migrate the _validate_alchemical_components into FEFLow, or at least put a note somewhere that we expect the validation to be migrated here in the future.
I think this is very related to #55 but it still needs some work to finish it.
There was a problem hiding this comment.
I would go with the note personally, I woulld prefer spend energies towards merging the setups for the two protocols rather than doing lots of workarounds until we do.
| @@ -997,6 +997,8 @@ def _create( | |||
| if extends: | |||
| raise NotImplementedError("Can't extend simulations yet") | |||
|
|
|||
| mapping = mapping[0] if isinstance(mapping, list) else mapping # type: ignore | |||
There was a problem hiding this comment.
I think that should work, we probably should migrate the _validate_alchemical_components into FEFLow, or at least put a note somewhere that we expect the validation to be migrated here in the future.
I think this is very related to #55 but it still needs some work to finish it.
|
@ijpulidos were you ok with the changes in this PR? @jthorton we hadn't communicated this (we need to document it) but because FEFlow aims to contain different Protocols by different folks, we aim to have the Protocol code owner (in this case Ivan), be the true approver if we can. Probably doesn't matter in this case, but something to try to strive for where possible. |
|
Ah sorry about that I thought the green tick was enough feel free to revert @ijpulidos! |
|
Yes, I think these changes look okay. It makes sense that the components are not exactly the same (but copies of it) in the tests, and that we can also explicitly handle list of mappings. Looks good to me! |
Fixes #89 using the same method as the relative hybrid topology method to select the first mapping if a list is provided.