feat: support nonorthogonal AMBER cells#971
feat: support nonorthogonal AMBER cells#971njzjz-bot wants to merge 2 commits intodeepmodeling:masterfrom
Conversation
Authored by OpenClaw (model: gpt-5.5)
Merging this PR will not alter performance
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #971 +/- ##
==========================================
+ Coverage 86.73% 86.76% +0.03%
==========================================
Files 86 86
Lines 8056 8083 +27
==========================================
+ Hits 6987 7013 +26
- Misses 1069 1070 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds support for non-orthogonal cells in AMBER MD format by introducing a ChangesNon-Orthogonal Cell Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_amber_md.py (1)
56-56: 💤 Low valueConsider adding
strict=Truetozip()(Ruff B905).- for frame, lengths, angles in zip(cells, cell_lengths, cell_angles): + for frame, lengths, angles in zip(cells, cell_lengths, cell_angles, strict=True):The three iterables are guaranteed to have the same first dimension here, so this is purely defensive.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_amber_md.py` at line 56, Update the loop that iterates over the three iterables by calling zip with strict=True to ensure the lengths match; replace the current zip(cells, cell_lengths, cell_angles) usage in the test (the loop binding frame, lengths, angles) with zip(cells, cell_lengths, cell_angles, strict=True) so a mismatch raises immediately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dpdata/amber/md.py`:
- Line 67: Replace the tuple-concatenation used to build shape from
cell_lengths.shape with tuple unpacking: construct shape by unpacking
cell_lengths.shape[:-1] and then appending the two 3s (update the assignment to
the variable shape in dpdata/amber/md.py that currently reads
cell_lengths.shape[:-1] + (3, 3)); this removes the concatenation that triggers
RUF005 and satisfies ruff linting.
---
Nitpick comments:
In `@tests/test_amber_md.py`:
- Line 56: Update the loop that iterates over the three iterables by calling zip
with strict=True to ensure the lengths match; replace the current zip(cells,
cell_lengths, cell_angles) usage in the test (the loop binding frame, lengths,
angles) with zip(cells, cell_lengths, cell_angles, strict=True) so a mismatch
raises immediately.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a10e149e-00d3-4f74-b0e6-e088a1da062b
📒 Files selected for processing (2)
dpdata/amber/md.pytests/test_amber_md.py
There was a problem hiding this comment.
Pull request overview
This PR extends the amber/md reader to support non-orthogonal (triclinic/monoclinic/hexagonal) AMBER NetCDF unit cells by converting AMBER’s (a,b,c) + (α,β,γ) metadata into dpdata’s 3×3 lower-triangular cell matrix representation, and adds tests covering valid/invalid angle cases and a regression read of a modified NetCDF fixture.
Changes:
- Added
cell_lengths_angles_to_cell()to convert AMBER cell lengths/angles into a 3×3 cell tensor. - Updated
read_amber_traj()to use the new conversion instead of rejecting non-90° angles. - Added unit tests for non-orthogonal conversion and for invalid/degenerate angle handling, plus a regression test that reads a modified AMBER NetCDF fixture.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
dpdata/amber/md.py |
Adds the lengths/angles → 3×3 cell conversion helper and uses it in AMBER trajectory reading. |
tests/test_amber_md.py |
Adds tests for the new conversion helper and for reading a trajectory with forced non-orthogonal cell angles. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Authored by OpenClaw (model: gpt-5.5)
Support AMBER MD trajectories with non-orthogonal unit cells.
This replaces the orthogonal-cell-only check in
read_amber_traj()with a length/angle to 3x3 cell conversion, while still rejecting invalid or degenerate AMBER cell angle metadata instead of silently returning collapsed cells.Changes:
cell_lengths_angles_to_cell()for AMBER cell length/angle conversion.read_amber_traj()so triclinic/monoclinic/hexagonal cells are supported.dpdata.LabeledSystem(..., fmt="amber/md")regression test using a real AMBER NetCDF fixture with non-orthogonal angles.Fixes #869.
Authored by OpenClaw (model: gpt-5.5)
Summary by CodeRabbit