Skip to content

add new dihedrals indentified by grappa to top #51

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

Open
emerbitz wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add new dihedrals indentified by grappa to top #51

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
29 changes: 15 additions & 14 deletions src/grappa/utils/gromacs_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,8 +197,9 @@ def apply_parameters(top: 'Topology', parameters: Parameters, apply_nrs: Set[str

# find the proper dihedral tuple in the topology that is equivalent to the given tuple
tup = find_proper(tup, top)
if not tup:
raise ValueError(f"Invalid proper dihedral tuple {tup}")
if tup is None:
tup = tuple(idx)
logging.warning(f"Adding new proper dihedral {tup} that was not in the original topology.")
Comment on lines +200 to +202
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.

dihedral_dict = {}
for ii in range(len(parameters.proper_ks[i])):
Expand Down Expand Up @@ -295,10 +296,10 @@ def parameterize_topology(
apply_parameters(current_topology, parameters, build_nrs)
return current_topology

def find_angle(tup: tuple, top: 'Topology') -> Tuple[int, int, int]:
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.
Comment on lines +299 to 301
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
If no equivalent tuple is found, it logs a warning and returns False.
If no equivalent tuple is found, it logs a warning and returns None.
"""
if not top.angles.get(tup):
# try equivalent tuples using the symmetries of the angle:
Expand All @@ -311,21 +312,21 @@ def find_angle(tup: tuple, top: 'Topology') -> Tuple[int, int, int]:

if len(tups_in_topology) == 0:
logging.warning(
f"Ignored parameters with invalid ids: {tup} for angles"
f"No equivalent tuple found for {tup} in angles from topology."
)
return None
elif len(tups_in_topology) > 1:
logging.warning(
f"Multiple equivalent tuples found for {tup} in angles"
f"Multiple equivalent tuples found for {tup} in angles from topology."
)
else:
tup = tups_in_topology[0]
return tup

def find_bond(tup: tuple, top: 'Topology') -> Tuple[int, int]:
def find_bond(tup: tuple, top: 'Topology') -> Tuple[int, int] | None:
"""
Find the bond tuple in the topology that is equivalent to the given tuple.
If no equivalent tuple is found, it logs a warning and returns False.
If no equivalent tuple is found, it logs a warning and returns None.
"""
if not top.bonds.get(tup):
# try equivalent tuples using the symmetries of the bond:
Expand All @@ -338,22 +339,22 @@ def find_bond(tup: tuple, top: 'Topology') -> Tuple[int, int]:

if len(tups_in_topology) == 0:
logging.warning(
f"Ignored parameters with invalid ids: {tup} for bonds"
f"No equivalent tuple found for {tup} in bonds from topology."
)
return None
elif len(tups_in_topology) > 1:
logging.warning(
f"Multiple equivalent tuples found for {tup} in bonds"
f"Multiple equivalent tuples found for {tup} in bonds from topology."
)
else:
tup = tups_in_topology[0]
return tup


def find_proper(tup: tuple, top: 'Topology') -> Tuple[int, int, int, int]:
def find_proper(tup: tuple, top: 'Topology') -> Tuple[int, int, int, int] | None:
"""
Find the proper dihedral tuple in the topology that is equivalent to the given tuple.
If no equivalent tuple is found, it logs a warning and returns False.
If no equivalent tuple is found, it logs a warning and returns None.
"""

if not top.proper_dihedrals.get(tup):
Expand All @@ -370,12 +371,12 @@ def find_proper(tup: tuple, top: 'Topology') -> Tuple[int, int, int, int]:

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."
)
Comment on lines 372 to 375
Copy link

Copilot AI Mar 26, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
return None
elif len(tups_in_topology) > 1:
logging.warning(
f"Multiple equivalent tuples found for {tup} in proper dihedrals"
f"Multiple equivalent tuples found for {tup} in proper dihedrals from topology."
)
else:
tup = tups_in_topology[0]
Expand Down
Loading