LAMMPS: Add vmax option for structure optimization #607
Conversation
📝 WalkthroughWalkthroughAdds an optional Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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: 2
🤖 Fix all issues with AI agents
In @src/atomistics/calculators/lammps/libcalculator.py:
- Around line 599-606: The _get_vmax_command function currently uses
isinstance(vmax, float) which rejects integer inputs like vmax=5; update the
type check in _get_vmax_command to accept both int and float (e.g.,
isinstance(vmax, (int, float)) or use numbers.Real) and format the value into
the returned string unchanged; also shorten the TypeError message (e.g., "vmax
must be a number") for consistency or replace with a custom exception if your
codebase prefers.
- Around line 52-57: The _get_vmax_command function currently only checks type
but not range; update it to validate that if vmax is not None it must be a float
and strictly > 0.0, otherwise raise ValueError (for non-positive values) or
TypeError (for wrong type); when valid, return the existing command string with
the vmax inserted (e.g., LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}"), otherwise
return LAMMPS_MINIMIZE_VOLUME when vmax is None.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/atomistics/calculators/lammps/libcalculator.py
🧰 Additional context used
🪛 Ruff (0.14.10)
src/atomistics/calculators/lammps/libcalculator.py
604-604: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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_siesta
- GitHub Check: unittest_grace
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_mace
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (macos-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: notebooks
- GitHub Check: unittest_qe
- GitHub Check: unittest_sphinxdft
- GitHub Check: unittest_old
- GitHub Check: coverage
🔇 Additional comments (1)
src/atomistics/calculators/lammps/libcalculator.py (1)
3-3: LGTM!The
Optionalimport is correctly added to support the newvmaxparameter type hint.
| vmax: Optional[float] = None, | ||
| lmp=None, | ||
| **kwargs, | ||
| ) -> Atoms: | ||
| template_str = ( | ||
| LAMMPS_MINIMIZE_VOLUME | ||
| _get_vmax_command(vmax=vmax) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
LAMMPS fix box/relax vmax parameter documentation valid values
💡 Result:
The vmax value is a positive fractional limit on how much the box may change in one minimizer iteration. Valid values:
- Any number > 0.0 (should be much less than 1.0).
- Interpreted as maximum fractional volume change per iteration when using couple xyz; otherwise it limits fractional change of any linear box dimension.
- Example recommended value: 0.001.
- Default if not specified: 0.0001.
References: LAMMPS fix box/relax documentation. [1][2]
Sources
[1] LAMMPS fix box/relax manual (release).
[2] LAMMPS documentation / examples (fix box/relax description).
🏁 Script executed:
# Find the _get_vmax_command function to understand implementation
fd -t f "libcalculator.py" | head -20Repository: pyiron/atomistics
Length of output: 111
🏁 Script executed:
# Search for vmax-related code and usage
rg "vmax" -B 3 -A 5 "src/atomistics/calculators/lammps/libcalculator.py" | head -60Repository: pyiron/atomistics
Length of output: 704
🏁 Script executed:
# Find the _get_vmax_command function definition
rg "_get_vmax_command" -B 2 -A 10 --type pyRepository: pyiron/atomistics
Length of output: 1854
Add validation for vmax parameter to ensure it is positive.
According to LAMMPS documentation, vmax must be a positive number (> 0.0). The current implementation only checks if the value is a float type, but does not validate the valid range. This should be enforced in _get_vmax_command() to provide immediate feedback to users rather than letting LAMMPS fail at runtime.
Suggested validation addition
def _get_vmax_command(vmax: Optional[float]) -> str:
if vmax is not None:
if isinstance(vmax, float):
if vmax <= 0.0:
raise ValueError(f"vmax must be positive (> 0.0), got {vmax}")
return LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}".format(vmax=vmax)
else:
raise TypeError("vmax must be a float.")
else:
return LAMMPS_MINIMIZE_VOLUME🤖 Prompt for AI Agents
In @src/atomistics/calculators/lammps/libcalculator.py around lines 52 - 57, The
_get_vmax_command function currently only checks type but not range; update it
to validate that if vmax is not None it must be a float and strictly > 0.0,
otherwise raise ValueError (for non-positive values) or TypeError (for wrong
type); when valid, return the existing command string with the vmax inserted
(e.g., LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}"), otherwise return
LAMMPS_MINIMIZE_VOLUME when vmax is None.
| def _get_vmax_command(vmax: Optional[float]) -> str: | ||
| if vmax is not None: | ||
| if isinstance(vmax, float): | ||
| return LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}".format(vmax=vmax) | ||
| else: | ||
| raise TypeError("vmax must be a float.") | ||
| else: | ||
| return LAMMPS_MINIMIZE_VOLUME |
There was a problem hiding this comment.
Fix type check to accept integers.
The isinstance(vmax, float) check on Line 601 will reject integer values (e.g., vmax=5), even though integers are valid numeric inputs for this parameter. In Python, isinstance(5, float) returns False.
🐛 Proposed fix to accept both int and float
def _get_vmax_command(vmax: Optional[float]) -> str:
if vmax is not None:
- if isinstance(vmax, float):
+ if isinstance(vmax, (int, float)):
return LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}".format(vmax=vmax)
else:
raise TypeError("vmax must be a float.")
else:
return LAMMPS_MINIMIZE_VOLUMEOptional: Consider refactoring the error message.
As per static analysis hint (TRY003), consider using a shorter error message or defining a custom exception class for consistency with Python conventions.
♻️ Example refactor
def _get_vmax_command(vmax: Optional[float]) -> str:
if vmax is not None:
if isinstance(vmax, (int, float)):
return LAMMPS_MINIMIZE_VOLUME + " vmax {vmax}".format(vmax=vmax)
else:
- raise TypeError("vmax must be a float.")
+ raise TypeError(f"Expected numeric vmax, got {type(vmax).__name__}")
else:
return LAMMPS_MINIMIZE_VOLUME🧰 Tools
🪛 Ruff (0.14.10)
604-604: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In @src/atomistics/calculators/lammps/libcalculator.py around lines 599 - 606,
The _get_vmax_command function currently uses isinstance(vmax, float) which
rejects integer inputs like vmax=5; update the type check in _get_vmax_command
to accept both int and float (e.g., isinstance(vmax, (int, float)) or use
numbers.Real) and format the value into the returned string unchanged; also
shorten the TypeError message (e.g., "vmax must be a number") for consistency or
replace with a custom exception if your codebase prefers.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #607 +/- ##
==========================================
- Coverage 86.81% 86.72% -0.10%
==========================================
Files 43 43
Lines 2419 2425 +6
==========================================
+ Hits 2100 2103 +3
- Misses 319 322 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Looks good to me. But code rabbit's comment also makes sense. |
Is a value of |
|
You're absolutely right. Even the LAMMPS documentation says it's a fraction. |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.