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

Jump to bottom

Allow multiple mappings #90

Merged
merged 4 commits into from
Sep 25, 2024
Merged

Allow multiple mappings #90

Show file tree
Hide file tree
Changes from 2 commits
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
15 changes: 13 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,15 +988,26 @@ 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]:
from openfe.protocols.openmm_rfe.equil_rfe_methods import (
_validate_alchemical_components,
)
from openfe.protocols.openmm_utils import system_validation

# Handle parameters
if mapping is None:
jthorton marked this conversation as resolved.
Show resolved Hide resolved
raise ValueError("`mapping` is required for this Protocol")
if extends:
raise NotImplementedError("Can't extend simulations yet")

# Get alchemical components & validate them + mapping
alchem_comps = system_validation.get_alchemical_components(stateA, stateB)
# raise an error if we have more than one mapping
_validate_alchemical_components(alchem_comps, mapping)
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
21 changes: 21 additions & 0 deletions feflow/tests/test_nonequilibrium_cycling.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -491,6 +491,27 @@ def test_failing_partial_charge_assign(

execute_DAG(dag, shared_basedir=shared, scratch_basedir=scratch)

def test_error_with_multiple_mappings(
self,
protocol_short,
benzene_vacuum_system,
toluene_vacuum_system,
mapping_benzene_toluene,
):
"""
Make sure that when a list of mappings is passed that an error is raised.
"""

with pytest.raises(
ValueError, match="A single LigandAtomMapping is expected for this Protocol"
):
_ = protocol_short.create(
stateA=benzene_vacuum_system,
stateB=toluene_vacuum_system,
name="Test protocol",
mapping=[mapping_benzene_toluene, mapping_benzene_toluene],
)


class TestSetupUnit:
def test_setup_user_charges(
Expand Down
Loading