Add seed parameter to LAMMPS run configuration#570
Conversation
Langevin uses two seeds. One for initialising velocities and one for the fix itself. The seed for the fix should be present even when `disable_initial_velocity` is set to `True`
WalkthroughA single-line parameter addition to the LAMMPS template rendering in the Langevin molecular dynamics calculation function, passing the seed parameter when initial velocity is enabled to maintain consistency across initialization code paths. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
atomistics/calculators/lammps/libcalculator.py (1)
439-439: LGTM! Seed correctly added for Langevin fix.The addition of
seed=seedensures the Langevin thermostat fix receives a seed even when initial velocity initialization is disabled, which aligns with the PR objective.Minor: Consider consistent parameter ordering.
For readability, consider matching the parameter order between the two branches. In the
ifblock (line 418),seedcomes aftertimestep, but here it comes before. While functionally equivalent for keyword arguments, consistency aids maintainability.Apply this diff to align parameter order:
input_template = Template(init_str).render( thermo=thermo, temp=Tstart, Tstart=Tstart, Tstop=Tstop, Tdamp=Tdamp, - seed=seed, timestep=timestep, + seed=seed, )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
atomistics/calculators/lammps/libcalculator.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: unittest_grace
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_old
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_matrix (macos-latest, 3.14)
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_qe
- GitHub Check: unittest_siesta
- GitHub Check: unittest_mace
- GitHub Check: notebooks
- GitHub Check: minimal
- GitHub Check: coverage
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #570 +/- ##
=======================================
Coverage 85.46% 85.46%
=======================================
Files 43 43
Lines 2545 2545
=======================================
Hits 2175 2175
Misses 370 370 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Tstart=Tstart, | ||
| Tstop=Tstop, | ||
| Tdamp=Tdamp, | ||
| seed=seed, |
There was a problem hiding this comment.
I though the seed is only used in LAMMPS_VELOCITY, as the LAMMPS_VELOCITY is not included in the else part it should not be necessary here. Or am I missing something?
There was a problem hiding this comment.
So, using the disable_initial_velocity = True, without a seed, causes LAMMPS to throw an error:
ERROR: Illegal fix langevin command (src/fix_langevin.cpp:59).
On the LAMMPS documentation for the fix langevin, the command mentions a seed, that is independent of the seed for the velocities:
fix ID group-ID langevin Tstart Tstop damp seed keyword values ...
Also, in commands.py, the fix langevin command asks for a seed too:
LAMMPS_LANGEVIN = "fix ensemble all langevin {{Tstart}} {{Tstop}} {{Tdamp}} {{seed}}"
There was a problem hiding this comment.
You are right, I missed the part it is in https://github.com/pyiron/atomistics/blob/langevin_seed_correction/atomistics/calculators/lammps/commands.py#L35
Langevin uses two seeds. One for initialising velocities and one for the fix itself. The seed for the fix should be present even when
disable_initial_velocityis set toTrueSummary by CodeRabbit