Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the GROMACS topology parameter application flow so that proper dihedrals predicted by Grappa are written into the output topology even when they were not present in the original .top, emitting warnings instead of raising.
Changes:
- Add missing proper dihedral entries to
top.proper_dihedralswhen no equivalent tuple exists in the input topology. - Refine tuple-finding helpers (
find_angle,find_bond,find_proper) to returnNoneand improve warning messages. - Minor typing and messaging adjustments.
Comments suppressed due to low confidence (1)
src/grappa/utils/gromacs_utils.py:383
- When multiple equivalent tuples are found, the function only logs a warning and then returns the original input tuple (which is known not to exist in the topology because of the initial
getcheck). Callers will then write a new dihedral entry under a tuple that wasn’t in the topology, potentially duplicating interactions instead of updating an existing one. Prefer selecting a deterministic match (e.g., the first match) or returning None / raising so the caller can handle the ambiguity explicitly.
elif len(tups_in_topology) > 1:
logging.warning(
f"Multiple equivalent tuples found for {tup} in proper dihedrals from topology."
)
else:
tup = tups_in_topology[0]
return tup
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if tup is None: | ||
| tup = tuple(idx) | ||
| logging.warning(f"Adding new proper dihedral {tup} that was not in the original topology.") |
There was a problem hiding this comment.
This change introduces new behavior (adding proper dihedrals that weren’t present in the input topology) but there doesn’t appear to be a targeted test asserting that: (1) parameterization no longer raises, (2) the new dihedral is present in the written topology, and (3) a warning is emitted. Adding a small unit/integration test that constructs a minimal Topology+Parameters case would help prevent regressions (especially around tuple ordering/equivalence).
| if len(tups_in_topology) == 0: | ||
| logging.warning( | ||
| f"Ignored parameters with invalid ids: {tup} for proper dihedrals" | ||
| f"No equivalent tuple found for {tup} in proper dihedrals from topology." | ||
| ) |
There was a problem hiding this comment.
With the new behavior in apply_parameters (warning + add dihedral when not found), find_proper now also emits a warning before returning None. Since find_proper is only called from apply_parameters in this module, this results in duplicate warnings for the same event and can make logs noisy in large systems. Consider letting find_proper be a pure lookup (no warning on miss) and have the caller own the single warning/message when deciding what to do on None.
| def find_angle(tup: tuple, top: 'Topology') -> Tuple[int, int, int] | None: | ||
| """ | ||
| Find the angle tuple in the topology that is equivalent to the given tuple. |
There was a problem hiding this comment.
The return type annotations here use PEP 604 unions ("| None"), but the rest of the codebase predominantly uses typing.Optional/typing.Union (and there’s no declared minimum Python version). If the project needs to remain compatible with Python <3.10, this will be a syntax error. Consider switching these annotations to Optional[...] to preserve compatibility and consistency; also, the tuple elements appear to be atom "nr" strings in this module, so the Tuple[int,...] annotations are likely inaccurate.
Background
So far, Grappa raises a ValueError if parameters are predicted for proper dihedrals that are not in the original top.
Changes