add surface builder and other interaction nodes #431
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BuildSurface
participant ASE
participant zntrack
participant HDF5
User->>BuildSurface: instantiate with parameters
User->>BuildSurface: call run()
BuildSurface->>ASE: create bulk structure
BuildSurface->>ASE: generate surface slab with Miller indices, layers
BuildSurface->>ASE: add vacuum padding
BuildSurface->>zntrack: manage output path and parameter tracking
BuildSurface->>HDF5: save surface slab frames
User->>BuildSurface: access frames property
BuildSurface->>HDF5: load saved surface frames
BuildSurface-->>User: return list of ASE Atoms objects
sequenceDiagram
participant User
participant AddAdsorbate
participant ASE
participant zntrack
participant HDF5
User->>AddAdsorbate: instantiate with slab, adsorbate data, parameters
User->>AddAdsorbate: call run()
AddAdsorbate->>AddAdsorbate: select adsorbates based on indices
AddAdsorbate->>AddAdsorbate: compute adsorbate positions avoiding collisions
AddAdsorbate->>ASE: place adsorbates on slab at computed positions
AddAdsorbate->>zntrack: manage output path and parameter tracking
AddAdsorbate->>HDF5: save slab with adsorbates frames
User->>AddAdsorbate: access frames property
AddAdsorbate->>HDF5: load saved frames
AddAdsorbate-->>User: return list of ASE Atoms objects with adsorbates
sequenceDiagram
participant User
participant AtomSelector
participant AtomConstraint
participant ASEGeoOpt
participant MACEMPModel
User->>AtomSelector: define selection criteria
AtomSelector-->>User: return atom indices
User->>AtomConstraint: create constraint using AtomSelector
AtomConstraint-->>User: ASE constraint object
User->>ASEGeoOpt: run geometry optimization with constraints and MACEMPModel
ASEGeoOpt->>MACEMPModel: compute forces
ASEGeoOpt-->>User: optimized structure and trajectory
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
![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 (10)
ipsuite/atom_selection/constraints.py (2)
75-87: Minor inefficiency – duplicate selector callsEach
get_constraintpath callsselect()twice per selector. Cache the result to avoid redundant work, especially for expensive selectors (e.g. neighbor analysis).
121-140: Long error strings flagged by RuffLines 129 & 133 exceed 90 chars (E501). Split them for CI green:
- f"Bond pair {i}, atom1 selection must select exactly 1 atom, got {len(indices1)}" + ( + f"Bond pair {i}: atom1 selection must select exactly one atom " + f"(got {len(indices1)})" + )Repeat for the second string.
ipsuite/atom_selection/selections.py (1)
246-257: Surface selection is O(N²) and ignores PBCThe double loop over atoms can be slow (>10 k atoms) and mis-counts across cell boundaries.
Considerase.neighborlist.NeighborListorscipy.spatial.cKDTreewith PBC support for an O(N)–ish solution and correct periodic handling.tests/integration/test_atom_selection_integration.py (1)
6-15: Mark heavy integration test as slowThe test spins up an ML model and geometry optimisation – likely minutes. Add
@pytest.mark.slow(or project equivalent) to keep routine CI fast.tests/unit_tests/atom_selection/test_selections.py (1)
13-15: Make randomness deterministic
np.random.randwithout a fixed seed can lead to rare flaky failures in geometry-based tests.
Setnp.random.seed(0)at module setup or use fixed coordinates.ipsuite/configuration_generation/surface_builder.py (3)
288-293: Propagate original stack-trace for easier debugging.Re-raise with
from errto distinguish bugs in caller code from bugs in theexceptblock.- except IndexError: - raise ValueError( + except IndexError as err: + raise ValueError( f"Index {index} is out of bounds for adsorbate list {i} " f"with length {len(adsorbate_list)}" - ) + ) from err
135-151: Unused private helper.
_get_com_positionis never called after implementation. Delete it or integrate it (e.g., usemol_index=Nonewith COM placement) to keep codebase lean.
234-246: Non-deterministic placement due to uncontrolled RNG.
np.random.uniformis called without seeding, making node outputs non-reproducible across runs.
Inject a deterministic seed via project config or expose arandom_seedparam.tests/integration/test_surface_builder.py (2)
234-239: Variable never used – replace with “_” to silence F841.- adsorbed_system = ips.AddAdsorbate( + _ = ips.AddAdsorbate(
204-206: Line >90 chars flagged by Ruff (E501).Consider breaking the comprehension across lines to keep within the project’s style limits.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
ipsuite/__init__.pyi(5 hunks)ipsuite/atom_selection/__init__.py(1 hunks)ipsuite/atom_selection/constraints.py(1 hunks)ipsuite/atom_selection/selections.py(1 hunks)ipsuite/configuration_generation/__init__.pyi(1 hunks)ipsuite/configuration_generation/surface_builder.py(1 hunks)ipsuite/interfaces.py(4 hunks)pyproject.toml(1 hunks)tests/integration/test_atom_selection_integration.py(1 hunks)tests/integration/test_surface_builder.py(1 hunks)tests/unit_tests/atom_selection/test_selections.py(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- pyproject.toml
- ipsuite/atom_selection/init.py
- ipsuite/init.pyi
🚧 Files skipped from review as they are similar to previous changes (1)
- ipsuite/configuration_generation/init.pyi
🧰 Additional context used
🧬 Code Graph Analysis (1)
ipsuite/atom_selection/constraints.py (1)
ipsuite/interfaces.py (5)
interfaces(131-133)AtomConstraint(88-108)AtomSelector(65-85)get_constraint(95-108)select(72-85)
🪛 Ruff (0.12.2)
ipsuite/atom_selection/constraints.py
129-129: Line too long (101 > 90)
(E501)
133-133: Line too long (101 > 90)
(E501)
ipsuite/configuration_generation/surface_builder.py
290-293: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
297-297: run is too complex (11 > 10)
(C901)
tests/integration/test_surface_builder.py
217-217: Line too long (96 > 90)
(E501)
234-234: Local variable adsorbed_system is assigned to but never used
Remove assignment to unused variable adsorbed_system
(F841)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: pytest (3.11, ubuntu-latest)
🔇 Additional comments (3)
ipsuite/interfaces.py (2)
65-85: Interfaces look solidThe two new protocols are well-scoped, clearly documented, and typed. No issues spotted.
131-134: Potential breaking change ininterfaces()
interfaces()now returns{"ipsuite": __all__}instead of{"ipsuite.interfaces": __all__}.
If any downstream code relies on the previous key it will silently break. Please double-check call-sites or provide a backward-compatibility shim.ipsuite/atom_selection/constraints.py (1)
36-40: Guard clause OK
FixAtomsConstraintcorrectly raises when nothing is selected – nice touch.
| surface_atoms = surface( | ||
| lattice=bulk_atoms, indices=self.indices, layers=self.layers | ||
| ) |
There was a problem hiding this comment.
Incorrect keyword argument – this will raise TypeError.
ase.build.surface signature is surface(atoms, indices, layers, …).
Passing the bulk object via lattice= is invalid and will crash at runtime.
- surface_atoms = surface(
- lattice=bulk_atoms, indices=self.indices, layers=self.layers
- )
+surface_atoms = surface(bulk_atoms, indices=self.indices, layers=self.layers)📝 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.
| surface_atoms = surface( | |
| lattice=bulk_atoms, indices=self.indices, layers=self.layers | |
| ) | |
| surface_atoms = surface(bulk_atoms, indices=self.indices, layers=self.layers) |
🤖 Prompt for AI Agents
In ipsuite/configuration_generation/surface_builder.py around lines 69 to 71,
the call to the surface function incorrectly uses the keyword argument
lattice=bulk_atoms, which is not valid and will cause a TypeError. Replace
lattice=bulk_atoms with the correct positional argument atoms=bulk_atoms by
passing bulk_atoms as the first positional argument without a keyword, followed
by indices=self.indices and layers=self.layers as positional arguments to match
the surface function signature.
| distance = np.sqrt((new_pos[0] - ex_x) ** 2 + (new_pos[1] - ex_y) ** 2) | ||
| min_distance = max(new_radius, existing_radii[i]) | ||
| if distance < min_distance: | ||
| return True |
There was a problem hiding this comment.
Collision test uses max instead of sum of radii.
To avoid overlap the allowed minimum distance between two exclusion circles should be r1 + r2, not max(r1, r2).
Using max silently allows collisions when radii differ.
- min_distance = max(new_radius, existing_radii[i])
+ min_distance = new_radius + existing_radii[i]📝 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.
| distance = np.sqrt((new_pos[0] - ex_x) ** 2 + (new_pos[1] - ex_y) ** 2) | |
| min_distance = max(new_radius, existing_radii[i]) | |
| if distance < min_distance: | |
| return True | |
| distance = np.sqrt((new_pos[0] - ex_x) ** 2 + (new_pos[1] - ex_y) ** 2) | |
| - min_distance = max(new_radius, existing_radii[i]) | |
| + min_distance = new_radius + existing_radii[i] | |
| if distance < min_distance: | |
| return True |
🤖 Prompt for AI Agents
In ipsuite/configuration_generation/surface_builder.py around lines 194 to 197,
the collision test incorrectly uses the maximum of the two radii to determine
minimum allowed distance, which can allow overlaps. Replace the use of
max(new_radius, existing_radii[i]) with the sum new_radius + existing_radii[i]
to correctly enforce that the distance between centers must be at least the sum
of their radii to avoid collisions.
Summary by CodeRabbit