Skip to content

feat: add molify.repeat to tile periodic boxes with their connectivity - #131

Merged
PythonFZ merged 3 commits into
mainfrom
feat/repeat-connectivity
Oct 2, 2026
Merged

PythonFZ merged 3 commits into
mainfrom
feat/repeat-connectivity

Conversation

@PythonFZ

@PythonFZ PythonFZ commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

What changed

  • New molify.repeat(atoms, rep) -> ase.Atoms in src/molify/repeat.py, exported from molify and listed in __all__.

  • It calls atoms.repeat(rep) for the geometry and rebuilds the index-based info entries:

    • connectivity: each bond (i, j, order) links atom i to the nearest periodic image of atom j. Image shifts come from ase.geometry.find_mic plus cell.scaled_positions, which covers triclinic cells and partial pbc. A pbc axis with a zero-length cell vector counts as non-periodic. Indices are Python int and bond orders Python float or None, also after an extxyz round trip.
    • original_index: tiled so that atom k gets the entry of source atom k % n.
    • smiles is removed, as for molify.pack boxes. Other info keys follow ase. Atoms without connectivity get a plain ase repeat.
  • Explicit ValueError for:

    • an invalid rep (0, negative, float, string, a length other than three). ase returns an empty structure or crashes for these.
    • an original_index of the wrong length.
    • out-of-range bond indices and self-bonds.
    • a bond at least half the shortest vector of the Minkowski-reduced periodic lattice long. In such a cell (e.g. the Si primitive cell) one atom pair bonds through several images, and molify stores one edge per atom pair. Sheared bases of an ordinary lattice repeat fine.

    Numpy integer rep values are accepted. Extxyz ndarray inputs work.

  • docs/source/packmol_tools.ipynb has a new "Repeating a Box" section with captured output.

Why

ase.Atoms.repeat tiles per-atom arrays and copies info as is. On a wrapped 36-atom box, box.repeat((2, 1, 1)) kept 30 bonds where 60 are expected, and ase2rdkit returned 42 fragments where 12 are expected. A stale original_index made ase2networkx raise IndexError.

Test evidence

  • tests/test_repeat.py has 20 tests (37 cases). They check against an independent oracle: the edges of ase2networkx(molify.repeat(atoms, rep)) must equal the distance-based edges of atoms.repeat(rep) with the stored bonds removed. Cases:
    • orthorhombic and triclinic cells, each with rep 1, 2, (2, 1, 1), (1, 2, 3)
    • a sheared basis b = (3L, L, 0) of a cubic box
    • 2D-periodic slab, also with pbc=True along its zero-length third vector
    • distance-based connectivity with None bond orders
    • extxyz round trip
  • Bond orders: every tiled bond carries the order of its source bond (1.0, 1.5 and 2.0 in a benzene + acrylic acid box), and the canonical SMILES fragments come out as the source fragments × 4.
  • Mutation check: the tests fail for a zero or sign-flipped image shift, Fortran image order, an untiled original_index, numpy ints in original_index, dropped, flattened or float(None) bond orders, a missing periodic mask, and a missing self-bond or bond-length check.
  • uv run pytest -q -p no:cacheprovider: 501 passed. uvx prek --all-files: clean.
  • 1800 atoms (1600 bonds) × 27 copies are tiled in about 7 ms.

ase2x.py, smiles2x.py and cli.py are untouched.

Closes #129

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added repeat to repeat a periodic structure along one or more cell directions while preserving its connectivity, including bonds across periodic boundaries and bond orders.
  • Documentation
    • Added a Packmol tools notebook example showing how to double a mixture along a cell direction and inspect atom, bond, and molecule counts.

@PythonFZ PythonFZ added enhancement New feature or request p-2 Priority: normal effort-2 Effort: moderate labels Oct 2, 2026
@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 563db68e-8b89-44ca-aacc-613b473467de

📥 Commits

Reviewing files that changed from the base of the PR and between 367fe78 and e24d00e.

📒 Files selected for processing (4)
  • docs/source/packmol_tools.ipynb
  • src/molify/__init__.py
  • src/molify/repeat.py
  • tests/test_repeat.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

Adds and exports molify.repeat() to repeat ASE structures while extending connectivity across periodic images. The function validates repeat factors and connectivity, preserves bond orders and original indices, and removes smiles from the result.

Changes

Periodic Structure Repetition

Layer / File(s) Summary
Repeat API and connectivity tiling
src/molify/repeat.py, src/molify/__init__.py, docs/source/packmol_tools.ipynb
Adds and exports repeat(). It validates repeat factors and connectivity, maps bonds to periodic images, and preserves bond orders and original_index metadata. The notebook documents and demonstrates repetition.
Repeat behavior validation
tests/test_repeat.py
Tests connectivity, bond orders, repeat shapes, periodic cell types, metadata, input immutability, file round-trips, and invalid inputs.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Merge Risk: ⚪ Minimal · up to e24d0

The new repetition API has no identified issue requiring a fix before merge.

Security Architecture Review

Security architecture risk: 🔵 Low · up to e24d0

The change remains an in-process molecular-data transformation, with no demonstrated new privileged or externally reachable execution path. Its main design considerations are metadata consistency, failure containment, and memory growth for large repetition requests. Dependency copy semantics and concurrent input mutation were not fully verified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The demonstrated exposure is caller-provided molecular data and the calling process's memory and computation. The scoped call chain establishes no tenant, credential, remote-service, or privileged-resource transition; deployment exposure remains outside the available evidence.

Trust Boundaries and Controls

  • observed — Input atom indices and periodic-image ambiguity are checked before rebuilt connectivity is assigned. Downstream graph-to-RDKit conversion retains its existing attribute validation and molecule sanitization controls.

Resilience and Maintainability Implications

  • inferred — Bond-validation or index-conversion failures do not return the locally constructed partial result. The implementation directly updates only result metadata, but ownership guarantees still depend on ASE copy behavior, and reads from a concurrently mutated source are not synchronized.

Hardening Proposals

  • proposed — If exposed to untrusted requests, apply caller-owned atom, bond, and output-size budgets. Consider validating size-independent connectivity constraints before repeated-geometry allocation to reduce avoidable work on rejected inputs.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 3.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding and exporting molify.repeat to tile periodic boxes while preserving connectivity.
Linked Issues check ✅ Passed Issue [#129] requires either ASE repeat connectivity support or a molify.repeat(ase.Atoms, ...) function. The PR adds and exports molify.repeat. The function calls atoms.repeat(rep), tiles conne…
Out of Scope Changes check ✅ Passed The changes stay within issue [#129]. The implementation, public export, automated tests, and notebook section all support repeating structures with connectivity. No unrelated source feature is introd…
Full details: Docstring Coverage

Explanation

Docstring coverage is 3.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 30 functions across 3 files. (1 skipped: 1 unsupported.)

  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

PythonFZ and others added 3 commits October 2, 2026 13:53
molify.repeat(atoms, rep) delegates the geometry to ase.Atoms.repeat and
rebuilds info['connectivity'] and info['original_index'] for the repeated
structure. Each bond links atom i to the nearest periodic image of atom j,
so bonds across the cell boundary connect neighbouring copies; bond orders
are kept. Image shifts come from ase.geometry.find_mic, which covers
triclinic cells and partial periodicity.

Invalid rep values, an original_index of the wrong length and bond indices
outside the structure raise ValueError. The packmol docs gain a
"Repeating a Box" section.

Closes #129

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- raise ValueError for self-bonds and for bonds at least half the smallest
  periodic cell height long, such as the Si primitive cell
- store bond orders as Python float, also after an extxyz round trip
- remove info['smiles'] from the repeated structure, as pack does
- validate rep with np.broadcast_to and read bonds inside _tile_bonds
- check each tiled bond order against its source bond, run the triclinic
  test for rep 1, 2, (2, 1, 1), (1, 2, 3), and cover empty connectivity

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- check each bond against half the shortest vector of the Minkowski-reduced
  periodic lattice, so sheared bases of ordinary cells repeat
- treat pbc along a zero-length cell vector as non-periodic
- accept rep as one integer or exactly three integers
- tile connectivity after atoms.repeat
- test sheared cells, distance-based connectivity with None bond orders,
  Python int original_index, the plain ase result without connectivity,
  and the public export

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@PythonFZ
PythonFZ force-pushed the feat/repeat-connectivity branch from 70e2b87 to e24d00e Compare October 2, 2026 12:54
@PythonFZ

PythonFZ commented Oct 2, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@PythonFZ
PythonFZ merged commit e06c39c into main Oct 2, 2026
22 checks passed
@PythonFZ
PythonFZ deleted the feat/repeat-connectivity branch October 2, 2026 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort-2 Effort: moderate enhancement New feature or request p-2 Priority: normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ase repeat on periodic boxed should expand the connectivity

1 participant