Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow multiple mappings #90

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Allow multiple mappings #90

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dd1d639
allow a list of mappings
jthorton Sep 23, 2024
82447f0
validate the mapping and systems, add test
jthorton Sep 23, 2024
55b8765
remove double mapping validation, fix toluene to toluene test
jthorton Sep 24, 2024
cc8735c
Merge branch 'main' into multi_map
IAlibay Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions feflow/protocols/nonequilibrium_cycling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Adapted from perses: https://github.com/choderalab/perses/blob/protocol-neqcyc/perses/protocols/nonequilibrium_cycling.py

from typing import Optional, List, Dict, Any
from typing import Optional, Any, Union
from collections.abc import Iterable
from itertools import chain

Expand Down Expand Up @@ -988,7 +988,7 @@ def _create(
self,
stateA: ChemicalSystem,
stateB: ChemicalSystem,
mapping: Optional[dict[str, ComponentMapping]] = None,
mapping: Optional[Union[ComponentMapping, list[ComponentMapping]]] = None,
extends: Optional[ProtocolDAGResult] = None,
) -> list[ProtocolUnit]:
# Handle parameters
Expand All @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! Would you recommend just using the openfe validate function rather than writing something similar again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

went with the validate method in 82447f0

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# inputs to `ProtocolUnit.__init__` should either be `Gufe` objects
# or JSON-serializable objects
num_cycles = self.settings.num_cycles
Expand Down