Skip to content

feat: support nonorthogonal AMBER cells#971

Open
njzjz-bot wants to merge 2 commits intodeepmodeling:masterfrom
njzjz-bot:fix/amber-nonorthogonal-cells
Open

feat: support nonorthogonal AMBER cells#971
njzjz-bot wants to merge 2 commits intodeepmodeling:masterfrom
njzjz-bot:fix/amber-nonorthogonal-cells

Conversation

@njzjz-bot
Copy link
Copy Markdown
Contributor

@njzjz-bot njzjz-bot commented May 5, 2026

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:

  • Add cell_lengths_angles_to_cell() for AMBER cell length/angle conversion.
  • Use it in read_amber_traj() so triclinic/monoclinic/hexagonal cells are supported.
  • Add helper tests for valid non-orthogonal cells and invalid/degenerate cell angles.
  • Add a public 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

  • New Features
    • Extended AMBER trajectory support to handle non-orthogonal unit cells. Previously limited to near-orthogonal geometries, the system now correctly converts arbitrary cell lengths and angles into proper 3D cell vectors, enabling compatibility with a wider range of molecular dynamics simulations.

Authored by OpenClaw (model: gpt-5.5)
@dosubot dosubot Bot added size:M This PR changes 30-99 lines, ignoring generated files. dpdata enhancement New feature or request labels May 5, 2026
@njzjz njzjz requested a review from Copilot May 5, 2026 16:27
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 5, 2026

Merging this PR will not alter performance

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

✅ 2 untouched benchmarks


Comparing njzjz-bot:fix/amber-nonorthogonal-cells (0163ace) with master (38e8763)

Open in CodSpeed

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 93.93939% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.76%. Comparing base (38e8763) to head (0163ace).

Files with missing lines Patch % Lines
dpdata/amber/md.py 93.93% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 5, 2026

Warning

Rate limit exceeded

@njzjz-bot has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 46 minutes and 8 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e66dceb1-173e-42b0-a263-5852cf72da7a

📥 Commits

Reviewing files that changed from the base of the PR and between ae2755a and 0163ace.

📒 Files selected for processing (2)
  • dpdata/amber/md.py
  • tests/test_amber_md.py
📝 Walkthrough

Walkthrough

This PR adds support for non-orthogonal cells in AMBER MD format by introducing a cell_lengths_angles_to_cell utility function that converts cell lengths and angles (in degrees) to 3×3 cell vectors with trigonometric computation and validation, and integrates it into the existing read_amber_traj function to replace the prior 90°-angle-only assumption.

Changes

Non-Orthogonal Cell Support

Layer / File(s) Summary
Conversion Utility
dpdata/amber/md.py (lines 21–75)
New cell_lengths_angles_to_cell function converts AMBER cell lengths and angles (degrees) to 3×3 cell matrices using trigonometric relations; validates for near-zero sin_gamma and non-positive z_factor and raises RuntimeError on invalid geometry.
Integration
dpdata/amber/md.py (line 145)
read_amber_traj replaces hard-coded 90°-angle assumption with call to cell_lengths_angles_to_cell, enabling general non-orthogonal cell handling.
Tests & Validation
tests/test_amber_md.py (imports, lines 37–111)
Test class TestAmberMDNonOrthogonal with three methods: validates conversion output shape and values, confirms RuntimeError on invalid angles, and verifies non-orthogonal cells are correctly read from AMBER NetCDF files into dpdata.LabeledSystem.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. 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 specifically describes the main change: adding support for non-orthogonal AMBER cells.
Linked Issues check ✅ Passed The PR fully addresses issue #869 by implementing non-orthogonal cell support for amber/md format with appropriate tests.
Out of Scope Changes check ✅ Passed All changes are directly aligned with supporting non-orthogonal AMBER cells; no out-of-scope modifications detected.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/test_amber_md.py (1)

56-56: 💤 Low value

Consider adding strict=True to zip() (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

📥 Commits

Reviewing files that changed from the base of the PR and between 38e8763 and ae2755a.

📒 Files selected for processing (2)
  • dpdata/amber/md.py
  • tests/test_amber_md.py

Comment thread dpdata/amber/md.py Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread dpdata/amber/md.py
Comment thread tests/test_amber_md.py Outdated
Comment thread tests/test_amber_md.py Outdated
Authored by OpenClaw (model: gpt-5.5)
@njzjz njzjz requested a review from wanghan-iapcm May 5, 2026 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dpdata enhancement New feature or request size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature Request] support non-orthogonal cells for amber/md

2 participants