Skip to content

Conversation

@IAlibay
Copy link
Member

@IAlibay IAlibay commented Dec 5, 2025

Fixes #1739

Checklist

  • All new code is appropriately documented (user-facing code must have complete docstrings).
  • Added a news entry, or the changes are not user-facing.
  • Ran pre-commit by making a comment with pre-commit.ci autofix before requesting review.

Manual Tests: these are slow so don't need to be run every commit, only before merging and when relevant changes are made (generally at reviewer-discretion).

Developers certificate of origin

@IAlibay IAlibay changed the title Migrate validation to Protocol._validate in HybridTopologyProtocol [WIP] Migrate validation to Protocol._validate in HybridTopologyProtocol Dec 5, 2025
@IAlibay IAlibay changed the title [WIP] Migrate validation to Protocol._validate in HybridTopologyProtocol Migrate validation to Protocol._validate in HybridTopologyProtocol Jan 2, 2026
@IAlibay
Copy link
Member Author

IAlibay commented Jan 3, 2026

pre-commit.ci autofix

@github-actions
Copy link

github-actions bot commented Jan 3, 2026

No API break detected ✅

@IAlibay IAlibay mentioned this pull request Jan 3, 2026
6 tasks
Comment on lines +538 to +545
if m.componentA not in alchemical_components["stateA"]:
raise ValueError(
f"Mapping componentA {m.componentA} not in alchemical components of stateA"
)
if m.componentB not in alchemical_components["stateB"]:
raise ValueError(
f"Mapping componentB {m.componentB} not in alchemical components of stateB"
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could maybe squash the repeated errors into a single loop?

Suggested change
if m.componentA not in alchemical_components["stateA"]:
raise ValueError(
f"Mapping componentA {m.componentA} not in alchemical components of stateA"
)
if m.componentB not in alchemical_components["stateB"]:
raise ValueError(
f"Mapping componentB {m.componentB} not in alchemical components of stateB"
)
for label in ["A", "B"]:
if getattr(m, f"component{label}") not in alchemical_components[f"state{label}"]:
raise ValueError(f"Mapping component{label} {gatattr(m, f'component{label}') not in alchemical components of state{label}")

* If an explicit charge correction is attempted and the
nonbonded method is not PME.
* If the absolute charge difference is greater than one
and an explicit charge correction is attempted.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
and an explicit charge correction is attempted.
and an explicit charge correction is attempted.
* If an explicit charge correction is attempted and there is no solvent present.

Raises
------
ValueError
* If the
Copy link
Collaborator

Choose a reason for hiding this comment

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

Needs finishing

Copy link
Member Author

Choose a reason for hiding this comment

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

Aaahh sorry I obviously gave up on the docs some point and never came back - thanks for pointing this out!

* Mappings which involve element changes in core atoms
"""
# if a single mapping is provided, convert to list
if isinstance(mapping, ComponentMapping):
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is done for us in the default_validate method on the base class so we could skip it.

Copy link
Collaborator

@jthorton jthorton left a comment

Choose a reason for hiding this comment

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

Great to see this being used, left a couple of optional things you might consider changing otherwise LGTM!

Copy link
Contributor

@hannahbaumann hannahbaumann left a comment

Choose a reason for hiding this comment

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

Thanks @IAlibay , love the validate method! Just some small spelling things.


if len(clashes) > 0:
errmsg = (
"Found SmallMoleculeComponents are are isomorphic "
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Found SmallMoleculeComponents are are isomorphic "
"Found SmallMoleculeComponents that are isomorphic "

return

if solvent_component is None:
errmsg = "Cannot use eplicit charge correction without solvent"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
errmsg = "Cannot use eplicit charge correction without solvent"
errmsg = "Cannot use explicit charge correction without solvent"

self._validate_endstates(stateA, stateB)

# Get alchemical components & validate them + mapping
# Valildate the mapping
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Valildate the mapping
# Validate the mapping


# Validate charge difference
# Note: validation depends on the mapping & solvent component checks
if stateA.contains(SolventComponent):
Copy link
Contributor

Choose a reason for hiding this comment

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

More a note to self, we'll have to see how to best do all these validations with the explicit solvent component.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is where we will need to make an explicit solvent component be a subclasses of both protein and solvent components

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is where we will need to make an explicit solvent component be a subclasses of both protein and solvent components

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate HybridTopologyProtocol to use validate

4 participants