feat(lammps): generate masses for lammps/lmp export#963
feat(lammps): generate masses for lammps/lmp export#963njzjz-bot wants to merge 3 commits intodeepmodeling:masterfrom
Conversation
Prefer explicitly stored masses when available, and otherwise infer per-type masses from atom_names when all names are valid element symbols. Keep the previous behavior for unknown type names so exports do not emit unsafe Masses sections. Add regression tests covering both known-element and unknown-type cases, and make the new tests independent of the current working directory. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
dosubot
bot
added
size:M
Merging this PR will not alter performance
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #963 +/- ##
==========================================
+ Coverage 86.65% 86.70% +0.04%
==========================================
Files 86 86
Lines 8019 8039 +20
==========================================
+ Hits 6949 6970 +21
+ Misses 1070 1069 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Run repository pre-commit hooks and keep the ruff import ordering fix. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdd LAMMPS mass generation: new helper Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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: 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/lammps/lmp.py`:
- Around line 553-554: The loop that pairs masses and atom names uses
zip(masses, system["atom_names"]) which can silently truncate mismatched
lengths; update the zip call to zip(masses, system["atom_names"], strict=True)
in the code that builds ret (the loop using enumerate over (mass, atom_name) and
mass_fmt) so Python will raise on length mismatches and avoid silent bugs.
- Around line 508-509: The current logic slices a provided masses list to
atom_names length which can silently mis-map types; in the function handling
system["masses"] (the variables masses and atom_names in lmp.py) change the
check to require exact length equality (len(masses) == len(atom_names)) and
otherwise return None or raise a clear ValueError instead of truncating; update
the branch that currently does "if masses is not None and len(masses) >=
len(atom_names): return np.asarray(masses[: len(atom_names)], ...)" to only
accept exact matches and ensure any downstream code that emits Masses handles
the None/error case.
🪄 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: 35a43b4e-3052-494f-a9a9-070032699387
📒 Files selected for processing (2)
dpdata/lammps/lmp.pytests/test_lammps_lmp_dump.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
dpdata/lammps/lmp.py (2)
506-509:⚠️ Potential issue | 🟠 MajorRequire exact length match for explicit masses to avoid silent mis-mapping.
The current logic
len(masses) >= len(atom_names)with slicing can silently truncate when more masses than types are provided, potentially assigning wrong masses to types. This was flagged in a previous review and should require exact equality.Proposed fix
atom_names = system["atom_names"] masses = system.get("masses") - if masses is not None and len(masses) >= len(atom_names): - return np.asarray(masses[: len(atom_names)], dtype=float) + if masses is not None: + masses = np.asarray(masses, dtype=float) + if masses.ndim == 1 and len(masses) == len(atom_names): + return masses + # Mismatched shape/length: fall through to element inference or return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dpdata/lammps/lmp.py` around lines 506 - 509, The code currently allows more masses than atom types and silently truncates via slicing; update the check in the block that reads atom_names and masses (variables atom_names and masses in this module/function) to require exact length equality (use len(masses) == len(atom_names)); if they differ, raise a clear ValueError (or similar) indicating the mismatch between provided masses and atom_names instead of slicing, otherwise return the masses converted to float with np.asarray(masses, dtype=float).
553-554:⚠️ Potential issue | 🟡 MinorAdd
strict=Truetozip()to satisfy Ruff B905 and guard against length mismatches.The
zip()call is missing thestrict=parameter, which is flagged by Ruff (B905). Addingstrict=Trueensures a runtime error ifmassesandatom_nameshave mismatched lengths.Proposed fix
- for ii, (mass, atom_name) in enumerate(zip(masses, system["atom_names"])): + for ii, (mass, atom_name) in enumerate( + zip(masses, system["atom_names"], strict=True) + ):As per coding guidelines: "Run ruff linting with
ruff check dpdata/".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dpdata/lammps/lmp.py` around lines 553 - 554, Update the zip call in the loop that builds `ret` so it uses strict=True to enforce equal lengths: change the `for ii, (mass, atom_name) in enumerate(zip(masses, system["atom_names"])):` loop in `dpdata/lammps/lmp.py` to pass strict=True to zip (i.e., enumerate(zip(masses, system["atom_names"], strict=True))) so mismatched lengths raise at runtime; ensure no other logic relies on silent truncation of `masses` vs `system["atom_names"]`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@dpdata/lammps/lmp.py`:
- Around line 506-509: The code currently allows more masses than atom types and
silently truncates via slicing; update the check in the block that reads
atom_names and masses (variables atom_names and masses in this module/function)
to require exact length equality (use len(masses) == len(atom_names)); if they
differ, raise a clear ValueError (or similar) indicating the mismatch between
provided masses and atom_names instead of slicing, otherwise return the masses
converted to float with np.asarray(masses, dtype=float).
- Around line 553-554: Update the zip call in the loop that builds `ret` so it
uses strict=True to enforce equal lengths: change the `for ii, (mass, atom_name)
in enumerate(zip(masses, system["atom_names"])):` loop in `dpdata/lammps/lmp.py`
to pass strict=True to zip (i.e., enumerate(zip(masses, system["atom_names"],
strict=True))) so mismatched lengths raise at runtime; ensure no other logic
relies on silent truncation of `masses` vs `system["atom_names"]`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6eb2e3bf-b985-4573-ad43-3d7dbed897af
📒 Files selected for processing (1)
dpdata/lammps/lmp.py
Reject explicit system["masses"] arrays unless they match atom_names exactly to avoid silently truncating and mis-mapping LAMMPS Masses entries. Add a regression test covering the mismatched-length case. Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
2 participants
Problem
lammps/lmpexport currently does not emit aMassessection.atom_namesare known element symbols, the masses can be generated safely.Change
massesdata when presentatom_nameswhen all names are valid element symbolsCloses #960
Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.4)
Summary by CodeRabbit
New Features
Tests