Improve robustness of openmx/md cell parsing and support new output format#967
Improve robustness of openmx/md cell parsing and support new output format#967shigeandtomo wants to merge 10 commits intodeepmodeling:masterfrom
openmx/md cell parsing and support new output format#967Conversation
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. |
1 participant
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