Add n_print_dump and n_print_thermo to control dump/thermo intervals independently#378
Add n_print_dump and n_print_thermo to control dump/thermo intervals independently#378jan-janssen wants to merge 1 commit into
Conversation
…independently lammps_file_interface_function previously derived both the dump-file write interval and the thermodynamic print interval from the single n_print key in calc_kwargs. These new optional parameters let callers override each independently, defaulting to the existing behavior when left unset.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #378 +/- ##
=======================================
Coverage 98.77% 98.77%
=======================================
Files 13 13
Lines 1223 1226 +3
=======================================
+ Hits 1208 1211 +3
Misses 15 15 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
📝 WalkthroughWalkthroughThe compatibility interface now accepts separate dump and thermo intervals. MD input generation uses these values independently, while static mode keeps thermo output unchanged. Tests cover the new behavior across MD, minimize, and static execution modes. ChangesOutput interval overrides
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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_compatibility_file.py (2)
755-778: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the independent final-dump decision.
These tests verify
dumptime, but not the changed Line 211 behavior. Add a case withn_print=100,n_print_dump=50,n_ionic_steps=950, anddump_final_structure=True; the generated input should not containwrite_dumpbecause 950 is divisible by 50.🤖 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 755 - 778, Extend test_n_print_dump_override_md to use n_ionic_steps=950 and dump_final_structure=True while keeping n_print=100 and n_print_dump=50, then assert the generated lmp.in does not contain “write_dump”. Preserve the existing dumptime and thermotime assertions.
762-762: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse dummy bindings for unused test outputs.
Ruff flags
shell_outputandparsed_outputin all five new tests. Replace them with_or underscore-prefixed names.Proposed cleanup
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, _, job_crashed = lammps_file_interface_function(Also applies to: 787-787, 812-812, 836-836, 856-856
🤖 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` at line 762, Update all five new test calls to lammps_file_interface_function so unused shell_output and parsed_output results use dummy bindings such as underscores or underscore-prefixed names, while preserving job_crashed and the existing test behavior.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 `@src/lammpsparser/compatibility/file.py`:
- Around line 125-129: Validate the effective dump interval selected by
dump_interval and n_print_thermo before generating the LAMMPS input: each must
be an integer greater than zero, rejecting zero, negative, and non-integer
values with a clear validation error. Apply the checks in the relevant
input-generation flow before the interval is used at the schedule calculation
around line 211, and add failure tests covering invalid values.
---
Nitpick comments:
In `@tests/test_compatibility_file.py`:
- Around line 755-778: Extend test_n_print_dump_override_md to use
n_ionic_steps=950 and dump_final_structure=True while keeping n_print=100 and
n_print_dump=50, then assert the generated lmp.in does not contain “write_dump”.
Preserve the existing dumptime and thermotime assertions.
- Line 762: Update all five new test calls to lammps_file_interface_function so
unused shell_output and parsed_output results use dummy bindings such as
underscores or underscore-prefixed names, while preserving job_crashed and the
existing test behavior.
🪄 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 Plus
Run ID: e3a00e55-efef-4ae8-8e0d-68684949c467
📒 Files selected for processing (2)
src/lammpsparser/compatibility/file.pytests/test_compatibility_file.py
| dump_interval = ( | ||
| n_print_dump if n_print_dump is not None else calc_kwargs.get("n_print", 1) | ||
| ) | ||
| if n_print_thermo is not None: | ||
| calc_kwargs["n_print"] = n_print_thermo |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject invalid output intervals before generating the input.
n_print_dump=0 reaches Line 211 and causes ZeroDivisionError; non-positive or non-integer values can also generate invalid LAMMPS schedules. Validate the effective dump interval and n_print_thermo as positive integers, and add failure tests.
Proposed validation
dump_interval = (
n_print_dump if n_print_dump is not None else calc_kwargs.get("n_print", 1)
)
+ if type(dump_interval) is not int or dump_interval <= 0:
+ raise ValueError("Dump interval must be a positive integer")
if n_print_thermo is not None:
+ if type(n_print_thermo) is not int or n_print_thermo <= 0:
+ raise ValueError("Thermo interval must be a positive integer")
calc_kwargs["n_print"] = n_print_thermoAlso applies to: 166-166, 211-211
🤖 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 125 - 129, Validate the
effective dump interval selected by dump_interval and n_print_thermo before
generating the LAMMPS input: each must be an integer greater than zero,
rejecting zero, negative, and non-integer values with a clear validation error.
Apply the checks in the relevant input-generation flow before the interval is
used at the schedule calculation around line 211, and add failure tests covering
invalid values.
1 participant
lammps_file_interface_function()previously derived both the dump-file write interval and the thermodynamic print interval from the singlen_printkey incalc_kwargs. These new optional parameters let callers override each independently, defaulting to the existing behavior when left unset.Summary by CodeRabbit
New Features
Tests