feat: do not output spin info for lmp format #886
Conversation
📝 WalkthroughWalkthroughAdds an optional output_spins flag to the LAMMPS writer path; the plugin reads and forwards this flag, and the writer emits per-atom spin columns only when spins exist and output_spins=True. Default behavior (no spins) is unchanged. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Plugin as LAMMPSLmpFormat.to_system
participant Writer as lammps.lmp.from_system_data
participant File as "LAMMPS Data File"
User->>Plugin: to_system(data, file_name, frame_idx, output_spins)
Plugin->>Writer: from_system_data(system, f_idx, output_spins)
alt spins present AND output_spins == true
Writer->>File: write per-atom coords + spin vectors & magnitudes
else
Writer->>File: write per-atom coords (no spin columns)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
✨ Finishing Touches
🧪 Generate unit tests
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
dpdata/lammps/lmp.py (3)
517-518: Header/style mismatch when emitting extra columns.If output_spins is True you append 4 extra columns but still write "Atoms # atomic". That misleads both dpdata’s detector and LAMMPS. At minimum, drop the style comment when extra columns are present.
Apply:
- ret += "Atoms # atomic\n" + if output_spins: + # Avoid mismatched atom-style comment when extra per-atom columns are present + ret += "Atoms\n" + else: + ret += "Atoms # atomic\n"
532-546: Guard normalization with epsilon, not exact zero.Floating-point “zero” rarely hits exactly 0; tiny norms will blow up the direction.
Apply:
- if "spins" in system and output_spins: + if "spins" in system and output_spins: + EPS = 1e-12 coord_fmt = ( @@ - spins_norm = np.linalg.norm(system["spins"][f_idx], axis=1) + spins_norm = np.linalg.norm(system["spins"][f_idx], axis=1)And below (see next comment) change the equality check accordingly.
547-571: Zero-norm spin handling writes bogus direction (z + 1).For zero magnitude, emitting z+1 is wrong. Emit a canonical direction and magnitude 0.
Apply:
- if "spins" in system and output_spins: - if spins_norm[ii] != 0: + if "spins" in system and output_spins: + if spins_norm[ii] > EPS: ret += coord_fmt % ( @@ - else: - ret += coord_fmt % ( - ii + 1, - system["atom_types"][ii] + 1, - coord[ii][0] - system["orig"][0], - coord[ii][1] - system["orig"][1], - coord[ii][2] - system["orig"][2], - system["spins"][f_idx][ii][0], - system["spins"][f_idx][ii][1], - system["spins"][f_idx][ii][2] + 1, - spins_norm[ii], - ) # noqa: UP031 + else: + # Zero magnitude: canonical direction (0,0,1) and magnitude 0 + ret += coord_fmt % ( + ii + 1, + system["atom_types"][ii] + 1, + coord[ii][0] - system["orig"][0], + coord[ii][1] - system["orig"][1], + coord[ii][2] - system["orig"][2], + 0.0, + 0.0, + 1.0, + 0.0, + ) # noqa: UP031dpdata/plugins/lammps.py (1)
111-133: Remove unused**kwargs, replaceassertwith proper error handling, and fix implicitOptionalhints
- Drop the unused
**kwargsparameters from bothto_system(dpdata/plugins/lammps.py:111) andfrom_system(line 45), or explicitly consume them.- Replace
assert frame_idx < len(data["coords"])with an explicit check raising aValueError(or similar) to avoid disabled assertions in optimized runs.- Change implicit
Optionaltype hints to useOptional[T](e.g.type_map: Optional[list[str]] = None,input_file: Optional[str] = None).
🧹 Nitpick comments (2)
dpdata/lammps/lmp.py (1)
487-580: Alternative to the boolean flag: drive columns by atom_style.More idiomatic: keep spins suppressed for lmp by default; when users need spins, require atom_style="dipole" (and write mux muy muz only; no extra “magnitude” column). This avoids a new parameter and aligns with LAMMPS.
I can draft a small writer shim that switches header and columns based on atom_style and drops the non-standard magnitude column. Want that?
dpdata/plugins/lammps.py (1)
116-129: Docstring: move output_spins under kwargs.Apply:
frame_idx : int The index of the frame to dump - output_spins : bool, optional - Whether to output spin information columns. Default is False. - When True, outputs additional columns for spin direction and magnitude. **kwargs : dict - other parameters + Optional writer options. Supported: + - output_spins (bool, default False): include spin columns + other parameters
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
dpdata/lammps/lmp.py(3 hunks)dpdata/plugins/lammps.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
dpdata/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ruff to lint and format all Python code under dpdata/ before committing
Files:
dpdata/plugins/lammps.pydpdata/lammps/lmp.py
dpdata/*/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When adding a new format, implement it as a new submodule under dpdata//
Files:
dpdata/plugins/lammps.pydpdata/lammps/lmp.py
🧬 Code graph analysis (2)
dpdata/plugins/lammps.py (1)
dpdata/lammps/lmp.py (1)
from_system_data(487-580)
dpdata/lammps/lmp.py (3)
dpdata/pwmat/atomconfig.py (1)
from_system_data(54-95)dpdata/vasp/poscar.py (1)
from_system_data(77-134)dpdata/gromacs/gro.py (1)
from_system_data(95-112)
🪛 Ruff (0.12.2)
dpdata/plugins/lammps.py
112-112: Unused method argument: kwargs
(ARG002)
130-130: Use of assert detected
(S101)
|
Hi, @OutisLi, please fix the unittest. |
|
please check the suggestions by coderabbitai |
OutisLi
force-pushed
the
fix/stru_mag
branch
from
59867cc to
0ba29c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
dpdata/plugins/lammps.py (2)
111-113: Don’t add a public param; consumeoutput_spinsvia kwargs.Keep the public signature stable and meet the PR ask. Also prevents ARG002 for unused kwargs once you pop it below.
- def to_system( - self, data, file_name: FileType, frame_idx=0, output_spins=False, **kwargs - ): + def to_system( + self, data, file_name: FileType, frame_idx=0, **kwargs + ):
130-130: Replace assert with a runtime bounds check (Ruff S101).Avoid asserts in production path; raise a clear exception instead.
- assert frame_idx < len(data["coords"]) + if not (0 <= frame_idx < len(data["coords"])): + raise IndexError( + f"frame_idx out of range: {frame_idx}; nframes={len(data['coords'])}" + )
🧹 Nitpick comments (1)
dpdata/plugins/lammps.py (1)
124-126: Docstring: markoutput_spinsas a kwarg, not a formal parameter.Reflects the API choice above without exposing a new arg.
- output_spins : bool, optional - Whether to output spin information columns. Default is False. - When True, outputs additional columns for spin direction and magnitude. + output_spins (kwarg) : bool, optional + Whether to output spin information columns. Default is False. + When True, outputs additional columns for spin direction and magnitude.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdata/lammps/lmp.py(3 hunks)dpdata/plugins/lammps.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- dpdata/lammps/lmp.py
🧰 Additional context used
📓 Path-based instructions (2)
dpdata/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use ruff to lint and format all Python code under dpdata/ before committing
Files:
dpdata/plugins/lammps.py
dpdata/*/**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
When adding a new format, implement it as a new submodule under dpdata//
Files:
dpdata/plugins/lammps.py
🧬 Code graph analysis (1)
dpdata/plugins/lammps.py (1)
dpdata/lammps/lmp.py (1)
from_system_data(487-580)
🪛 Ruff (0.12.2)
dpdata/plugins/lammps.py
112-112: Unused method argument: kwargs
(ARG002)
130-130: Use of assert detected
(S101)
CodSpeed WallTime Performance ReportMerging #886 will not alter performanceComparing Summary
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
OutisLi
force-pushed
the
fix/stru_mag
branch
from
80ef876 to
5589f80
Compare
| @@ -48,8 +48,8 @@ def setUp(self): | |||
| def tearDown(self): | |||
| pass # if os.path.isfile(self.lmp_coord_name):os.remove(self.lmp_coord_name) | |||
|
|
|||
| def test_dump_input(self): | |||
| self.tmp_system.to("lammps/lmp", self.lmp_coord_name) | |||
| def test_dump_input(self, output_spins=False): | |||
There was a problem hiding this comment.
Do not add any parameter options to the test function.
| @@ -58,9 +58,9 @@ def test_dump_input(self): | |||
| 2 2 1.2621856000 0.7018028000 0.5513885000 0.0000000000 0.8000000000 0.6000000000 5.0000000000""" | |||
| self.assertTrue(coord_ref in c) | |||
|
|
|||
| def test_dump_input_zero_spin(self): | |||
| def test_dump_input_zero_spin(self, output_spins=True): | |||
There was a problem hiding this comment.
Do not add any parameter options to the test function.
| def test_dump_input(self): | ||
| self.tmp_system.to("lammps/lmp", self.lmp_coord_name) | ||
| def test_dump_input(self, output_spins=False): | ||
| self.tmp_system.to("lammps/lmp", self.lmp_coord_name, output_spins=output_spins) |
There was a problem hiding this comment.
For the lammps spin tests ,please set output_spins to True.
fix issue #885 in dpdata and issue #312 in dpgen2
However, adding an additional parameter is not the best method. Please provide another way to fix this issue, thanks.
Summary by CodeRabbit
New Features
Tests