LAMMPS: Allow user to change the velocity rescaling - default remains 2 #608
Conversation
📝 WalkthroughWalkthroughThe pull request replaces a hardcoded velocity scaling factor (2) with a configurable Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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
🤖 Fix all issues with AI agents
In @src/atomistics/calculators/lammps/helpers.py:
- Line 105: The parameter velocity_rescale_factor in
lammps_thermal_expansion_loop is typed as float and will produce an invalid
LAMMPS command if None is passed; change its type to Optional[float] and add the
same conditional guard used in libcalculator.py: only emit/compose the LAMMPS
velocity initialization/rescale command when velocity_rescale_factor is not
None, otherwise skip velocity initialization entirely to avoid generating "None
* temp" in the command.
🧹 Nitpick comments (2)
tests/test_lammpslib_md.py (1)
166-167: Consider adding tests for custom velocity rescale factors.Both "no velocity" tests use
velocity_rescale_factor=None, but there are no tests validating custom non-default values (e.g.,velocity_rescale_factor=1.0orvelocity_rescale_factor=3.0). Adding such a test would help ensure the configurable factor works as expected.src/atomistics/calculators/lammps/libcalculator.py (1)
465-514: Consider exposingvelocity_rescale_factorin the thermal expansion function.The
calc_molecular_dynamics_thermal_expansion_with_lammpslibfunction always usesLAMMPS_VELOCITY(line 491) but doesn't expose thevelocity_rescale_factorparameter to callers. It relies on the default value (2.0) inlammps_thermal_expansion_loop.For API consistency with other MD functions, consider adding the parameter here and passing it through to
lammps_thermal_expansion_loop.♻️ Suggested change
def calc_molecular_dynamics_thermal_expansion_with_lammpslib( structure: Atoms, potential_dataframe: pandas.DataFrame, Tstart: float = 15.0, Tstop: float = 1500.0, Tstep: int = 5, Tdamp: float = 0.1, run: int = 100, thermo: int = 100, timestep: float = 0.001, Pstart: float = 0.0, Pstop: float = 0.0, Pdamp: float = 1.0, seed: int = 4928459, dist: str = "gaussian", + velocity_rescale_factor: float = 2.0, lmp=None, output_keys=OutputThermalExpansion.keys(), **kwargs, ) -> dict: ... return lammps_thermal_expansion_loop( ... dist=dist, + velocity_rescale_factor=velocity_rescale_factor, lmp=lmp, ... )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/atomistics/calculators/lammps/commands.pysrc/atomistics/calculators/lammps/helpers.pysrc/atomistics/calculators/lammps/libcalculator.pytests/test_lammpslib_md.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/atomistics/calculators/lammps/libcalculator.py (1)
src/atomistics/shared/output.py (2)
OutputMolecularDynamics(42-51)keys(11-18)
🪛 Ruff (0.14.10)
src/atomistics/calculators/lammps/libcalculator.py
159-159: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
238-238: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
321-321: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
397-397: Do not perform function call OutputMolecularDynamics.keys in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
⏰ 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). (15)
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_grace
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (macos-latest, 3.14)
- GitHub Check: unittest_mace
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_siesta
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: unittest_qe
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: unittest_old
- GitHub Check: notebooks
- GitHub Check: unittest_sphinxdft
- GitHub Check: coverage
🔇 Additional comments (8)
tests/test_lammpslib_md.py (1)
74-75: Test correctly validates the newvelocity_rescale_factor=Nonebehavior.The test verifies that when
velocity_rescale_factor=None, the simulation starts without initial velocities, resulting in near-zero temperature (lines 84-85). This correctly validates the replacement of the olddisable_initial_velocity=Truebehavior.src/atomistics/calculators/lammps/commands.py (1)
23-23: Clean parameterization of the velocity rescale factor.The template correctly replaces the hardcoded
2with the configurable{{velocity_rescale_factor}}placeholder, maintaining the same LAMMPS command structure.src/atomistics/calculators/lammps/helpers.py (1)
119-119: Propagation looks correct for the thermal expansion use case.The
velocity_rescale_factoris correctly passed to the template render context, which will substitute intoLAMMPS_VELOCITYcommand.src/atomistics/calculators/lammps/libcalculator.py (5)
157-184: NVT implementation correctly handles the newvelocity_rescale_factorparameter.The conditional logic properly branches between including
LAMMPS_VELOCITY(when factor is not None) and excluding it (when None). The parameter is correctly passed to the template render.
236-266: NPT implementation consistent with NVT.The pattern follows the same correct structure as the NVT function.
319-346: NPH implementation consistent with other MD functions.
395-424: Langevin implementation consistent with other MD functions.
159-159: Pre-existing static analysis warning (B008).The Ruff B008 warning about function calls in default arguments (
OutputMolecularDynamics.keys()) is a pre-existing pattern throughout this file and not introduced by this PR. This is a known trade-off for API discoverability.
| Pdamp: float = 1.0, | ||
| seed: int = 4928459, | ||
| dist: str = "gaussian", | ||
| velocity_rescale_factor: float = 2.0, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the function at line 105 and surrounding context
sed -n '90,120p' src/atomistics/calculators/lammps/helpers.pyRepository: pyiron/atomistics
Length of output: 945
🏁 Script executed:
# Find the corresponding function in libcalculator.py
rg -n "velocity_rescale_factor" src/ --type py -B 2 -A 2Repository: pyiron/atomistics
Length of output: 6183
🏁 Script executed:
# Search for how velocity_rescale_factor is used in the template rendering
rg -n "velocity_rescale_factor" src/atomistics/calculators/lammps/ --type pyRepository: pyiron/atomistics
Length of output: 1705
Type inconsistency with libcalculator.py functions.
lammps_thermal_expansion_loop uses velocity_rescale_factor: float = 2.0, while all MD functions in libcalculator.py use Optional[float] = 2.0 with conditional logic to handle None values. The helpers.py function lacks this guard, so passing None would render "None * temp" in the LAMMPS velocity command, causing an error.
Align the type to Optional[float] and add the same conditional logic used in libcalculator.py (skip velocity initialization when None), or document that this function always requires a numeric value.
🤖 Prompt for AI Agents
In @src/atomistics/calculators/lammps/helpers.py at line 105, The parameter
velocity_rescale_factor in lammps_thermal_expansion_loop is typed as float and
will produce an invalid LAMMPS command if None is passed; change its type to
Optional[float] and add the same conditional guard used in libcalculator.py:
only emit/compose the LAMMPS velocity initialization/rescale command when
velocity_rescale_factor is not None, otherwise skip velocity initialization
entirely to avoid generating "None * temp" in the command.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #608 +/- ##
=======================================
Coverage 86.81% 86.81%
=======================================
Files 43 43
Lines 2419 2419
=======================================
Hits 2100 2100
Misses 319 319 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.