fix(lammps): distinguish neutral charge and dipole columns#1020
fix(lammps): distinguish neutral charge and dipole columns#1020njzjz-bot wants to merge 1 commit into
Conversation
Improve automatic six-column detection for neutral decimal charges and restrict legacy spin extraction to atomic-style extension rows so dipole moments are not reported as spins. Add regressions for both cases; existing tests used non-neutral charges and therefore missed the ambiguity. Coding-Agent: Codex Codex-Version: codex-cli 0.144.4 Model: gpt-5.6-sol Reasoning-Effort: xhigh
dosubot
Bot
added
size:XS
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
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 |
Merging this PR will not alter performance
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1020 +/- ##
=======================================
Coverage 86.87% 86.88%
=======================================
Files 89 89
Lines 8266 8270 +4
=======================================
+ Hits 7181 7185 +4
Misses 1085 1085 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
wanghan-iapcm
left a comment
wanghan-iapcm
left a comment
There was a problem hiding this comment.
Both #995 fixes look correct -- the ./e neutral-charge detection inspects the right column, the dipole guard correctly suppresses spurious spins, and the two new tests genuinely fail on pre-fix code. One follow-up on the spin whitelist, see inline.
| def get_spins(lines: list[str], atom_style: str = "atomic") -> np.ndarray | None: | ||
| # Dipole/sphere/etc. styles use their extra columns for different physical | ||
| # quantities. Atomic and spin styles carry magnetic spin vectors. | ||
| if atom_style not in {"atomic", "spin"}: |
There was a problem hiding this comment.
"spin" here is unreachable: it isn't a key in ATOM_STYLE_COLUMNS, so detect_atom_style never returns it, and passing atom_style="spin" raises ValueError: Unsupported atom style: spin in _atom_info_style before get_spins runs. So only "atomic" actually enables spins, and the comment implies a "spin" style that doesn't exist.
Side effect: a genuine 9-column spin file not commented Atoms # atomic (e.g. # spin, or no comment) is auto-detected as dipole, so get_spins now returns None and its spins are silently dropped (they were returned before). dpdata's own round-trip output uses # atomic, so this only hits externally-produced spin files -- and those already read coordinates from the dipole column layout, so the ambiguity is pre-existing -- but the spin loss is new.
Consider either adding a real "spin" entry to ATOM_STYLE_COLUMNS (layout id type x y z spx spy spz sp) so the whitelist is honest and # spin files parse, or dropping "spin" from the set and comment to avoid implying unsupported behavior.
2 participants
Fixes #995.
Improve neutral decimal charge-style detection and prevent dipole fields from being reported as spins.
Tests:
cd tests && python -m unittest test_lammps_atom_styles test_lammps_spinWhy existing tests missed it: Existing charge tests used non-integral charges, and no dipole-style fixture checked the optional spin key.
Coding agent: Codex
Codex version: codex-cli 0.144.4
Model: gpt-5.6-sol
Reasoning effort: xhigh