Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughA new geometry module is introduced with two atom structure manipulation utilities: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #479 +/- ##
==========================================
+ Coverage 82.69% 83.04% +0.35%
==========================================
Files 25 26 +1
Lines 1832 1870 +38
==========================================
+ Hits 1515 1553 +38
Misses 317 317 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/structuretoolkit/build/geometry.py (3)
98-100: Clarify iteration semantics in docstring.With
iterations=N, the function performs up toN+1merge passes (one initial pass plus N recursive calls). The current docstring says "up toiterationstimes" which understates the actual count by one.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/geometry.py` around lines 98 - 100, The docstring for the merge function is inaccurate: with iterations=N the code runs the initial merge plus N recursive calls (N+1 total passes). Update the merge(...) docstring to state the exact semantics (e.g., "performs up to iterations+1 merge passes: the initial pass plus up to N recursive passes") and clarify whether the count includes the initial pass or not; reference the merge function and its iterations parameter so readers understand the relationship to the recursive calls in the function body.
38-41: Rename ambiguous variableIand guard against division by zero.
- The variable name
Iis ambiguous per PEP 8 (can be confused with1orl). Considerclose_maskormask.- If two atoms occupy the exact same position (
dd[I]contains zeros), division at line 41 will produceinf/nan.♻️ Proposed fix
- I = dd < min_dist + close_mask = dd < min_dist - vv = neigh.vecs[I, 0, :] - vv /= dd[I, None] + vv = neigh.vecs[close_mask, 0, :] + dists = dd[close_mask, None] + # Avoid division by zero for coincident atoms + dists = np.maximum(dists, 1e-10) + vv = vv / dists - disp = np.clip(min_dist - dd[I], 0, step_size) + disp = np.clip(min_dist - dd[close_mask], 0, step_size) displacement = disp[:, None] * vv # (N_close, 3) - structure.positions[I, axis] -= displacement[:, axis] + structure.positions[close_mask, axis] -= displacement[:, axis]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/geometry.py` around lines 38 - 41, Rename ambiguous variable I to a clearer name like close_mask (or mask) where it is defined (currently I = dd < min_dist), and before normalizing neigh.vecs[close_mask, 0, :] by dd[close_mask], guard against zeros by replacing zero distances with 1 (or using np.where/dd + epsilon) so you do not divide by zero; ensure the normalization uses dd_safe = dd[close_mask, None] (or similar) and apply it to neigh.vecs to avoid producing inf/nan in the vector normalization.
16-27: Docstring is missingaxisandinplaceparameters.The function accepts
axisandinplaceparameters but they are not documented in the docstring.📝 Proposed docstring update
"""Displace atoms to avoid minimum overlap. Args: structure (:class:`ase.Atoms`): structure to modify min_dist (float): Minimum distance to enforce between atoms step_size (float): Maximum distance to displace atoms in one step + axis (int or None): + If given, only displace atoms along this axis (0, 1, or 2). + Defaults to ``None`` (all axes). + inplace (bool): + If ``True``, modify structure in place; otherwise return a copy. + Defaults to ``False``. iterations (int): Maximum number of displacements made before giving up + + Returns: + :class:`ase.Atoms`: The modified structure. + + Raises: + RuntimeError: If convergence is not achieved within ``iterations``. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/structuretoolkit/build/geometry.py` around lines 16 - 27, The docstring for the displace-atoms function in src/structuretoolkit/build/geometry.py omits documentation for the axis and inplace parameters; update the docstring to include an Args entry for axis (e.g., axis: None, int or tuple/list of ints — axes to consider for displacement; None means all axes) and for inplace (bool — whether to modify the provided ASE Atoms object in place or operate on and return a copy), document their types and default values, and update the Returns section to state what is returned when inplace is False vs True (e.g., modified Atoms returned when not inplace, otherwise None or the original object).tests/test_geometry.py (1)
110-123: Replace EN DASH with HYPHEN-MINUS in comment.The comment on line 112 uses EN DASH (
–) instead of standard HYPHEN-MINUS (-). This is flagged by Ruff (RUF003).📝 Proposed fix
def test_iterations_zero_stops_early(self): """With iterations=0 only one pass runs; further clashes are left unresolved.""" - # Three atoms in a row: 0–1 and 1–2 clash. First pass merges 0+1 → 0.25. + # Three atoms in a row: 0-1 and 1-2 clash. First pass merges 0+1 -> 0.25.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_geometry.py` around lines 110 - 123, The comment in test_iterations_zero_stops_early contains an EN DASH (U+2013) in the phrase "0–1 and 1–2 clash" which Ruff flags; edit the comment in the test_iterations_zero_stops_early function and replace the EN DASH with a standard ASCII hyphen-minus so it reads "0-1 and 1-2 clash" (also scan the same comment for any other non-ASCII dash characters and normalize them to hyphen-minus if present).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/structuretoolkit/build/geometry.py`:
- Around line 54-56: The type hints in the merge function currently reference
ase.Atoms but ase is not imported; update the annotation to avoid the undefined
name by either changing the hints to string literals (e.g., "ase.Atoms") in the
merge signature and any other annotations in this module, or add a conditional
import under typing.TYPE_CHECKING (from typing import TYPE_CHECKING; if
TYPE_CHECKING: import ase) so runtime import is avoided while type checkers
still see ase.Atoms; ensure the merge function signature and any related
annotations consistently use the chosen approach.
In `@tests/test_geometry.py`:
- Around line 71-77: The test test_noop currently compares len(result) to
len(atoms) but merge modifies in place so both are the same object; before
calling merge(atoms) save the original length (e.g., original_len = len(atoms))
and then assert len(result) == original_len instead of comparing to len(atoms);
keep the existing saved original_positions and numpy array equality check as-is
to validate positions after merge.
---
Nitpick comments:
In `@src/structuretoolkit/build/geometry.py`:
- Around line 98-100: The docstring for the merge function is inaccurate: with
iterations=N the code runs the initial merge plus N recursive calls (N+1 total
passes). Update the merge(...) docstring to state the exact semantics (e.g.,
"performs up to iterations+1 merge passes: the initial pass plus up to N
recursive passes") and clarify whether the count includes the initial pass or
not; reference the merge function and its iterations parameter so readers
understand the relationship to the recursive calls in the function body.
- Around line 38-41: Rename ambiguous variable I to a clearer name like
close_mask (or mask) where it is defined (currently I = dd < min_dist), and
before normalizing neigh.vecs[close_mask, 0, :] by dd[close_mask], guard against
zeros by replacing zero distances with 1 (or using np.where/dd + epsilon) so you
do not divide by zero; ensure the normalization uses dd_safe = dd[close_mask,
None] (or similar) and apply it to neigh.vecs to avoid producing inf/nan in the
vector normalization.
- Around line 16-27: The docstring for the displace-atoms function in
src/structuretoolkit/build/geometry.py omits documentation for the axis and
inplace parameters; update the docstring to include an Args entry for axis
(e.g., axis: None, int or tuple/list of ints — axes to consider for
displacement; None means all axes) and for inplace (bool — whether to modify the
provided ASE Atoms object in place or operate on and return a copy), document
their types and default values, and update the Returns section to state what is
returned when inplace is False vs True (e.g., modified Atoms returned when not
inplace, otherwise None or the original object).
In `@tests/test_geometry.py`:
- Around line 110-123: The comment in test_iterations_zero_stops_early contains
an EN DASH (U+2013) in the phrase "0–1 and 1–2 clash" which Ruff flags; edit the
comment in the test_iterations_zero_stops_early function and replace the EN DASH
with a standard ASCII hyphen-minus so it reads "0-1 and 1-2 clash" (also scan
the same comment for any other non-ASCII dash characters and normalize them to
hyphen-minus if present).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b96163a5-4eb2-4806-ab71-76b9f118bd7a
📒 Files selected for processing (2)
src/structuretoolkit/build/geometry.pytests/test_geometry.py
Comment on lines
+54
to
+56
| def merge( | ||
| structure: "ase.Atoms", cutoff: float = 1.8, iterations: int = 10 | ||
| ) -> "ase.Atoms": |
There was a problem hiding this comment.
Fix undefined ase in type hints.
The type hints reference ase.Atoms but ase is not imported. Since this is used only for documentation purposes, use a string literal or add a TYPE_CHECKING import.
🔧 Proposed fix using string literal (simplest)
def merge(
- structure: "ase.Atoms", cutoff: float = 1.8, iterations: int = 10
-) -> "ase.Atoms":
+ structure: "Atoms", cutoff: float = 1.8, iterations: int = 10
+) -> "Atoms":Or with proper TYPE_CHECKING import:
+from typing import TYPE_CHECKING
+
+if TYPE_CHECKING:
+ from ase import Atoms
+
import numpy as np
from structuretoolkit.analyse import get_neighbors📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def merge( | |
| structure: "ase.Atoms", cutoff: float = 1.8, iterations: int = 10 | |
| ) -> "ase.Atoms": | |
| def merge( | |
| structure: "Atoms", cutoff: float = 1.8, iterations: int = 10 | |
| ) -> "Atoms": |
🧰 Tools
🪛 Ruff (0.15.9)
[error] 55-55: Undefined name ase
(F821)
[error] 56-56: Undefined name ase
(F821)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/structuretoolkit/build/geometry.py` around lines 54 - 56, The type hints
in the merge function currently reference ase.Atoms but ase is not imported;
update the annotation to avoid the undefined name by either changing the hints
to string literals (e.g., "ase.Atoms") in the merge signature and any other
annotations in this module, or add a conditional import under
typing.TYPE_CHECKING (from typing import TYPE_CHECKING; if TYPE_CHECKING: import
ase) so runtime import is avoided while type checkers still see ase.Atoms;
ensure the merge function signature and any related annotations consistently use
the chosen approach.
Comment on lines
+71
to
+77
| def test_noop(self): | ||
| """Perfect FCC Cu has no contacts below default cutoff; structure is unchanged.""" | ||
| atoms = bulk("Cu", cubic=True).repeat(3) | ||
| original_positions = atoms.positions.copy() | ||
| result = merge(atoms) | ||
| self.assertEqual(len(result), len(atoms)) | ||
| np.testing.assert_array_equal(result.positions, original_positions) |
There was a problem hiding this comment.
Test compares length against same object due to in-place modification.
Since merge modifies the structure in place, atoms and result refer to the same object. The assertion len(result) == len(atoms) is always true regardless of what merge does. Compare against a saved original length instead.
💚 Proposed fix
def test_noop(self):
"""Perfect FCC Cu has no contacts below default cutoff; structure is unchanged."""
atoms = bulk("Cu", cubic=True).repeat(3)
original_positions = atoms.positions.copy()
+ original_len = len(atoms)
result = merge(atoms)
- self.assertEqual(len(result), len(atoms))
+ self.assertEqual(len(result), original_len)
np.testing.assert_array_equal(result.positions, original_positions)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_noop(self): | |
| """Perfect FCC Cu has no contacts below default cutoff; structure is unchanged.""" | |
| atoms = bulk("Cu", cubic=True).repeat(3) | |
| original_positions = atoms.positions.copy() | |
| result = merge(atoms) | |
| self.assertEqual(len(result), len(atoms)) | |
| np.testing.assert_array_equal(result.positions, original_positions) | |
| def test_noop(self): | |
| """Perfect FCC Cu has no contacts below default cutoff; structure is unchanged.""" | |
| atoms = bulk("Cu", cubic=True).repeat(3) | |
| original_positions = atoms.positions.copy() | |
| original_len = len(atoms) | |
| result = merge(atoms) | |
| self.assertEqual(len(result), original_len) | |
| np.testing.assert_array_equal(result.positions, original_positions) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_geometry.py` around lines 71 - 77, The test test_noop currently
compares len(result) to len(atoms) but merge modifies in place so both are the
same object; before calling merge(atoms) save the original length (e.g.,
original_len = len(atoms)) and then assert len(result) == original_len instead
of comparing to len(atoms); keep the existing saved original_positions and numpy
array equality check as-is to validate positions after merge.
samwaseda
reviewed
Apr 7, 2026
Member
samwaseda
left a comment
samwaseda
left a comment
There was a problem hiding this comment.
Can we separate repulse and merge into 2 different PR's? I guess I would like to rewrite merge (especially the name is not particularly easy to understand...)
Contributor
Author
@samwaseda I added the commits separately in #481 and #480 Not sure why this seems to trigger a merge conflict now though. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
repulse()iteratively pushes them further apart.merge()replaces two atoms that are too close by one in the center of their connecting vector.Summary by CodeRabbit
Release Notes
New Features
Tests