Improve robustness of openmx/md cell parsing and support new output format#967
Conversation
dosubot
Bot
added
size:M
Merging this PR will not alter performance
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 21 seconds. ⌛ 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 (1)
📝 WalkthroughWalkthroughRefactored OpenMX parsing: simplified atom-species handling, reworked cell parsing to extract nine floats after "Cell_Vectors=", added SCF convergence detection via warnings, removed redundant SCF checks in coordinate/force loaders, replaced Methane2-based test with new Au111Surface test artifacts and test module. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (1)
tests/test_openmx_multiple_formats.py (1)
49-56: Assert the expected non-convergence warning.The new fixture intentionally includes
scf_conv=0; wrapping construction withassertWarnsRegexboth verifies the new behavior and avoids unasserted warning noise.Proposed test update
class TestOPENMXTraj(unittest.TestCase, TestOPENMXTRAJProps): def setUp(self): - self.system = dpdata.System("openmx/Au111Surface", fmt="openmx/md") + with self.assertWarnsRegex(UserWarning, "SCF not converged"): + self.system = dpdata.System("openmx/Au111Surface", fmt="openmx/md") class TestOPENMXLabeledTraj(unittest.TestCase, TestOPENMXTRAJProps): def setUp(self): - self.system = dpdata.LabeledSystem("openmx/Au111Surface", fmt="openmx/md") + with self.assertWarnsRegex(UserWarning, "SCF not converged"): + self.system = dpdata.LabeledSystem("openmx/Au111Surface", fmt="openmx/md")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_openmx_multiple_formats.py` around lines 49 - 56, The tests create systems in TestOPENMXTraj.setUp and TestOPENMXLabeledTraj.setUp using dpdata.System and dpdata.LabeledSystem but do not assert the new non-convergence warning produced by the fixture with scf_conv=0; update both setUp methods to wrap the construction in self.assertWarnsRegex(WarningClassOrBase, r"non-converge|non[- ]converg", msg=None) (use the actual warning class emitted by the OpenMX loader if available, otherwise Warning) so the tests explicitly expect and match the non-convergence warning and suppress unasserted warning noise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dpdata/openmx/omx.py`:
- Around line 77-85: The SCF warning uses an unnecessary f-string and omits the
required stacklevel when calling warnings.warn; inside the loop that detects
tokens starting with "scf_conv=" (the code that sets scf_conv and checks if
scf_conv == 0) replace warnings.warn(f"SCF not converged!") with
warnings.warn("SCF not converged!", stacklevel=2) (or an appropriate stacklevel)
so the message is a plain string and includes the stacklevel argument.
In `@tests/test_openmx_multiple_formats.py`:
- Around line 19-32: The test_cell assertion uses the wrong indexing for the
expected array: it compares self.system["cells"][ff][ii][jj] against
cells[ii][jj] which ignores the frame axis and can index out of bounds; update
the expected access to include the frame dimension (use cells[ff][ii][jj]) so
the test compares corresponding frames and scalar elements; modify the assertion
inside test_cell accordingly (referencing test_cell, self.system["cells"], and
the local variable cells).
---
Nitpick comments:
In `@tests/test_openmx_multiple_formats.py`:
- Around line 49-56: The tests create systems in TestOPENMXTraj.setUp and
TestOPENMXLabeledTraj.setUp using dpdata.System and dpdata.LabeledSystem but do
not assert the new non-convergence warning produced by the fixture with
scf_conv=0; update both setUp methods to wrap the construction in
self.assertWarnsRegex(WarningClassOrBase, r"non-converge|non[- ]converg",
msg=None) (use the actual warning class emitted by the OpenMX loader if
available, otherwise Warning) so the tests explicitly expect and match the
non-convergence warning and suppress unasserted warning noise.
🪄 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: 9dabf793-73ed-46d2-a0ce-01eb96e8ee39
⛔ Files ignored due to path filters (2)
tests/openmx/Au111Surface.datis excluded by!**/*.dattests/openmx/Methane2.datis excluded by!**/*.dat
📒 Files selected for processing (5)
dpdata/openmx/omx.pytests/openmx/Au111Surface.mdtests/openmx/Methane2.mdtests/test_openmx_check_convergence.pytests/test_openmx_multiple_formats.py
💤 Files with no reviewable changes (2)
- tests/openmx/Methane2.md
- tests/test_openmx_check_convergence.py
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #967 +/- ##
=======================================
Coverage 86.70% 86.70%
=======================================
Files 86 86
Lines 8039 8034 -5
=======================================
- Hits 6970 6966 -4
+ Misses 1069 1068 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dpdata/openmx/omx.py`:
- Around line 75-80: The convergence check was moved to look for "scf_conv=" on
the Cell_Vectors line but omitted the legacy per-coordinate flag in load_coords;
update load_coords() to first look for the new "scf_conv=" token (as currently
done) and if not found, also check parts[14] on coordinate lines for the legacy
convergence flag and issue warnings.warn("SCF not converged!", stacklevel=2)
when that legacy flag indicates non-convergence; also add unit tests that feed
both legacy-format input (with parts[14] non-zero) and new-format input (with
scf_conv=0 token) and assert that a warning is raised in each case to cover both
formats.
In `@tests/test_openmx_multiple_formats.py`:
- Around line 51-57: The setup methods for TestOPENMXTRAJProps.setUp and
TestOPENMXLabeledTraj.setUp call dpdata.System and dpdata.LabeledSystem which
emit a UserWarning for a non-converged SCF; wrap those constructor calls in a
warnings.catch_warnings() context and use warnings.simplefilter("ignore",
UserWarning) (or record and assert if you prefer an explicit check) so the
warning is captured/suppressed during setup rather than leaking outside tests.
🪄 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: ee5affe4-ea88-489a-afdf-acf06e9c8293
📒 Files selected for processing (2)
dpdata/openmx/omx.pytests/test_openmx_multiple_formats.py
|
Dear Jinzhe Zeng, This PR introduces support for the upcoming OpenMX version 4.0 output format. Since the new version is expected to be released soon, it would be very helpful if this PR could be reviewed and merged when possible. Thank you very much for your time and consideration. |
There was a problem hiding this comment.
Pull request overview
This PR updates the OpenMX openmx/md reader to better handle multiple .md output variants by making Cell_Vectors parsing less dependent on fixed token positions, and adds support for the newer scf_conv flag emitted on the MD header line. It also refreshes the OpenMX test fixtures by replacing the removed Methane2 snapshots with an Au111Surface snapshot that exercises the new output format.
Changes:
- Make
Cell_Vectors=parsing indpdata/openmx/omx.pyextract the 3×3 cell from the substring followingCell_Vectors=(instead of relying on total column counts). - Emit an SCF non-convergence warning when
scf_conv=0is present on the MD header line. - Update tests/fixtures: add
Au111Surfacesnapshot + new test module; removeMethane2fixtures and the old convergence-check test module.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
dpdata/openmx/omx.py |
Updates cell parsing and adds scf_conv warning behavior. |
tests/test_openmx_multiple_formats.py |
Adds coverage for parsing the new Au111Surface OpenMX MD output variant. |
tests/test_openmx_check_convergence.py |
Removes prior Methane2-based convergence fixture test module. |
tests/openmx/Au111Surface.md |
New .md fixture including scf_conv tokens. |
tests/openmx/Au111Surface.dat |
New .dat fixture paired with Au111Surface .md. |
tests/openmx/Methane2.md |
Removes old Methane2 .md snapshot. |
tests/openmx/Methane2.dat |
Removes old Methane2 .dat snapshot. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| part = line.split("Cell_Vectors=")[1] | ||
| parts = part.split() | ||
| values = list(map(float, parts[:9])) |
| # Checking SCF converged or not | ||
| for token in line.split(): | ||
| if token.startswith("scf_conv="): | ||
| scf_conv = int(token.split("=")[1]) | ||
| if scf_conv == 0: | ||
| warnings.warn("SCF not converged!", stacklevel=2) |
| # print(cells.shape) | ||
| # print(coords.shape) | ||
| # print(len(energy)) | ||
| # print(force.shape) |
| def setUp(self): | ||
| with warnings.catch_warnings(): | ||
| warnings.simplefilter("ignore", UserWarning) | ||
| self.system = dpdata.System("openmx/Au111Surface", fmt="openmx/md") | ||
|
|
njzjz-bot
left a comment
njzjz-bot
left a comment
There was a problem hiding this comment.
I reviewed the OpenMX parser changes and the updated fixtures/tests. The new Cell_Vectors= parsing is more robust across the existing OpenMX MD header variants, and limiting SCF convergence handling to the official scf_conv= token looks reasonable given the clarification that the previous per-coordinate flag was not part of the official format.
I also checked the current PR status and ran targeted local checks:
uv run --with ruff ruff check dpdata/openmx/omx.py tests/test_openmx_multiple_formats.pyuv run --with ruff ruff format --check dpdata/openmx/omx.py tests/test_openmx_multiple_formats.pycd tests && uv run python -W error -m unittest test_openmx_multiple_formats.py test_openmx.py
All passed. No blocking issues from my side.
Reviewed by OpenClaw 2026.4.22 (00bd2cf), model: custom-chat-jinzhezeng-group/gpt-5.5.
Dear developers,
This PR improves the robustness of the
openmx/mdparser to support multiple output file formats, including both newer and older versions.Main changes:
Cell_Vectorsparsing by removing dependency on fixed column positionsscf_convflag introduced in OpenMX version 4.0 (upcoming)Thank you for your consideration.
Summary by CodeRabbit
New Features
Improvements
Tests