Add optional dump_final_structure flag for MD runs#363
Conversation
When n_ionic_steps is not evenly divisible by n_print, LAMMPS's periodic dump command silently skips the final configuration even though log.lammps still reports that step (see notebooks/2026/2026-06-27-lammps-dump). With this flag enabled, a write_dump command is appended after the run command to append the missing final frame to dump.out. Off by default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds an optional final ChangesMD final dump capture
Sequence Diagram(s)sequenceDiagram
participant lammps_file_interface_function
participant LAMMPS
participant parse_lammps_output_files
lammps_file_interface_function->>LAMMPS: write lmp.in with periodic dump.out and optional write_dump all custom dump.out ... append yes
LAMMPS->>LAMMPS: run MD steps and emit log.lammps / dump.out
parse_lammps_output_files->>LAMMPS: read log.lammps and dump.out
LAMMPS-->>parse_lammps_output_files: generic["steps"] and generic["positions"]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
=======================================
Coverage 98.76% 98.77%
=======================================
Files 13 13
Lines 1214 1223 +9
=======================================
+ Hits 1199 1208 +9
Misses 15 15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lammpsparser/compatibility/file.py (1)
188-203: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHonor
input_control_fileoverrides in the appendedwrite_dump.
_modify_input_dict()can replace the regulardump/dump_modifylines, but this post-runwrite_dumpis still built from the hard-coded defaults. Withwrite_dump_if_missing=True, that can produce a final frame indump.outwith different columns or formatting than the periodic frames, which breaks the “same fields/format” contract and risks inconsistent parsing. Please derive the appended command from the effective post-override dump configuration, or rejectdump/dump_modifyoverrides when this flag is enabled.🤖 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 `@src/lammpsparser/compatibility/file.py` around lines 188 - 203, The appended write_dump command in the input assembly still uses hard-coded dump defaults, so it can ignore overrides applied by _modify_input_dict(). Update the logic around the lmp_str_lst construction to build the post-run write_dump from the effective dump/dump_modify configuration produced by input_control_file, or explicitly disallow dump-related overrides when write_dump_if_missing is enabled. Use the existing _modify_input_dict(), write_dump_if_missing, and dump_fields/dump_format handling to keep the final frame consistent with the periodic dumps.
🤖 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.
Outside diff comments:
In `@src/lammpsparser/compatibility/file.py`:
- Around line 188-203: The appended write_dump command in the input assembly
still uses hard-coded dump defaults, so it can ignore overrides applied by
_modify_input_dict(). Update the logic around the lmp_str_lst construction to
build the post-run write_dump from the effective dump/dump_modify configuration
produced by input_control_file, or explicitly disallow dump-related overrides
when write_dump_if_missing is enabled. Use the existing _modify_input_dict(),
write_dump_if_missing, and dump_fields/dump_format handling to keep the final
frame consistent with the periodic dumps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d6d95e57-2dda-457b-9cd3-b134c39dc102
⛔ Files ignored due to path filters (2)
tests/static/dump_missing_final_step/dump.outis excluded by!**/*.outtests/static/dump_with_final_step/dump.outis excluded by!**/*.out
📒 Files selected for processing (5)
src/lammpsparser/compatibility/file.pytests/static/dump_missing_final_step/log.lammpstests/static/dump_with_final_step/log.lammpstests/test_compatibility_file.pytests/test_output.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lammpsparser/compatibility/file.py (1)
192-200: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing divisibility guard —
n_printis read but never used, and the final frame is duplicated when the run is evenly divisible.Line 192 reads
n_printbut it is never referenced. Per the PR objective, thewrite_dumpshould only be appended when the final step would otherwise be missing, i.e. whenn_ionic_steps % n_print != 0; otherwise the periodicdumpalready captured the final step and appending here writes a duplicate final frame intodump.out. The downstream regression (tests/test_output.py::test_final_step_recovered_with_write_dump) expects dump frame count to equal log step count, which an unconditional append breaks for the divisible case.The docstring at Lines 89-90 should also be updated to describe this conditional behavior.
🐛 Proposed fix: only append when the final step is actually missing
lmp_str_lst += ["run {} ".format(n_ionic_steps)] n_print = calc_kwargs.get("n_print", 1) - if dump_final_structure: + if dump_final_structure and n_print and n_ionic_steps % n_print != 0: lmp_str_lst += [ "write_dump all custom dump.out " + dump_fields + " modify sort id format line " + dump_format + " append yes" ]🤖 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 `@src/lammpsparser/compatibility/file.py` around lines 192 - 200, The final-structure dump handling in file.py is unconditional even though `n_print` is already read, which can duplicate the last frame when the run length is evenly divisible. Update the logic around the `dump_final_structure` / `write_dump` path in the relevant compatibility function so it only appends the final dump when `n_ionic_steps % n_print != 0`, using `n_print` as the divisibility guard. Also update the associated docstring/comments for this behavior so the conditional final-frame recovery is documented clearly.
🤖 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 `@src/lammpsparser/compatibility/file.py`:
- Line 36: The parser compatibility API is inconsistent because the function
signature still exposes dump_final_structure while callers use
write_dump_if_missing, causing a TypeError. Update the relevant function
signature and any internal references in the file.py compatibility path to use
write_dump_if_missing consistently, and adjust the docstring to match the new
kwarg name so the public API and documentation align.
---
Outside diff comments:
In `@src/lammpsparser/compatibility/file.py`:
- Around line 192-200: The final-structure dump handling in file.py is
unconditional even though `n_print` is already read, which can duplicate the
last frame when the run length is evenly divisible. Update the logic around the
`dump_final_structure` / `write_dump` path in the relevant compatibility
function so it only appends the final dump when `n_ionic_steps % n_print != 0`,
using `n_print` as the divisibility guard. Also update the associated
docstring/comments for this behavior so the conditional final-frame recovery is
documented clearly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 762ac59f-b08f-4650-a851-f1f193d4c63e
📒 Files selected for processing (2)
src/lammpsparser/compatibility/file.pytests/test_compatibility_file.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_compatibility_file.py
| write_restart_file: bool = False, | ||
| read_restart_file: bool = False, | ||
| restart_file: str = "restart.out", | ||
| dump_final_structure: bool = False, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the kwarg name used by callers vs. the function signature.
rg -nP '\bwrite_dump_if_missing\b|\bdump_final_structure\b' -g '*.py'Repository: pyiron/lammpsparser
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## repo files"
git ls-files | sed -n '1,200p'
echo "## locate target file and test file"
fd -a 'file.py|test_compatibility_file.py' . || true
echo "## search for compatibility/file and relevant symbols"
rg -n --hidden --glob '!**/.git/**' 'compatibility/file.py|def .*dump|write_dump_if_missing|dump_final_structure|test_compatibility_file' .Repository: pyiron/lammpsparser
Length of output: 4622
Rename the kwarg to write_dump_if_missing The current signature exposes dump_final_structure, but the call sites pass write_dump_if_missing=..., which raises TypeError. Update the parameter name and matching docstring to keep the API consistent.
🤖 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 `@src/lammpsparser/compatibility/file.py` at line 36, The parser compatibility
API is inconsistent because the function signature still exposes
dump_final_structure while callers use write_dump_if_missing, causing a
TypeError. Update the relevant function signature and any internal references in
the file.py compatibility path to use write_dump_if_missing consistently, and
adjust the docstring to match the new kwarg name so the public API and
documentation align.
|
based on lammps/lammps#5005 |
jan-janssen
changed the title
|
@Gitdowski This is the most simple implementation to fix the dumping of the final structure. It still comes with a couple of challenges:
Can you check if this resolves your issue? |
|
@copilot Please add more unit tests to cover the missing lines. |
Added additional unit tests to cover the missing lines in the new
Addressed in commit |
|
In LAMMPS we could use something like : |
|
Thank you Jan! On a first glance this could do the thing
I am not sure if I understand your concern here. Every lammps simulation triggered by lammpsparser should only start one
I am also on your side here. For reference, here is the comment on "appending to files is always a bad idea": lammps/lammps#5005 (comment)
I will test it today and let you know! |
I thought when somebody does a minimization, writes a restart file and then from this restart file starts and MD trajectory, then the total number of steps could be the sum of the steps from the minimization and the molecular dynamics trajectory. I have not check this, but that is what I understood from the documentation. |
|
I didn't consider restart files. Yes, this could indeed happen... I'll think about this |
|
Concerning the restart files: I think we are save here because this is already handled in Therefore, frames always reset to 0 and no too complicated logic is required. I'll add a check to avoid that the last frame is dumped twice if it falls on a regular dump frame. |
|
Check PR #365 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_compatibility_file.py (1)
241-246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winEnable
dump_final_structurein the divisible-case test.Line 241 evaluates to
Falsefor1000 % 100, so this test never covers the intended “flag enabled, but no duplicate final frame” branch. It currently overlaps withtest_write_dump_if_missing_default_offinstead.Suggested fix
- dump_final_structure=n_ionic_steps % n_print != 0, + dump_final_structure=True,🤖 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_compatibility_file.py` around lines 241 - 246, The divisible-case coverage in test_write_dump_if_missing_default_off is wrong because dump_final_structure is currently disabled when n_ionic_steps is divisible by n_print, so this test never exercises the enabled-flag branch. Update the setup in test_compatibility_file.py to explicitly pass dump_final_structure=True for that case, and keep the existing assertions in place to verify that lmp.in does not contain a duplicate write_dump line.
🤖 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.
Outside diff comments:
In `@tests/test_compatibility_file.py`:
- Around line 241-246: The divisible-case coverage in
test_write_dump_if_missing_default_off is wrong because dump_final_structure is
currently disabled when n_ionic_steps is divisible by n_print, so this test
never exercises the enabled-flag branch. Update the setup in
test_compatibility_file.py to explicitly pass dump_final_structure=True for that
case, and keep the existing assertions in place to verify that lmp.in does not
contain a duplicate write_dump line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2c77544-6122-458b-96ae-eace6d9bb565
📒 Files selected for processing (2)
src/lammpsparser/compatibility/file.pytests/test_compatibility_file.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lammpsparser/compatibility/file.py
|
@Gitdowski From my perspective this is ready to be merged. If you agree we can merge these changes and release a new version. |
|
@jan-janssen Yes! |
Summary
dumpcommand silently skips the final configuration of an MD run whenn_ionic_stepsis not evenly divisible byn_print, even thoughlog.lammpsstill reports thermo data for that step (demonstrated innotebooks/2026/2026-06-27-lammps-dump/dump_frequency_v1.ipynb).write_dump_if_missingflag (defaultFalse) tolammps_file_interface_function. When enabled and the MD run's step count isn't evenly divisible byn_print, awrite_dump ... append yescommand is appended right after theruncommand, capturing the missing final structure into the samedump.outfile using the same fields/format as the regulardumpcommand.Test plan
tests/test_compatibility_file.py: new tests assert the generatedlmp.indoes/doesn't contain thewrite_dumpline for (a) flag on + non-divisible, (b) flag on + divisible, (c) flag off (default)tests/test_output.py+ new static fixtures (tests/static/dump_missing_final_step,tests/static/dump_with_final_step): characterize the bug (steps/positions length mismatch) and confirm parsing recovers correctly once the extra frame is presentpython -m pytest tests/— all pass except the pre-existingtest_compatibility_integration.pyfailure (segfaults locally due to MPI/LAMMPS binary setup, unrelated to this change, reproduced onmaintoo)pre-commit run ruff / ruff-format🤖 Generated with Claude Code
Summary by CodeRabbit
dump_final_structureoption (off by default) to capture and append the final MD structure via an additionalwrite_dump ... append yeswhen the last step isn’t aligned to the dump cadence.static/minimizemode now raises an error.