feat(extxyz): support stress header, improve robustness and ASE compatibility#983
feat(extxyz): support stress header, improve robustness and ASE compatibility#983SchrodingersCattt wants to merge 4 commits into
Conversation
…tibility - Add stress= header → virial conversion (virial = stress_sign * V * stress), supporting both 3×3 and Voigt (6-component) formats. Default stress_sign=-1 follows ASE convention. User can pass stress_sign=1 for opposite convention. Fixes deepmodeling#973. - Thread **kwargs from MultiSystems.from_file() through to the parser so that stress_sign (and future options) reach handle_single_xyz_frame(). - Tolerate unknown per-atom properties (magmom, charges, tags, etc.) with a warning instead of crashing with RuntimeError. - Recognize both 'force' and 'forces' as per-atom property keys. Writer now outputs 'forces' for ASE SinglePointCalculator compatibility (addresses qchempku2017's comment on deepmodeling#973). - Support flexible energy key lookup: try energy, Energy, free_energy, REF_energy, energies in order. - Handle missing Lattice gracefully (nopbc=True + 100Å dummy box) and parse pbc="F F F" / pbc="T T T" header field. - Writer now emits pbc field and stress field (stress = -virial/volume) alongside virial for better ASE interoperability. - Fix from_labeled_system to actually read files when called directly (previously was a broken passthrough causing TypeError). - Add comprehensive tests for all new functionality.
dosubot
Bot
added
size:L
Merging this PR will not alter performance
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #983 +/- ##
==========================================
+ Coverage 86.68% 86.75% +0.07%
==========================================
Files 89 89
Lines 8116 8174 +58
==========================================
+ Hits 7035 7091 +56
- Misses 1081 1083 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Warning Review limit reached
More reviews will be available in 44 minutes and 8 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe QUIP/GAP XYZ parser gains stress→virial conversion ( Changesextxyz Parser and Formatter Enhancements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_quip_gap_xyz.py (2)
159-159: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueReplace multiplication sign
×with letterxin docstring.Line 159 docstring uses the Unicode multiplication sign
×(U+00D7) instead of the Latin letterx. This triggers Ruff's RUF002 warning for ambiguous characters in source code.🔧 Proposed fix
- """Voigt stress=[0.01,0.02,0.03,0.004,0.005,0.006] → 3×3 → virial=-V*stress.""" + """Voigt stress=[0.01,0.02,0.03,0.004,0.005,0.006] → 3x3 → virial=-V*stress."""🤖 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_quip_gap_xyz.py` at line 159, The docstring on line 159 contains a Unicode multiplication sign character `×` (U+00D7) instead of a regular ASCII letter `x`, which triggers a Ruff RUF002 warning for ambiguous characters. Replace the Unicode multiplication sign `×` with the regular letter `x` in the docstring text where it appears in the expression "3×3" to resolve the ambiguous character warning.Source: Linters/SAST tools
122-145: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueAddress Ruff style warnings: prefer
next(iter(...))over list slicing for single-element extraction.Lines 130, 153, 174, 192, 202, 212, 224, 244, 255, 274, 289, 304, 314, 331, 336 all use
list(...)[0]to extract the single system fromMultiSystems.systems.values(). Ruff recommendsnext(iter(...))for clarity and consistency.🔧 Suggested refactor
Replace each instance of
list(self.ms.systems.values())[0]with:next(iter(self.ms.systems.values()))Example for line 130:
- self.system = list(self.ms.systems.values())[0] + self.system = next(iter(self.ms.systems.values()))🤖 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_quip_gap_xyz.py` around lines 122 - 145, The setUp method in the TestStressToVirial class uses list slicing `list(self.ms.systems.values())[0]` to extract a single element from the systems dictionary values, which violates Ruff style conventions. Replace this pattern with `next(iter(self.ms.systems.values()))` to extract the first element more idiomatically. This same pattern should be fixed wherever it appears in the test file to ensure consistent style compliance with Ruff linting standards.Source: Linters/SAST tools
🤖 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/formats/xyz/quip_gap_xyz.py`:
- Line 24: Replace unicode multiplication sign (×) characters with ASCII
lowercase x in the docstring comments throughout the file to satisfy Ruff
linting rules. Locate all occurrences of the unicode × character in docstrings
and comments (including the one in "3×3 matrix" and other similar patterns) and
replace them with the ASCII character x to resolve RUF001/RUF002 lint warnings
across lines 24, 27, and 48.
---
Nitpick comments:
In `@tests/test_quip_gap_xyz.py`:
- Line 159: The docstring on line 159 contains a Unicode multiplication sign
character `×` (U+00D7) instead of a regular ASCII letter `x`, which triggers a
Ruff RUF002 warning for ambiguous characters. Replace the Unicode multiplication
sign `×` with the regular letter `x` in the docstring text where it appears in
the expression "3×3" to resolve the ambiguous character warning.
- Around line 122-145: The setUp method in the TestStressToVirial class uses
list slicing `list(self.ms.systems.values())[0]` to extract a single element
from the systems dictionary values, which violates Ruff style conventions.
Replace this pattern with `next(iter(self.ms.systems.values()))` to extract the
first element more idiomatically. This same pattern should be fixed wherever it
appears in the test file to ensure consistent style compliance with Ruff linting
standards.
🪄 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: 3b34f0ae-a3a2-4ebd-b311-e6d46a3eb0c9
📒 Files selected for processing (10)
dpdata/formats/xyz/quip_gap_xyz.pydpdata/plugins/xyz.pytests/test_quip_gap_xyz.pytests/xyz/energy_key_variants.xyztests/xyz/forces_key.xyztests/xyz/nopbc.xyztests/xyz/pbc_false.xyztests/xyz/stress_only.xyztests/xyz/stress_voigt.xyztests/xyz/unknown_props.xyz
…KEYS/_STRESS_KEYS constants Recognize both singular and plural forms for virial and stress header keys, consistent with _ENERGY_KEYS and _FORCE_KEYS patterns.
1 participant
Summary
Enhance the native extxyz reader/writer (
quip_gap_xyz.py) to fix silent virial data loss and improve ASE interoperability.Fixes #973
Changes
Reader (
handle_single_xyz_frame)stress=header whenvirial=is absent. Supports both 3×3 (9 values) and Voigt (6 values). Conversion:virial = stress_sign * V * stress(eV/ų assumed).stress_signparameter-1(ASE convention). User can passstress_sign=1for opposite convention viadpdata.MultiSystems.from_file(..., stress_sign=1).magmom,charges,tags,local_energy, etc. now emit a warning instead of crashing withRuntimeError.forceskeyforceandforcesas per-atom property keys (ASE usesforces).energy,Energy,free_energy,REF_energy,energiesin order.Latticegracefully (nopbc=True+ 100Å dummy box). Parsepbc="F F F"header.Writer (
format_single_frame)forcesinstead offorceSinglePointCalculatorrequires keyforces(addresses @qchempku2017 comment).stressfieldstress = -virial / volumewritten alongsidevirialfor ASE interop.pbcfieldpbc="T T T"orpbc="F F F"based on system data.Plugin (
QuipGapXYZFormat)from_labeled_systemTypeError. Now properly reads files when called directly.**kwargsfrom_multi_systems→QuipGapxyzSystems→handle_single_xyz_frameso user options (e.g.stress_sign) reach the parser.Tests
22 new tests added to
tests/test_quip_gap_xyz.pycovering:stress_signparameterforceskey, energy key variantspbc="F F F"headerforces/stress/pbcoutputLabeledSystemdirect file readingAll 127 tests in
test_quip_gap_xyz.pypass. All 138 tests in related files (test_xyz.py,test_quip_gap_xyz_to_methods.py) pass.Sign Convention
The stress→virial conversion follows the ASE convention by default:
virial = -V * stresswhere V is the cell volume and stress is the stress tensor in eV/ų. This matches
dpdata/plugins/ase.pyline 110. Users can override withstress_sign=1for codes using the opposite convention.Summary by CodeRabbit
Release Notes
New Features
stressand/orvirialwhen available, and improvedLattice/pbc/nopbchandling.Bug Fixes
Tests