Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
for more information, see https://pre-commit.ci
📝 WalkthroughWalkthroughA new geometry utility module introduces the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 #480 +/- ##
==========================================
+ Coverage 82.69% 82.90% +0.20%
==========================================
Files 25 26 +1
Lines 1832 1854 +22
==========================================
+ Hits 1515 1537 +22
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: 1
🧹 Nitpick comments (2)
src/structuretoolkit/build/geometry.py (2)
16-27: Docstring is missingaxisandinplaceparameters.The docstring documents
structure,min_dist,step_size, anditerations, but omitsaxisandinplace.📝 Proposed docstring additions
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, list, or None): + Coordinate axis/axes along which atoms may be displaced. + If None, all axes are used. + inplace (bool): + If True, modify structure in place; otherwise return a copy. iterations (int): Maximum number of displacements made before giving up + + Returns: + :class:`ase.Atoms`: The modified structure (same object if inplace=True). + + Raises: + RuntimeError: If minimum distance is not achieved within the iteration limit. """🤖 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 function is missing descriptions for the axis and inplace parameters; update the docstring in src/structuretoolkit/build/geometry.py (the function that begins with "Displace atoms to avoid minimum overlap") to include brief entries explaining axis (which axis or axes to constrain displacement to, allowed values and default) and inplace (whether the operation modifies the provided ase.Atoms in place or returns a new object and its default), matching the style and parameter order used for structure, min_dist, step_size, and iterations so callers understand behaviour and defaults.
38-38: Rename ambiguous variableIto a descriptive name.Per Ruff E741, single uppercase
Iis easily confused with1orl. A descriptive name liketoo_closeorclose_maskimproves readability.♻️ Proposed fix
- I = dd < min_dist + too_close = dd < min_dist - vv = neigh.vecs[I, 0, :] - vv /= dd[I, None] + vv = neigh.vecs[too_close, 0, :] + vv /= dd[too_close, None] - disp = np.clip(min_dist - dd[I], 0, step_size) + disp = np.clip(min_dist - dd[too_close], 0, step_size) displacement = disp[:, None] * vv # (N_close, 3) - structure.positions[I, axis] -= displacement[:, axis] + structure.positions[too_close, 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` at line 38, Rename the ambiguous single-letter variable I used in the distance comparison to a descriptive name (e.g., too_close or close_mask) inside the function that computes dd < min_dist so it's clear this is a boolean mask of points closer than min_dist; update all subsequent uses of I accordingly (search for the symbol I in this module/function and replace with the chosen name) and run tests/linters to ensure the Ruff E741 warning is resolved.
🤖 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 40-41: The code divides vv (from neigh.vecs[I,0,:]) by dd[I,None]
which can be zero if atoms coincide; add a guard before the in-place division
that detects dd[I] == 0 and either raises a clear ValueError (e.g. "Structure
contains coincident atoms") or skips/handles those vectors to avoid inf/nan.
Locate the normalization code using the symbols vv, neigh.vecs, dd, and I and
implement a check such as if dd[I] == 0: raise ValueError(...) (or set vv[...] =
0 and continue) to prevent division-by-zero; you can also add a precondition
using get_neighbors(...).distances to validate input earlier.
---
Nitpick comments:
In `@src/structuretoolkit/build/geometry.py`:
- Around line 16-27: The docstring for the displace function is missing
descriptions for the axis and inplace parameters; update the docstring in
src/structuretoolkit/build/geometry.py (the function that begins with "Displace
atoms to avoid minimum overlap") to include brief entries explaining axis (which
axis or axes to constrain displacement to, allowed values and default) and
inplace (whether the operation modifies the provided ase.Atoms in place or
returns a new object and its default), matching the style and parameter order
used for structure, min_dist, step_size, and iterations so callers understand
behaviour and defaults.
- Line 38: Rename the ambiguous single-letter variable I used in the distance
comparison to a descriptive name (e.g., too_close or close_mask) inside the
function that computes dd < min_dist so it's clear this is a boolean mask of
points closer than min_dist; update all subsequent uses of I accordingly (search
for the symbol I in this module/function and replace with the chosen name) and
run tests/linters to ensure the Ruff E741 warning is resolved.
🪄 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: f970238a-6b86-4064-b990-185197356d42
📒 Files selected for processing (2)
src/structuretoolkit/build/geometry.pytests/test_geometry.py
|
@copilot Complete the docstring of |
…ses sections Agent-Logs-Url: https://github.com/pyiron/structuretoolkit/sessions/0ba06c20-b234-4542-9c47-90adee68ccc1 Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Done in 26b9b08. The docstring now documents all parameters ( |
|
@copilot Add type hints |
Agent-Logs-Url: https://github.com/pyiron/structuretoolkit/sessions/dc485354-9225-480f-9de6-7924e42d047d Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces a new geometry utility (repulse) in structuretoolkit.build to iteratively displace atoms so that all nearest-neighbor distances meet a configurable minimum distance threshold, along with unit tests validating expected behaviors.
Changes:
- Added
repulse()to iteratively separate atoms that are closer thanmin_dist, with optional axis-restricted motion and in-place vs copy semantics. - Added
TestRepulseunit tests covering no-op behavior, in-place behavior, iteration exhaustion, axis restriction, andmin_distenforcement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/structuretoolkit/build/geometry.py |
Implements the new repulse() geometric relaxation utility and exports it via __all__. |
tests/test_geometry.py |
Adds unit tests exercising core repulse() behavior and edge cases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| vv = neigh.vecs[I, 0, :] | ||
| vv /= dd[I, None] | ||
|
|
There was a problem hiding this comment.
vv /= dd[I, None] will divide by zero if two atoms occupy the same position (distance 0), producing inf/NaN displacements and corrupting positions. Please handle dd == 0 explicitly (e.g., raise a clear error or choose a fallback displacement direction with an epsilon).
There was a problem hiding this comment.
Done in f0f24e0. Coincident atoms (distance == 0) now get a fallback displacement direction ([1.0, 0.0, 0.0]) instead of dividing by zero, and the convergence check was also fixed to use >= so exact equality is treated as converged.
There was a problem hiding this comment.
@copilot This makes no sense, because both of the atoms would be moved by the same vector. Give the direction depending on the index of the neighboring atom (which is stored in neigh.indices)
There was a problem hiding this comment.
Fixed in 992f343. For coincident atoms, the fallback now uses neigh.indices to determine direction: if atom_index < neighbor_index, the atom gets +x; otherwise -x. This guarantees the two coincident atoms are pushed in opposite directions and separate correctly.
| ``iterations`` steps. | ||
| """ | ||
| if not inplace: | ||
| structure = structure.copy() |
There was a problem hiding this comment.
axis is documented as 0/1/2, but invalid values currently surface as a NumPy IndexError at the assignment. It would be better to validate axis up front and raise a ValueError with a clear message when axis is not None and not in {0,1,2}.
| structure = structure.copy() | |
| structure = structure.copy() | |
| if axis is not None and axis not in {0, 1, 2}: | |
| raise ValueError("axis must be None or one of {0, 1, 2}") |
| return structure | ||
|
|
||
|
|
||
| __all__ = ["repulse"] |
There was a problem hiding this comment.
If repulse is intended to be part of the public build API (similar to create_mesh, grainboundary, etc.), consider re-exporting it from structuretoolkit.build.__init__ (and possibly the top-level structuretoolkit.__init__) so users can import it consistently.
| for _ in range(iterations): | ||
| neigh = get_neighbors(structure, num_neighbors=1) | ||
| dd = neigh.distances[:, 0] | ||
| if dd.min() > min_dist: |
There was a problem hiding this comment.
| if dd.min() > min_dist: | |
| if dd.min() >= min_dist: |
…in repulse() Agent-Logs-Url: https://github.com/pyiron/structuretoolkit/sessions/f6dc014c-fb3c-4ea9-b3b3-90c9dc891ce6 Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
…ices Agent-Logs-Url: https://github.com/pyiron/structuretoolkit/sessions/496fe42a-e9e5-44e1-9179-c59ad7037f28 Co-authored-by: samwaseda <37879103+samwaseda@users.noreply.github.com>
Adds
repulse()tostructuretoolkit.build.geometry— an iterative utility that displaces atoms until all nearest-neighbour distances meet a configurable minimum threshold.Key design points
min_distto its nearest neighbour is pushed away along the interatomic vector by up tostep_sizeper iterationdistance == 0): fallback direction is assigned usingneigh.indices— atom with lower index gets+x, higher index gets-x, so the pair separates rather than translating togetherdd.min() >= min_dist(strict>would false-fail when a distance lands exactly on the threshold)axis: optionally restrict displacements to a single Cartesian axis (0/1/2)inplace: modify the input structure directly or return a copy (default)RuntimeErrorif not converged withiniterationssteps