Add Lammps function for backwards compatibility to pyiron_atomistics#262
Add Lammps function for backwards compatibility to pyiron_atomistics#262jan-janssen merged 61 commits intomainfrom
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a LAMMPS compatibility layer: calculation helpers (calc_md, calc_minimize, calc_static), a file-based runner and initializer (lammps_file_interface_function, lammps_file_initialization), LAMMPS-compatible structure utilities and atom_type handling, plus tests and a static LAMMPS log fixture. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Interface as lammps_file_interface_function
participant Init as lammps_file_initialization
participant Calc as calc_md/calc_minimize/calc_static
participant DataUtil as write_lammps_datafile
participant FS as Filesystem
participant LAMMPS as LAMMPS_Process
participant Parser as parse_lammps_output
User->>Interface: call(working_directory, structure, potential, calc_mode, calc_kwargs)
Interface->>FS: ensure working directory exists
Interface->>Init: generate initial directives (units, dimension, boundary, read_data)
Interface->>Calc: build calculation lines for selected mode
Calc-->>Interface: return calc lines
Interface->>DataUtil: write_lammps_datafile(structure, atom_type=...)
Interface->>FS: write `lmp.in` (init + potential + calc lines)
Interface->>LAMMPS: run lmp_command
LAMMPS-->>FS: produce logs/output files
Interface->>Parser: parse_lammps_output(log files)
Parser-->>Interface: parsed results
Interface-->>User: return (shell_output, parsed_output, False)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #262 +/- ##
==========================================
- Coverage 95.85% 92.03% -3.83%
==========================================
Files 5 7 +2
Lines 652 854 +202
==========================================
+ Hits 625 786 +161
- Misses 27 68 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
# Conflicts: # tests/test_potential.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
pyiron_lammps/compatibility/calculate.py (3)
71-88: Warnings should use an appropriate category andstacklevelAll deprecation/behaviour warnings (
delta_temp,delta_press,langevinwithout temperature,n_print > max_iter) currently callwarnings.warnwithout a category orstacklevel. This makes it hard for users to silence or locate the originating call site.Consider, for each of these, using
DeprecationWarning(for deprecated args) orUserWarningand settingstacklevel=2, e.g.:- warnings.warn( - "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`." - ) + warnings.warn( + "`delta_temp` is deprecated, please use `temperature_damping_timescale`.", + DeprecationWarning, + stacklevel=2, + )and similarly for
delta_press, the Langevin‑ignored case, and then_printadjustment.Also applies to: 175-175, 240-243
117-137:force_skewedis dead code
force_skewedis initialised toFalseand potentially set toTruein the pressure loop, but never read. This is just noise now.You can safely remove it and, if you still want to keep the comment about skewed cells, keep that in
_pressure_to_lammpswhere the actual handling lives.- force_skewed = False @@ - if ii > 2: - force_skewed = True
295-305: Minor:calc_staticlist construction can be simplified
calc_staticworks, but concatenating lists is a bit less clear than extending:def calc_static(): return ["variable thermotime equal 1"] + _get_thermo() + ["run 0"]You may consider using unpacking for readability:
def calc_static(): return [ "variable thermotime equal 1", *_get_thermo(), "run 0", ]This also addresses the Ruff RUF005 hint.
pyiron_lammps/__init__.py (1)
2-6: New compatibility helpers are imported but not added to__all__Importing
calc_md,calc_minimize,calc_static,lammps_file_initialization, andlammps_file_interface_functionat module level correctly exposes them aspyiron_lammps.<name>.However, they are not included in
__all__, sofrom pyiron_lammps import *will not export them.If the intent is to treat these as part of the official public API (for backwards compatibility), consider extending
__all__:__all__ = [ "get_potential_by_name", "get_potential_dataframe", "parse_lammps_output_files", "write_lammps_structure", + "calc_md", + "calc_minimize", + "calc_static", + "lammps_file_initialization", + "lammps_file_interface_function", ]If you prefer a narrower star‑import surface, leaving
__all__unchanged is fine and the direct imports are sufficient.Also applies to: 22-28
pyiron_lammps/compatibility/file.py (2)
101-105: Minor: use a clearer loop variable name when writing the input fileThe list comprehension:
with open(..., "w") as f: f.writelines([l + "\n" for l in lmp_str_lst])uses the single‑letter name
l, which Ruff flags as ambiguous (E741) and is a bit hard to read.A simple readability tweak:
- with open(os.path.join(working_directory, "lmp.in"), "w") as f: - f.writelines([l + "\n" for l in lmp_str_lst]) + with open(os.path.join(working_directory, "lmp.in"), "w") as f: + f.writelines([line + "\n" for line in lmp_str_lst])also makes static analysis happy.
Also applies to: 123-124
146-155:lammps_file_initializationlooks correct; just a tiny clean‑up opportunityThe initialization commands are straightforward and consistent with ASE’s
pbc:boundary = " ".join(["p" if coord else "f" for coord in structure.pbc]) init_commands = [ "units " + units, "dimension " + str(dimension), "boundary " + boundary + "", "atom_style atomic", "read_data lammps.data", ]The
+ ""is redundant; you could simplify that line to:- "boundary " + boundary + "", + f"boundary {boundary}",but functionally it is fine as‑is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/static/potentials_lammps.csvis excluded by!**/*.csv
📒 Files selected for processing (3)
pyiron_lammps/__init__.py(1 hunks)pyiron_lammps/compatibility/calculate.py(1 hunks)pyiron_lammps/compatibility/file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pyiron_lammps/compatibility/file.py (4)
pyiron_lammps/compatibility/calculate.py (3)
calc_md(8-198)calc_minimize(201-292)calc_static(295-296)pyiron_lammps/output.py (1)
parse_lammps_output(73-135)pyiron_lammps/potential.py (1)
get_potential_by_name(349-355)pyiron_lammps/structure.py (1)
write_lammps_datafile(453-464)
pyiron_lammps/__init__.py (2)
pyiron_lammps/compatibility/calculate.py (3)
calc_md(8-198)calc_minimize(201-292)calc_static(295-296)pyiron_lammps/compatibility/file.py (2)
lammps_file_initialization(146-155)lammps_file_interface_function(17-143)
🪛 Ruff (0.14.5)
pyiron_lammps/compatibility/calculate.py
73-73: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
82-82: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
107-107: Avoid specifying long messages outside the exception class
(TRY003)
113-115: Avoid specifying long messages outside the exception class
(TRY003)
136-136: Local variable force_skewed is assigned to but never used
Remove assignment to unused variable force_skewed
(F841)
155-157: Avoid specifying long messages outside the exception class
(TRY003)
175-175: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
242-242: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
255-258: Avoid specifying long messages outside the exception class
(TRY003)
267-267: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
296-296: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
388-388: Avoid specifying long messages outside the exception class
(TRY003)
396-403: Avoid specifying long messages outside the exception class
(TRY003)
pyiron_lammps/compatibility/file.py
119-121: Avoid specifying long messages outside the exception class
(TRY003)
124-124: Ambiguous variable name: l
(E741)
126-126: subprocess call with shell=True identified, security issue
(S602)
⏰ 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). (1)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
🔇 Additional comments (1)
pyiron_lammps/compatibility/calculate.py (1)
307-313:_is_isotropic_hydrostaticassumes a sequence of length ≥ 6The helper assumes
pressure[:3]andpressure[3:]slices behave as expected for a 6‑element sequence and thatNone in ...is meaningful. Since_pressure_to_lammpsnormalizes to a 6‑entry list before calling this, that’s fine, but it’s tightly coupled.If you expect external calls to
_is_isotropic_hydrostatic, consider documenting that it must receive a length‑6 list and is not general‑purpose. Otherwise, this is OK as an internal helper.
|
|
||
| def calc_minimize( | ||
| ionic_energy_tolerance=0.0, | ||
| ionic_force_tolerance=1e-4, | ||
| max_iter=100000, | ||
| pressure=None, | ||
| n_print=100, | ||
| style="cg", | ||
| rotation_matrix=None, | ||
| units="metal", | ||
| ): | ||
| """ | ||
| Sets parameters required for minimization. | ||
|
|
||
| Args: | ||
| ionic_energy_tolerance (float): If the magnitude of difference between energies of two consecutive steps is | ||
| lower than or equal to `ionic_energy_tolerance`, the minimisation terminates. (Default is 0.0 eV.) | ||
| ionic_force_tolerance (float): If the magnitude of the global force vector at a step is lower than or equal | ||
| to `ionic_force_tolerance`, the minimisation terminates. (Default is 1e-4 eV/angstrom.) | ||
| e_tol (float): Same as ionic_energy_tolerance (deprecated) | ||
| f_tol (float): Same as ionic_force_tolerance (deprecated) | ||
| max_iter (int): Maximum number of minimisation steps to carry out. If the minimisation converges before | ||
| `max_iter` steps, terminate at the converged step. If the minimisation does not converge up to | ||
| `max_iter` steps, terminate at the `max_iter` step. (Default is 100000.) | ||
| pressure (None/float/numpy.ndarray/list): Target pressure. If set to None, an isochoric (constant V) | ||
| calculation is performed. If set to a scalar, the shear of the cell and the ratio of the x, y, and | ||
| z components is kept constant, while an isotropic, hydrostatic pressure is applied. A list of up to | ||
| length 6 can be given to specify xx, yy, zz, xy, xz, and yz components of the pressure tensor, | ||
| respectively. These values can mix floats and `None` to allow only certain degrees of cell freedom | ||
| to change. (Default is None, run isochorically.) | ||
| n_print (int): Write (dump or print) to the output file every n steps (Default: 100) | ||
| style ('cg'/'sd'/other values from Lammps docs): The style of the numeric minimization, either conjugate | ||
| gradient, steepest descent, or other keys permissible from the Lammps docs on 'min_style'. (Default | ||
| is 'cg' -- conjugate gradient.) | ||
| rotation_matrix (numpy.ndarray): The rotation matrix from the pyiron to Lammps coordinate frame. | ||
| """ | ||
| # This docstring is a source for the calc_minimize method in pyiron_atomistics.lammps.base.LammpsBase.calc_minimize and | ||
| # pyiron_atomistics.lammps.interactive.LammpsInteractive.calc_minimize -- Please ensure that changes to signature or | ||
| # defaults stay consistent! | ||
|
|
||
| max_evaluations = 100 * max_iter | ||
| if n_print > max_iter: | ||
| warnings.warn("n_print larger than max_iter, adjusting to n_print=max_iter") | ||
| n_print = max_iter | ||
|
|
||
| if units not in LAMMPS_UNIT_CONVERSIONS.keys(): | ||
| raise NotImplementedError | ||
| energy_units = LAMMPS_UNIT_CONVERSIONS[units]["energy"] | ||
| force_units = LAMMPS_UNIT_CONVERSIONS[units]["force"] | ||
|
|
||
| ionic_energy_tolerance *= energy_units | ||
| ionic_force_tolerance *= force_units | ||
|
|
||
| if pressure is not None: | ||
| if rotation_matrix is None: | ||
| raise ValueError( | ||
| "No rotation matrix given while trying to convert pressure. " | ||
| "This is most likely due to no structure being defined." | ||
| ) | ||
| # force_skewed = False | ||
| pressure = _pressure_to_lammps( | ||
| pressure=pressure, rotation_matrix=rotation_matrix, units=units | ||
| ) | ||
| if np.isscalar(pressure): | ||
| str_press = " iso {}".format(pressure) | ||
| else: | ||
| str_press = "" | ||
| for ii, (press, str_axis) in enumerate( | ||
| zip(pressure, ["x", "y", "z", "xy", "xz", "yz"]) | ||
| ): | ||
| if press is not None: | ||
| str_press += " {} {}".format(str_axis, press) | ||
| # if ii > 2: | ||
| # force_skewed = True | ||
| if len(str_press) > 1: | ||
| str_press += " couple none" | ||
| line_lst = [ | ||
| "variable thermotime equal {} ".format(n_print), | ||
| ] | ||
| line_lst += [ | ||
| _get_thermo(), | ||
| r"fix ensemble all box/relax" + str_press, | ||
| "min_style " + style, | ||
| "minimize " | ||
| + str(ionic_energy_tolerance) | ||
| + " " | ||
| + str(ionic_force_tolerance) | ||
| + " " | ||
| + str(int(max_iter)) | ||
| + " " | ||
| + str(int(max_evaluations)), | ||
| ] |
There was a problem hiding this comment.
calc_minimize has a likely NameError and nests _get_thermo() as a list
In calc_minimize:
str_pressis only defined inside theif pressure is not None:block, but you unconditionally use it in:
line_lst += [
_get_thermo(),
r"fix ensemble all box/relax" + str_press,
...
]If pressure is None (isochoric minimisation) this will raise UnboundLocalError: local variable 'str_press' referenced before assignment.
_get_thermo()is added as a single element inside the list literal, soline_lstends up containing a nested list, andlammps_file_interface_functionlater doesl + "\n"for each element. When it hits the nested list from_get_thermo(), this will raiseTypeError.
A minimal fix:
- if pressure is not None:
+ str_press = ""
+ if pressure is not None:
@@
- line_lst = [
- "variable thermotime equal {} ".format(n_print),
- ]
- line_lst += [
- _get_thermo(),
- r"fix ensemble all box/relax" + str_press,
- "min_style " + style,
- "minimize "
- + str(ionic_energy_tolerance)
- + " "
- + str(ionic_force_tolerance)
- + " "
- + str(int(max_iter))
- + " "
- + str(int(max_evaluations)),
- ]
+ line_lst = ["variable thermotime equal {} ".format(n_print)]
+ line_lst += _get_thermo()
+ line_lst += [
+ r"fix ensemble all box/relax" + str_press,
+ f"min_style {style}",
+ "minimize "
+ + str(ionic_energy_tolerance)
+ + " "
+ + str(ionic_force_tolerance)
+ + " "
+ + str(int(max_iter))
+ + " "
+ + str(int(max_evaluations)),
+ ]This both avoids the NameError and keeps line_lst flat so file writing works.
🧰 Tools
🪛 Ruff (0.14.5)
242-242: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
255-258: Avoid specifying long messages outside the exception class
(TRY003)
267-267: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
🤖 Prompt for AI Agents
In pyiron_lammps/compatibility/calculate.py around lines 200-291, fix two
issues: ensure str_press is always defined and avoid inserting _get_thermo() as
a nested list; set str_press = "" before the pressure conditional so it exists
for the isochoric path, and replace the list insertion that does line_lst += [
_get_thermo(), ... ] with a flat extension (e.g. line_lst += _get_thermo() or
line_lst.extend(_get_thermo())) and then append the remaining string lines (or
use line_lst += [ "fix ensemble all box/relax" + str_press, "min_style "+style,
"minimize "+... ]) so line_lst remains a flat list of strings.
| Args: | ||
| working_directory (str): directory in which the LAMMPS calculation is executed | ||
| structure (Atoms): ase.atoms.Atoms - atomistic structure | ||
| potential (str): Name of the LAMMPS potential based on the NIST database and the OpenKIM database | ||
| calc_mode (str): select mode of calculation ["static", "md", "minimize", "vcsgc"] | ||
| calc_kwargs (dict): key-word arguments for the calculate function, the input parameters depend on the calc_mode: | ||
| "static": No parameters | ||
| "md": "temperature", "pressure", "n_ionic_steps", "time_step", "n_print", "temperature_damping_timescale", | ||
| "pressure_damping_timescale", "seed", "tloop", "initial_temperature", "langevin", "delta_temp", | ||
| "delta_press", job_name", "rotation_matrix" | ||
| "minimize": "ionic_energy_tolerance", "ionic_force_tolerance", "max_iter", "pressure", "n_print", "style", | ||
| "rotation_matrix" | ||
| "vcsgc": "mu", "ordered_element_list", "target_concentration", "kappa", "mc_step_interval", "swap_fraction", | ||
| "temperature_mc", "window_size", "window_moves", "temperature", "pressure", "n_ionic_steps", | ||
| "time_step", "n_print", "temperature_damping_timescale", "pressure_damping_timescale", "seed", | ||
| "initial_temperature", "langevin", "job_name", "rotation_matrix" | ||
| cutoff_radius (float): cut-off radius for the interatomic potential | ||
| units (str): Units for LAMMPS | ||
| bonds_kwargs (dict): key-word arguments to create atomistic bonds: | ||
| "species", "element_list", "cutoff_list", "max_bond_list", "bond_type_list", "angle_type_list", | ||
| server_kwargs (dict): key-word arguments to create server object - the available parameters are: | ||
| "user", "host", "run_mode", "queue", "qid", "cores", "threads", "new_h5", "structure_id", "run_time", | ||
| "memory_limit", "accept_crash", "additional_arguments", "gpus", "conda_environment_name", | ||
| "conda_environment_path" | ||
| enable_h5md (bool): activate h5md mode for LAMMPS | ||
| write_restart_file (bool): enable writing the LAMMPS restart file | ||
| read_restart_file (bool): enable loading the LAMMPS restart file | ||
| restart_file (str): file name of the LAMMPS restart file to copy | ||
| executable_version (str): LAMMPS version to for the execution | ||
| executable_path (str): path to the LAMMPS executable | ||
| input_control_file (str|list|dict): Option to modify the LAMMPS input file directly | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Function docstring is out of sync with the actual signature and supported modes
lammps_file_interface_function’s docstring still describes the much richer pyiron_atomistics.lammps.lammps.lammps_function interface:
- Mentions
calc_modeoptions including"vcsgc"(not supported here). - Lists many parameters (
cutoff_radius,bonds_kwargs,server_kwargs,enable_h5md, restart options, executable_*`, etc.) that are not present in the function signature and are not handled anywhere.
Currently, only:
(working_directory, structure, potential, calc_mode, calc_kwargs, units, lmp_command, resource_path)are accepted, and calc_mode is restricted to "static", "md", "minimize" (other values raise ValueError).
To avoid confusion for users of this new entry point:
- Either update the docstring to match this slimmer API and explicitly state that higher‑level features are provided elsewhere, or
- Extend the function to accept and honour (or at least plumb through) the documented arguments if full drop‑in compatibility is required.
Similarly, remove or clarify "vcsgc" in both the calc_mode description and calc_kwargs breakdown unless support is planned here.
Also applies to: 87-105, 107-121
🏁 Script executed:
#!/bin/bash
# Extract the function definition, docstring, and implementation
cat pyiron_lammps/compatibility/file.py | head -150 | tail -110Length of output: 5353
🏁 Script executed:
#!/bin/bash
# Search for calc_mode validation and ValueError handling
rg -n -A5 -B5 'calc_mode' pyiron_lammps/compatibility/file.pyLength of output: 2890
🏁 Script executed:
#!/bin/bash
# Look for parameter handling in the file (cutoff_radius, bonds_kwargs, etc.)
rg -n 'cutoff_radius|bonds_kwargs|server_kwargs|enable_h5md|restart_file|executable' pyiron_lammps/compatibility/file.pyLength of output: 793
Update docstring to match actual function signature and supported calc_modes
lammps_file_interface_function has a simplified API incompatible with its docstring. The function accepts only: working_directory, structure, potential, calc_mode, calc_kwargs, units, lmp_command, resource_path (8 parameters), but the docstring documents 18+ additional parameters (cutoff_radius, bonds_kwargs, server_kwargs, enable_h5md, restart/executable options, etc.) that are never handled.
Similarly, the docstring lists "vcsgc" as a supported calc_mode (line 52), but the implementation (lines 119–121) only accepts "static", "md", or "minimize", raising ValueError for any other value.
Users following the docstring will encounter immediate errors. Either:
- Update the docstring to match the current slimmer API and clarify that advanced features are in the upstream interface, or
- Accept and plumb the documented parameters if full compatibility is intended.
Also remove "vcsgc" from the docstring unless support is added.
| os.makedirs(working_directory, exist_ok=True) | ||
| potential_dataframe = get_potential_by_name( | ||
| potential_name=potential, resource_path=resource_path | ||
| ) | ||
| write_lammps_datafile( | ||
| structure=structure, | ||
| potential_elements=potential_dataframe["Species"], | ||
| bond_dict=None, | ||
| units=units, | ||
| file_name="lammps.data", | ||
| working_directory=working_directory, | ||
| ) | ||
| lmp_str_lst = lammps_file_initialization(structure=structure) | ||
| lmp_str_lst += potential_dataframe["Config"] | ||
| lmp_str_lst += ["variable dumptime equal {} ".format(calc_kwargs.get("n_print", 1))] | ||
| lmp_str_lst += [ | ||
| "dump 1 all custom ${dumptime} dump.out id type xsu ysu zsu fx fy fz vx vy vz", | ||
| 'dump_modify 1 sort id format line "%d %d %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g"', | ||
| ] |
There was a problem hiding this comment.
lammps_file_initialization ignores the units argument passed to lammps_file_interface_function
You accept units in lammps_file_interface_function and pass it to:
write_lammps_datafile(..., units=units, ...)parse_lammps_output(..., units=units, ...)
but you call:
lmp_str_lst = lammps_file_initialization(structure=structure)lammps_file_initialization has its own units parameter defaulting to "metal", so the LAMMPS input file will always contain:
units metal
even if the caller requested a different unit system. This desynchronises the simulation units from the datafile/output parsing.
You probably want:
- lmp_str_lst = lammps_file_initialization(structure=structure)
+ lmp_str_lst = lammps_file_initialization(structure=structure, units=units)so all components (input, datafile, parsing) agree on the unit system.
Also applies to: 123-124
🤖 Prompt for AI Agents
In pyiron_lammps/compatibility/file.py around lines 87 to 105 (and also check
lines 123-124), lammps_file_initialization is called without forwarding the
units parameter so the generated LAMMPS input always uses the default "metal";
update the call to pass units=units (e.g., lmp_str_lst =
lammps_file_initialization(structure=structure, units=units)) so the input,
datafile, and output parsing all use the same unit system.
| shell = subprocess.check_output( | ||
| lmp_command, | ||
| cwd=working_directory, | ||
| shell=True, | ||
| universal_newlines=True, | ||
| env=os.environ.copy(), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
subprocess.check_output(..., shell=True) may be risky if lmp_command is not trusted
The function executes:
shell = subprocess.check_output(
lmp_command,
cwd=working_directory,
shell=True,
universal_newlines=True,
env=os.environ.copy(),
)Using shell=True with a string command is standard in many HPC wrappers, but it means any untrusted content in lmp_command is interpreted by the shell.
If lmp_command can be influenced by untrusted input, you may want to:
- Switch to
shell=Falseand pass a list of arguments, e.g.["mpiexec", "-n", "1", "--oversubscribe", "lmp_mpi", "-in", "lmp.in"], or - Clearly document that
lmp_commandmust be trusted and is executed via the shell.
Given this is a low‑level interface, tightening it or documenting the expectation would be enough.
I'll verify this security concern by examining the code context to determine if lmp_command is actually user-controlled and if there's genuine risk.
🏁 Script executed:
#!/bin/bash
# Examine the full function context around the subprocess call
cat pyiron_lammps/compatibility/file.py | head -150 | tail -50Length of output: 1799
🏁 Script executed:
#!/bin/bash
# Get broader context of the file to understand function signature and parameters
grep -n "def " pyiron_lammps/compatibility/file.pyLength of output: 113
🏁 Script executed:
#!/bin/bash
# Search for lmp_command usage and origin in the file
rg -n "lmp_command" pyiron_lammps/compatibility/file.py -B 5 -A 5Length of output: 748
🏁 Script executed:
#!/bin/bash
# Check for any imports and the overall structure of this module
head -50 pyiron_lammps/compatibility/file.pyLength of output: 1609
I need to examine where this function is called to understand the actual risk profile.
---
🏁 Script executed:
```shell
#!/bin/bash
# Find all callers of lammps_file_interface_function
rg -n "lammps_file_interface_function" --type py -B 2 -A 2
Length of output: 624
🏁 Script executed:
#!/bin/bash
# Check if the function is exported or documented as public API
grep -r "lammps_file_interface_function" --include="*.py" --include="*.md" . 2>/dev/null | head -20Length of output: 137
🏁 Script executed:
#!/bin/bash
# Look for __init__.py or __all__ exports to see if function is in public API
find . -name "__init__.py" -type f -exec grep -l "lammps_file_interface_function" {} \;Length of output: 27
Tighten lmp_command handling to prevent shell injection
The security concern is valid. lmp_command is a user-controllable function parameter exported in the public API, passed directly to subprocess.check_output(..., shell=True) without validation. Callers can inject arbitrary shell commands.
Recommended fixes:
- Switch to
shell=Falseand acceptlmp_commandas a list:["mpiexec", "-n", "1", "--oversubscribe", "lmp_mpi", "-in", "lmp.in"] - Or, add input validation (allowlist known commands/patterns) and document that
lmp_commandmust be trusted - Or, parse and validate
lmp_commandto ensure it only contains expected executable and known-safe options
🧰 Tools
🪛 Ruff (0.14.5)
126-126: subprocess call with shell=True identified, security issue
(S602)
for more information, see https://pre-commit.ci
…ction # Conflicts: # tests/test_compatibility_file.py
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_compatibility_file.py (2)
8-30: Minor readability nit in working-directory setup
os.path.join(__file__, "..", "lmp")works afterabspath/normalization, but it’s slightly non‑obvious because it treats the file path as a directory. Usingos.path.dirname(__file__)would make the intent clearer:- self.working_dir = os.path.abspath(os.path.join(__file__, "..", "lmp")) + self.working_dir = os.path.abspath( + os.path.join(os.path.dirname(__file__), "..", "lmp") + )
tearDowncleanup logic itself looks good.
53-75: Slightly improve path handling and consider brittleness of exact line matchesTwo small points here:
- For clarity and portability, prefer
os.path.joinover string concatenation when openinglmp.in:- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f:
- The
content_expectedvscontentcomparison enforces exact line matches (including whitespace), which is excellent for strict backward‑compatibility but will also break on harmless formatting tweaks. If the goal is primarily to validate semantics rather than formatting, you might consider more flexible checks (e.g., prefix/substring or regex) for some lines. If strict reproduction oflmp.inis required, then this is fine as‑is and this point can be ignored.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-143)
🪛 GitHub Actions: Pipeline
tests/test_compatibility_file.py
[error] 41-41: test_calc_md failed due to upstream ValueError: No resource_path found during test execution.
🪛 Ruff (0.14.5)
tests/test_compatibility_file.py
41-41: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tests/test_compatibility_file.py (1)
31-52: Fixcpcommand path causingCalledProcessErrorand markshell_outputas unusedCI shows both tests failing because the
cp ../static/compatibility_output/* .command exits non‑zero when run vialammps_file_interface_function(i.e., fromcwd=self.working_dir). This usually means thecompatibility_outputdirectory or its contents are not found at the expected relative location.To make this more robust and independent of the current working directory, construct an absolute path to
compatibility_outputfrom__file__, similar to how you handleresource_path, and reuse it in both tests:@@ - def test_calc_md_npt(self): - structure = bulk("Al", cubic=True).repeat([2, 2, 2]) - potential = "1999--Mishin-Y--Al--LAMMPS--ipr1" - units = "metal" - calc_kwargs = { + def test_calc_md_npt(self): + structure = bulk("Al", cubic=True).repeat([2, 2, 2]) + potential = "1999--Mishin-Y--Al--LAMMPS--ipr1" + units = "metal" + compatibility_output_dir = os.path.join( + os.path.dirname(__file__), "static", "compatibility_output" + ) + calc_kwargs = { @@ - shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function( @@ - units=units, - lmp_command="cp ../static/compatibility_output/* .", - resource_path=os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ), + units=units, + lmp_command=f"cp {compatibility_output_dir}/* .", + resource_path=os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + ), ) @@ - def test_calc_md_nvt(self): - structure = bulk("Al", cubic=True).repeat([2, 2, 2]) - potential = "1999--Mishin-Y--Al--LAMMPS--ipr1" - units = "metal" - calc_kwargs = {"temperature": 500.0, "n_ionic_steps": 1000, "n_print": 100} - shell_output, parsed_output, job_crashed = lammps_file_interface_function( + def test_calc_md_nvt(self): + structure = bulk("Al", cubic=True).repeat([2, 2, 2]) + potential = "1999--Mishin-Y--Al--LAMMPS--ipr1" + units = "metal" + compatibility_output_dir = os.path.join( + os.path.dirname(__file__), "static", "compatibility_output" + ) + calc_kwargs = {"temperature": 500.0, "n_ionic_steps": 1000, "n_print": 100} + _, parsed_output, job_crashed = lammps_file_interface_function( @@ - units=units, - lmp_command="cp ../static/compatibility_output/* .", - resource_path=os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ), + units=units, + lmp_command=f"cp {compatibility_output_dir}/* .", + resource_path=os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + ), )This should resolve the
CalledProcessErroras long astests/static/compatibility_outputis present in the repo, and it also addresses the Ruff warning by markingshell_outputas intentionally unused. The small cleanup onresource_pathremoves the redundant".."in front of an absolute path. (Based on relevant code inpyiron_lammps/compatibility/file.py.)Also applies to: 80-96
🧹 Nitpick comments (1)
tests/test_compatibility_file.py (1)
9-30: Tighten working directory construction (optional)
self.working_dir = os.path.abspath(os.path.join(__file__, "..", "lmp"))works, but it treats__file__as a directory component. It’s more idiomatic (and a bit clearer) to base this on the containing directory:- self.working_dir = os.path.abspath(os.path.join(__file__, "..", "lmp")) + self.working_dir = os.path.abspath( + os.path.join(os.path.dirname(__file__), "lmp") + )This also makes it easier to reason about relative paths like
../static/...used later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-143)
🪛 GitHub Actions: Pipeline
tests/test_compatibility_file.py
[error] 41-41: test_calc_md_npt failed due to subprocess.CalledProcessError from cp command.
[error] 85-85: test_calc_md_nvt failed due to subprocess.CalledProcessError from cp command.
🪛 Ruff (0.14.5)
tests/test_compatibility_file.py
41-41: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
85-85: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (1)
tests/test_compatibility_file.py (1)
56-79: Good coverage and expectations on generatedlmp.incontentThe assertions on
parsed_output["generic"]keys and the expectedlmp.inlines (units, boundary, pair_style, fixes, thermo, run, etc.) are precise and align with the intended behavior oflammps_file_interface_functionfor MD/NPT and MD/NVT modes. This is a solid end‑to‑end regression test for the new compatibility layer.Also applies to: 100-122
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
tests/test_compatibility_file.py (2)
54-54: Unusedshell_outputvariable in all test methodsStatic analysis correctly identifies that
shell_outputis unpacked but never used in any of the test methods. Replace with_to indicate it's intentionally unused.Apply this change to all eight occurrences:
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function((Apply to lines 54, 101, 143, 184, 225, 261, 298, and 337.)
Also applies to: 101-101, 143-143, 184-184, 225-225, 261-261, 298-298, 337-337
69-69: Non-portable path construction using string concatenationThe tests build file paths using string concatenation (e.g.,
self.working_dir + "/lmp.in"), which is not portable across operating systems. Useos.path.joininstead.Apply this change to all eight occurrences:
- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f:(Apply to lines 69, 116, 158, 199, 239, 275, 313, and 352.)
Also applies to: 116-116, 158-158, 199-199, 239-239, 275-275, 313-313, 352-352
pyiron_lammps/compatibility/calculate.py (6)
10-24:time_stepparameter is documented but not accepted bycalc_mdThe docstring (line 41) describes a
time_stepparameter in femtoseconds, and the file interface function's documentation mentions it as part ofcalc_kwargsfor "md" mode. However:
- The function signature (lines 10-24) does not include
time_step- The timestep is hard-coded to
0.001at line 188- Passing
time_stepviacalc_kwargswill raiseTypeError: calc_md() got an unexpected keyword argument 'time_step'Add
time_stepto the signature with a default value and apply unit conversion:def calc_md( temperature=None, pressure=None, n_print=100, + time_step=1.0, temperature_damping_timescale=100.0, pressure_damping_timescale=1000.0,Then replace the hard-coded timestep:
- line_lst.append("timestep 0.001") + line_lst.append(f"timestep {time_step * time_units}")Also applies to: 41-41, 188-188
92-107: Explicitinitial_temperatureis not unit-converted, causing mismatch in non-metal unitsAt line 102,
temperatureis multiplied bytemperature_units. However, when a user provides an explicitinitial_temperature(lines 105-106), it is never converted. Only the implicit default (2 * temperature[0]) uses the already-converted temperature value.When
temperature_units != 1(non-metal units), this creates a unit mismatch where explicitinitial_temperatureis in pyiron units but is interpreted as LAMMPS units.Apply unit conversion to explicit
initial_temperature:# Apply initial overheating (default uses the theorem of equipartition of energy between KE and PE) - if initial_temperature is None and temperature is not None: - initial_temperature = 2 * temperature[0] + if temperature is not None: + if initial_temperature is None: + initial_temperature = 2 * temperature[0] + else: + initial_temperature = float(initial_temperature) * temperature_units
113-122: NPT pressure calculation requiresrotation_matrix, but no guard preventsNoneAt line 120-122,
_pressure_to_lammpsis called withrotation_matrixwithout validating it first. Inside_pressure_to_lammps(line 400-401), the code doesnp.matrix.trace(rotation_matrix), which will raise a low-level NumPy exception ifrotation_matrixisNone.The
calc_minimizefunction already guards this case (lines 259-264). Apply the same pattern here.Add validation before the pressure conversion:
if pressure is not None: # NPT if temperature is None or temperature.min() <= 0: raise ValueError( "Target temperature for fix nvt/npt/nph cannot be 0 or negative" ) + + if rotation_matrix is None: + raise ValueError( + "No rotation matrix given while trying to convert pressure. " + "This is most likely due to no structure being defined." + ) force_skewed = False
218-224: Docstring documents deprecatede_tol/f_tolparameters that are not acceptedThe docstring (lines 222-223) mentions
e_tolandf_tolas deprecated aliases, but the function signature (lines 203-212) does not include them. Callers using these names will getTypeError: unexpected keyword argument.Either remove the parameters from the docstring, or add backwards compatibility via
**kwargs:def calc_minimize( structure: Atoms, ionic_energy_tolerance=0.0, ionic_force_tolerance=1e-4, ..., **kwargs ): if "e_tol" in kwargs: warnings.warn("e_tol is deprecated, use ionic_energy_tolerance", DeprecationWarning, stacklevel=2) ionic_energy_tolerance = kwargs.pop("e_tol") if "f_tol" in kwargs: warnings.warn("f_tol is deprecated, use ionic_force_tolerance", DeprecationWarning, stacklevel=2) ionic_force_tolerance = kwargs.pop("f_tol") if kwargs: raise TypeError(f"Unexpected keyword arguments: {tuple(kwargs)}")
324-354:initial_temperature=0does not skip velocity initialization as documentedThe
calc_mddocstring (lines 56-57) states: "If 0, the velocity field is not initialized (in which case the initial velocity given in structure will be used)."However,
_set_initial_velocityalways returns avelocity all create ...command regardless of the temperature value, overwriting any existing velocities.Special-case
temperature == 0to return an empty string:def _set_initial_velocity( temperature, seed=80996, ... ): + if temperature == 0: + return "" + arg = ""Then in
calc_md, only append the command if non-empty:- line_lst.append( - _set_initial_velocity( + vel_cmd = _set_initial_velocity( temperature=initial_temperature, ... - ) - ) + ) + if vel_cmd: + line_lst.append(vel_cmd)Also applies to: 56-57
357-420:_pressure_to_lammpslacksNonecheck forrotation_matrix, causing obscure errorsAt lines 400-401, the function does
np.matrix.trace(rotation_matrix)without first checking ifrotation_matrixisNone. When called withrotation_matrix=None, this produces a low-level NumPy exception instead of a clear error message.Add an explicit guard:
# If necessary, rotate the pressure tensor to the Lammps coordinate frame. # Isotropic, hydrostatic pressures are rotation invariant. + if rotation_matrix is None: + raise ValueError( + "rotation_matrix must be provided when converting a non-scalar pressure." + ) if not np.isclose( np.matrix.trace(rotation_matrix), 3 ) and not _is_isotropic_hydrostatic(pressure):
🧹 Nitpick comments (3)
pyiron_lammps/compatibility/calculate.py (3)
75-75: Missingstacklevel=2inwarnings.warncallsStatic analysis correctly flags that
warnings.warn()calls should includestacklevel=2so the warning points to the caller's code, not the library internals.Add
stacklevel=2to allwarnings.warn()calls:- warnings.warn( - "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`." - ) + warnings.warn( + "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`.", + stacklevel=2 + )Apply the same change to lines 84, 177, 245, 440, and 469.
Also applies to: 84-84, 177-177, 245-245, 440-440, 469-469
138-138: Unused variableforce_skewedcan be removedStatic analysis correctly identifies that
force_skewedis assigned at line 138 but never used.Remove the unused variable:
- if ii > 2: - force_skewed = True
277-277: Loop variableiiis unused in the loop bodyStatic analysis flags that the loop counter
iiat line 277 is never referenced. Rename it to_iior_to indicate it's intentionally unused.- for ii, (press, str_axis) in enumerate( + for _, (press, str_axis) in enumerate( zip(pressure, ["x", "y", "z", "xy", "xz", "yz"]) ):
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyiron_lammps/compatibility/calculate.py(1 hunks)tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pyiron_lammps/compatibility/calculate.py (1)
pyiron_lammps/structure.py (2)
UnfoldingPrism(38-191)is_skewed(433-450)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-145)
🪛 Ruff (0.14.5)
pyiron_lammps/compatibility/calculate.py
75-75: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
84-84: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
109-109: Avoid specifying long messages outside the exception class
(TRY003)
115-117: Avoid specifying long messages outside the exception class
(TRY003)
138-138: Local variable force_skewed is assigned to but never used
Remove assignment to unused variable force_skewed
(F841)
157-159: Avoid specifying long messages outside the exception class
(TRY003)
177-177: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
245-245: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
261-264: Avoid specifying long messages outside the exception class
(TRY003)
277-277: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
304-304: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
396-396: Avoid specifying long messages outside the exception class
(TRY003)
404-411: Avoid specifying long messages outside the exception class
(TRY003)
440-440: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
469-469: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
tests/test_compatibility_file.py
54-54: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
101-101: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
143-143: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
184-184: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
225-225: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
261-261: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
298-298: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
337-337: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (1)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
🔇 Additional comments (1)
tests/test_compatibility_file.py (1)
1-373: Tests look comprehensive and well-structuredThe test suite provides excellent coverage of the compatibility interface:
- Error handling for invalid
calc_mode- MD modes: NPT, NPT+Langevin, NVT, NVT+Langevin
- Static and minimize modes with and without pressure
- Validation of both parsed outputs and generated LAMMPS input files
The structure is clean, setup/teardown is appropriate, and assertions are thorough.
for more information, see https://pre-commit.ci
…on_lammps into LammpsStructureCompatibility
for more information, see https://pre-commit.ci
…on_lammps into LammpsStructureCompatibility
for more information, see https://pre-commit.ci
…to lammps_function_water
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyiron_lammps/structure.py (1)
403-411: Guard against empty structures when inferring coordinate dimensionality
dim = len(self._structure.positions[0])will raise anIndexErrorfor empty structures, whereas usingself._structure.positions.shape[1]works forlen(structure) == 0as well. The same pattern appears elsewhere.Consider switching to
positions.shape[1](and optionally aligning other callers) to keep empty-structure handling consistent:- for id_atom, (el, coord) in enumerate(zip(el_lst, coords)): - dim = len(self._structure.positions[0]) + for id_atom, (el, coord) in enumerate(zip(el_lst, coords)): + dim = self._structure.positions.shape[1] c = np.zeros(3) c[:dim] = coord
♻️ Duplicate comments (5)
pyiron_lammps/compatibility/file.py (2)
128-134: Shell execution oflmp_commandis potentially unsafe; consider tightening or documenting
subprocess.check_outputis invoked withshell=Trueand a user-providedlmp_commandstring. This is idiomatic in many HPC workflows, but iflmp_commandcan be influenced by untrusted input, it allows shell injection.Options:
- Prefer
shell=Falseand acceptlmp_commandas aList[str].- Or keep
shell=Truebut clearly document thatlmp_commandis executed through the shell and must be trusted (e.g., user/job-level only).Given this is a low-level compatibility entry point, explicit documentation may be enough if changing the calling convention is undesirable.
Check current Python `subprocess` documentation for best practices around `shell=True` vs. list arguments, especially for long-running HPC-style commands.
27-83: Docstring is out-of-sync with the actual API and supportedcalc_modesThe docstring for
lammps_file_interface_functionstill describes the much richerpyiron_atomistics.lammps.lammps.lammps_functioninterface:
- Documents many parameters that are not in the signature and never used here (
cutoff_radius,bonds_kwargs,server_kwargs, restart options,enable_h5md, executable_*`, etc.).- Mentions a
"vcsgc"mode and a very largecalc_kwargssurface for it, but the implementation only supports"static","md", and"minimize"and raises for anything else.- Contains a stray
job_name"without an opening quote.This will mislead callers of this compatibility entry point.
Either trim the docstring to match this slimmer API (and explicitly state that advanced features live in the upstream job API), or extend the function to accept and plumb through those arguments and implement
"vcsgc"if full drop-in compatibility is required.pyiron_lammps/compatibility/calculate.py (3)
96-112: Temperature handling inconsistencies:initial_temperatureunits and “no initialization” behaviourTwo related issues in the MD helper:
Explicit
initial_temperatureis not unit-converted
temperatureis scaled bytemperature_units.- The implicit default
initial_temperature = 2 * temperature[0]is therefore in LAMMPS units.- But an explicitly provided
initial_temperatureis passed straight through to_set_initial_velocitywith no conversion, so for non-"metal"units it is interpreted in the wrong unit system.A minimal fix:
Apply initial overheating (default uses the theorem of equipartition of energy between KE and PE)
- if initial_temperature is None and temperature is not None:
initial_temperature = 2 * temperature[0]
Apply initial overheating (default uses the theorem of equipartition of energy between KE and PE)
- if temperature is not None:
if initial_temperature is None:initial_temperature = 2 * temperature[0]else:initial_temperature = float(initial_temperature) * temperature_units
initial_temperature=0does not match the docstring promiseThe docstring says
initial_temperature = 0should “not initialize” velocities, but_set_initial_velocityalways returns avelocity all create ...command. To honour the docstring:def _set_initial_velocity( temperature, seed=80996, gaussian=False, append_value=False, zero_lin_momentum=True, zero_rot_momentum=True, ):@@
- arg = ""
- if temperature == 0:
return ""- arg = ""
@@
- return "velocity all create " + str(temperature) + " " + str(seed) + arg
- return "velocity all create " + str(temperature) + " " + str(seed) + arg
and in `calc_md`: ```diff - line_lst.append( - _set_initial_velocity( + vel_cmd = _set_initial_velocity( temperature=initial_temperature, seed=seed, gaussian=True, append_value=False, zero_lin_momentum=True, zero_rot_momentum=True, - ) - ) + ) + if vel_cmd: + line_lst.append(vel_cmd)Also applies to: 192-203, 329-359
118-127: Constant-pressure MD still assumes a validrotation_matrixand can fail with a low-level NumPy errorIn the NPT branch of
calc_md:pressure = _pressure_to_lammps( pressure=pressure, rotation_matrix=rotation_matrix, units=units )and
_pressure_to_lammpsunconditionally does:if not np.isclose(np.matrix.trace(rotation_matrix), 3) and ...: ...If
pressureis specified as a tensor/list androtation_matrixisNone(the default), this produces an opaque NumPy error instead of a clear user-facing message.You already guard this case in
calc_minimizevia_get_rotation_matrix. For MD, mirror that behaviour or introduce arotation_matrix is Nonecheck in_pressure_to_lammps:-def _pressure_to_lammps(pressure, rotation_matrix, units="metal"): +def _pressure_to_lammps(pressure, rotation_matrix, units="metal"): @@ - # If necessary, rotate the pressure tensor to the Lammps coordinate frame. - # Isotropic, hydrostatic pressures are rotation invariant. - if not np.isclose( - np.matrix.trace(rotation_matrix), 3 - ) and not _is_isotropic_hydrostatic(pressure): + # If necessary, rotate the pressure tensor to the Lammps coordinate frame. + # Isotropic, hydrostatic pressures are rotation invariant. + if rotation_matrix is None: + raise ValueError( + "rotation_matrix must be provided when converting a non-isotropic pressure tensor." + ) + if not np.isclose(np.matrix.trace(rotation_matrix), 3) and not _is_isotropic_hydrostatic(pressure):and in
calc_md, either:
- Require callers to pass a valid rotation matrix when
pressureis a tensor, or- Derive it from a structure (similar to
calc_minimize) if you have one available in the calling context.Also applies to: 362-425
208-243:calc_minimizedocstring documents deprecatede_tol/f_tolthat are not acceptedThe docstring still mentions
e_tolandf_tolas deprecated aliases, but the function signature does not accept them. Any caller using these names will now see aTypeError, contrary to the docstring.Either:
- Remove
e_tol/f_tolfrom the docstring, or- Add a small compatibility layer via
**kwargs:-def calc_minimize( - structure: Atoms, - ionic_energy_tolerance=0.0, - ionic_force_tolerance=1e-4, - max_iter=100000, - pressure=None, - n_print=100, - style="cg", - rotation_matrix=None, - units="metal", -): +def calc_minimize( + structure: Atoms, + ionic_energy_tolerance=0.0, + ionic_force_tolerance=1e-4, + max_iter=100000, + pressure=None, + n_print=100, + style="cg", + rotation_matrix=None, + units="metal", + **kwargs, +): @@ - max_evaluations = 100 * max_iter + if "e_tol" in kwargs: + ionic_energy_tolerance = kwargs.pop("e_tol") + if "f_tol" in kwargs: + ionic_force_tolerance = kwargs.pop("f_tol") + if kwargs: + raise TypeError(f"Unexpected keyword arguments: {tuple(kwargs)}") + + max_evaluations = 100 * max_iterso the deprecation note matches actual behaviour.
🧹 Nitpick comments (2)
pyiron_lammps/compatibility/structure.py (1)
127-133: Minor: unused loop index in bond constructionThe loop
for i_shell, ib_shell_lst in enumerate(b_lst): for ib in np.unique(ib_shell_lst): ...never uses
i_shell. Renaming it to_clarifies intent and quiets linters:- for ia, i_bonds in enumerate(bonds_lst): + for ia, i_bonds in enumerate(bonds_lst): @@ - for i_shell, ib_shell_lst in enumerate(b_lst): + for _shell_idx, ib_shell_lst in enumerate(b_lst):pyiron_lammps/compatibility/file.py (1)
116-118: Minor: avoid ambiguous single-letter loop variable when writing input fileThe list comprehension:
f.writelines([l + "\n" for l in lmp_str_lst])works but uses the ambiguous name
l. Renaming it improves readability and avoids E741:- f.writelines([l + "\n" for l in lmp_str_lst]) + f.writelines([line + "\n" for line in lmp_str_lst])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
pyiron_lammps/compatibility/calculate.py(1 hunks)pyiron_lammps/compatibility/file.py(1 hunks)pyiron_lammps/compatibility/structure.py(1 hunks)pyiron_lammps/structure.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pyiron_lammps/compatibility/file.py (4)
pyiron_lammps/compatibility/calculate.py (3)
calc_md(10-205)calc_minimize(208-305)calc_static(308-309)pyiron_lammps/output.py (1)
parse_lammps_output(73-135)pyiron_lammps/potential.py (1)
get_potential_by_name(349-355)pyiron_lammps/structure.py (1)
write_lammps_datafile(501-513)
pyiron_lammps/compatibility/calculate.py (1)
pyiron_lammps/structure.py (2)
UnfoldingPrism(38-191)is_skewed(481-498)
pyiron_lammps/compatibility/structure.py (1)
pyiron_lammps/structure.py (12)
structure(228-234)structure(237-251)LammpsStructure(194-478)_get_velocities_input_string(253-275)get_lammps_id_dict(299-302)el_eam_lst(278-284)el_eam_lst(287-296)rotate_positions(433-447)lammps_header(305-335)simulation_cell(337-358)potential(220-221)potential(224-225)
🪛 Ruff (0.14.5)
pyiron_lammps/compatibility/file.py
112-114: Avoid specifying long messages outside the exception class
(TRY003)
117-117: Ambiguous variable name: l
(E741)
128-128: subprocess call with shell=True identified, security issue
(S602)
pyiron_lammps/compatibility/calculate.py
80-80: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
89-89: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
114-114: Avoid specifying long messages outside the exception class
(TRY003)
120-122: Avoid specifying long messages outside the exception class
(TRY003)
143-143: Local variable force_skewed is assigned to but never used
Remove assignment to unused variable force_skewed
(F841)
162-164: Avoid specifying long messages outside the exception class
(TRY003)
182-182: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
250-250: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
266-269: Avoid specifying long messages outside the exception class
(TRY003)
282-282: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
309-309: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
401-401: Avoid specifying long messages outside the exception class
(TRY003)
409-416: Avoid specifying long messages outside the exception class
(TRY003)
445-445: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
474-474: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
pyiron_lammps/compatibility/structure.py
100-100: Avoid specifying long messages outside the exception class
(TRY003)
131-131: Loop control variable i_shell not used within loop body
Rename unused i_shell to _i_shell
(B007)
⏰ 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). (1)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
🔇 Additional comments (6)
pyiron_lammps/structure.py (1)
501-512:atom_typeplumbing intowrite_lammps_datafilelooks correctThe new
atom_typeparameter is cleanly added with a default, passed through toLammpsStructure, and keeps existing call sites working unchanged. This aligns with the compatibility layer expecting different atom styles.pyiron_lammps/compatibility/structure.py (1)
310-336:get_bondswrapper looks consistent withstructuretoolkitneighbor APIThe
get_bondshelper simply configuresget_neighbors(..., mode="ragged")and forwards toneighbors.get_bonds, exposing radius/max_shells/prec in a thin wrapper. This is a reasonable compatibility layer aroundstructuretoolkitand fits the rest of the module.pyiron_lammps/compatibility/file.py (1)
148-156: Initialization helper is simple and aligns with LAMMPS expectationsAside from the
unitsforwarding issue mentioned earlier,lammps_file_initializationcleanly derives periodic vs fixed boundaries fromstructure.pbcand emits a minimal, predictable header (units,dimension,boundary,atom_style atomic,read_data). This is a reasonable compatibility shim.pyiron_lammps/compatibility/calculate.py (3)
248-304: Minimization command assembly and thermo setup look sane
calc_minimizenow:
- Validates
unitsand converts tolerances usingLAMMPS_UNIT_CONVERSIONS.- Builds a flat
line_lstwithvariable thermotime, thermo setup via_get_thermo(), optionalfix ensemble all box/relax, followed bymin_styleandminimize ...line.- Returns
(line_lst, structure)for the caller to update its structure reference.This is consistent and avoids the nested-list bug from earlier iterations.
308-317: Static calculation helper is minimal and consistent with MD/minimize thermo handling
calc_staticreuses_get_thermoand adds a single"run 0"line, which is exactly what you want for a “static” evaluation. No issues here.
320-327: Hydrostatic/isotropic pressure helper behaves as expected
_is_isotropic_hydrostaticcorrectly checks:
- Axial components all equal and non-
None.- Shear components all
Noneor all zero.This is a good encapsulation for deciding whether a pressure tensor can be used without rotation. No changes needed.
| if calc_kwargs is None: | ||
| calc_kwargs = {} | ||
|
|
||
| os.makedirs(working_directory, exist_ok=True) | ||
| potential_dataframe = get_potential_by_name( | ||
| potential_name=potential, resource_path=resource_path | ||
| ) | ||
| lmp_str_lst = lammps_file_initialization(structure=structure) | ||
| lmp_str_lst += potential_dataframe["Config"] | ||
| lmp_str_lst += ["variable dumptime equal {} ".format(calc_kwargs.get("n_print", 1))] | ||
| lmp_str_lst += [ | ||
| "dump 1 all custom ${dumptime} dump.out id type xsu ysu zsu fx fy fz vx vy vz", | ||
| 'dump_modify 1 sort id format line "%d %d %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g %20.15g"', | ||
| ] | ||
|
|
||
| if calc_mode == "static": | ||
| lmp_str_lst += calc_static() | ||
| elif calc_mode == "md": | ||
| if "n_ionic_steps" in calc_kwargs.keys(): | ||
| n_ionic_steps = int(calc_kwargs.pop("n_ionic_steps")) | ||
| else: | ||
| n_ionic_steps = 1 | ||
| lmp_str_lst += calc_md(**calc_kwargs) | ||
| lmp_str_lst += ["run {} ".format(n_ionic_steps)] |
There was a problem hiding this comment.
units argument is not consistently respected (input script always uses "metal")
The units parameter is correctly forwarded to:
write_lammps_datafile(..., units=units, ...)parse_lammps_output(..., units=units, ...)
but the actual LAMMPS input and MD/minimization helpers still hard-code or default to "metal":
lammps_file_initialization(structure=structure)uses its own defaultunits="metal".calc_mdandcalc_minimizeare called without aunitsargument, so they default to"metal"again.
For any call with units != "metal", the datafile and parsing will assume one unit system while the LAMMPS run itself is configured with another, which is a serious physical inconsistency.
Recommend:
- lmp_str_lst = lammps_file_initialization(structure=structure)
+ lmp_str_lst = lammps_file_initialization(structure=structure, units=units)
@@
- if calc_mode == "static":
- lmp_str_lst += calc_static()
- elif calc_mode == "md":
+ if calc_mode == "static":
+ lmp_str_lst += calc_static()
+ elif calc_mode == "md":
@@
- lmp_str_lst += calc_md(**calc_kwargs)
+ lmp_str_lst += calc_md(units=units, **calc_kwargs)
@@
- elif calc_mode == "minimize":
- lmp_str_tmp_lst, structure = calc_minimize(structure=structure, **calc_kwargs)
+ elif calc_mode == "minimize":
+ lmp_str_tmp_lst, structure = calc_minimize(
+ structure=structure, units=units, **calc_kwargs
+ )and keep lammps_file_initialization as the single source of truth for the units card.
Also applies to: 119-126, 148-156
🤖 Prompt for AI Agents
pyiron_lammps/compatibility/file.py lines ~84-107 (also apply same fixes to
~119-126 and ~148-156): the LAMMPS input currently hardcodes "metal" via
lammps_file_initialization and omitted units when calling calc_md/calc_minimize,
causing mismatch with write_lammps_datafile and parse_lammps_output; fix by
passing the local units variable through to lammps_file_initialization (e.g.,
lammps_file_initialization(structure=structure, units=units)) and also pass
units into calc_md and calc_minimize calls (e.g., calc_md(..., units=units) /
calc_minimize(..., units=units)), and remove or override any internal defaults
so the units card in the generated input script is always derived from the
single units parameter used elsewhere.
| class LammpsStructureCompatibility(LammpsStructure): | ||
| def __init__( | ||
| self, | ||
| bond_dict: Optional[Dict] = None, | ||
| units: str = "metal", | ||
| atom_type: str = "atomic", | ||
| ): | ||
| super().__init__(bond_dict=bond_dict, units=units, atom_type=atom_type) | ||
| self._molecule_ids = [] | ||
|
|
There was a problem hiding this comment.
atom_type argument is effectively ignored by the compatibility structure
LammpsStructureCompatibility.__init__ forwards atom_type only to the base class, which stores it in _atom_type, but your overridden structure setter branches on self.atom_type (initially None). As a result, LammpsStructureCompatibility(atom_type="full") (or "bond"/"charge") will still execute the "atomic" branch unless the caller later mutates self.atom_type manually.
To make atom_type behave as expected for this compatibility entry point, either:
- Explicitly mirror the argument into
self.atom_typein__init__, or - Change the setter to branch on
self._atom_typeinstead ofself.atom_type.
For example:
class LammpsStructureCompatibility(LammpsStructure):
def __init__(
self,
bond_dict: Optional[Dict] = None,
units: str = "metal",
atom_type: str = "atomic",
):
- super().__init__(bond_dict=bond_dict, units=units, atom_type=atom_type)
- self._molecule_ids = []
+ super().__init__(bond_dict=bond_dict, units=units, atom_type=atom_type)
+ self.atom_type = atom_type
+ self._molecule_ids = []
@@
- if self.atom_type == "full":
+ if self.atom_type == "full":
input_str = self.structure_full()Also applies to: 41-51
🤖 Prompt for AI Agents
In pyiron_lammps/compatibility/structure.py around lines 12-21 and 41-51, the
constructor forwards atom_type to the base but does not initialize the property
used by the overridden structure setter, so passing atom_type has no effect;
after the super().__init__(...) call assign the incoming atom_type to the
instance property used by the setter (e.g., self.atom_type = atom_type) so the
setter branches correctly, or alternatively change the setter to read
self._atom_type instead—implement the former by setting the property in both
constructors present in those line ranges.
| def structure_bond(self): | ||
| """ | ||
|
|
||
| Returns: | ||
|
|
||
| """ | ||
| species_lammps_id_dict = self.get_lammps_id_dict(self.el_eam_lst) | ||
| self.molecule_ids = None | ||
| # analyze structure to get molecule_ids, bonds, angles etc | ||
| coords = self.rotate_positions(self._structure) | ||
|
|
||
| elements = self._structure.get_chemical_symbols() | ||
|
|
||
| ## Standard atoms stuff | ||
| atoms = "Atoms \n\n" | ||
| # atom_style bond | ||
| # format: atom-ID, molecule-ID, atom_type, x, y, z | ||
| format_str = "{0:d} {1:d} {2:d} {3:f} {4:f} {5:f} " | ||
| if len(self._structure.positions[0]) == 3: | ||
| for id_atom, (x, y, z) in enumerate(coords): | ||
| id_mol = self.molecule_ids[id_atom] | ||
| atoms += ( | ||
| format_str.format( | ||
| id_atom + 1, | ||
| id_mol, | ||
| species_lammps_id_dict[elements[id_atom]], | ||
| x, | ||
| y, | ||
| z, | ||
| ) | ||
| + "\n" | ||
| ) | ||
| elif len(self._structure.positions[0]) == 2: | ||
| for id_atom, (x, y) in enumerate(coords): | ||
| id_mol = self.molecule_ids[id_atom] | ||
| atoms += ( | ||
| format_str.format( | ||
| id_atom + 1, | ||
| id_mol, | ||
| species_lammps_id_dict[elements[id_atom]], | ||
| x, | ||
| y, | ||
| 0.0, | ||
| ) | ||
| + "\n" | ||
| ) | ||
| else: |
There was a problem hiding this comment.
molecule_ids is never initialized and will currently crash structure_bond/structure_full
Both structure_bond and structure_full index self.molecule_ids[id_atom], but:
- There is no
@propertyformolecule_ids. __init__only defines_molecule_ids = [].structure_bond/structure_fullstart withself.molecule_ids = None, which simply sets an attribute toNone.
This leads to TypeError: 'NoneType' object is not subscriptable as soon as you hit the for id_atom loops.
You probably want a lazily populated molecule ID array. A minimal fix is to add a proper property that uses _molecule_ids as backing storage and interprets None as "reset to default sequential IDs":
class LammpsStructureCompatibility(LammpsStructure):
@@
- super().__init__(bond_dict=bond_dict, units=units, atom_type=atom_type)
- self._molecule_ids = []
+ super().__init__(bond_dict=bond_dict, units=units, atom_type=atom_type)
+ self._molecule_ids = None
+
+ @property
+ def molecule_ids(self):
+ if self._molecule_ids is None:
+ if self._structure is None:
+ raise ValueError("Structure not set; cannot infer molecule IDs")
+ # Default: each atom is its own molecule
+ self._molecule_ids = list(range(1, len(self._structure) + 1))
+ return self._molecule_ids
+
+ @molecule_ids.setter
+ def molecule_ids(self, value):
+ # `None` means "reset" and lazily re-infer on next access
+ self._molecule_ids = None if value is None else list(value)
@@ def structure_bond(self):
- self.molecule_ids = None
+ self.molecule_ids = None
@@ def structure_full(self):
- self.molecule_ids = None
+ self.molecule_ids = NoneAlso applies to: 168-175
| if len(self._bond_dict.keys()) > 0: | ||
| cutoff_list = list() | ||
| for val in self._bond_dict.values(): | ||
| cutoff_list.append(np.max(val["cutoff_list"])) | ||
| max_cutoff = np.max(cutoff_list) | ||
| # Calculate neighbors only once |
There was a problem hiding this comment.
Handle bond_dict=None in structure_full to avoid AttributeError
structure_full assumes self._bond_dict is a mapping:
if len(self._bond_dict.keys()) > 0:
...but LammpsStructureCompatibility inherits _bond_dict from LammpsStructure, where it defaults to None. Calling structure_full with the default bond_dict will therefore raise AttributeError: 'NoneType' object has no attribute 'keys'.
If None means "no bonds defined", a simple and robust guard is:
- if len(self._bond_dict.keys()) > 0:
+ if self._bond_dict and len(self._bond_dict.keys()) > 0:so the rest of the code runs with zero bonds/angles and the header correctly reports zero types.
🤖 Prompt for AI Agents
In pyiron_lammps/compatibility/structure.py around lines 181 to 186, the code
assumes self._bond_dict is a mapping and calls self._bond_dict.keys(), which
raises AttributeError when _bond_dict is None; treat None as "no bonds" by using
a safe local mapping (e.g. bond_dict = self._bond_dict or {}) or check "if
self._bond_dict:" before accessing keys, then only build cutoff_list and compute
max_cutoff when the mapping is non-empty and otherwise set max_cutoff (and
derived neighbor logic) to a sensible default (e.g. 0) so the rest of the
function proceeds and headers report zero bond/angle types.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
tests/test_compatibility_file.py (2)
55-381: Mark unusedshell_outputand useos.path.joinforlmp.inpathsAcross the MD/static/minimize tests:
- The first element of the returned tuple (
shell_output) is never used.lmp.inis opened via string concatenation (self.working_dir + "/lmp.in"), which is less portable and is flagged in earlier reviews.To clean this up and satisfy Ruff:
- Mark
shell_outputas intentionally unused in all tests:- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function(Apply this at lines 62, 109, 151, 192, 233, 269, 306, and 345.
- Use
os.path.joinwhen openinglmp.inin all tests:- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f:Apply this at lines 77, 124, 167, 207, 247, 283, 321, and 360.
These changes keep the tests behaviour identical while improving clarity and portability.
37-54:test_calc_errorstill fails due to missingresource_pathBoth calls to
lammps_file_interface_functionintest_calc_errorare made withoutresource_path, soget_potential_by_nameraisesValueError("No resource_path found")before you hit the intended error branches. The secondassertRaises(NotImplementedError)therefore fails (pipeline error).Pass the same
resource_pathyou use elsewhere in this file:def test_calc_error(self): with self.assertRaises(ValueError): lammps_file_interface_function( working_directory=self.working_dir, structure=self.structure, potential=self.potential, calc_mode="error", - units=self.units, + units=self.units, + resource_path=os.path.join(self.static_path, "potential"), ) with self.assertRaises(NotImplementedError): lammps_file_interface_function( working_directory=self.working_dir, structure=self.structure, potential=self.potential, units="error", calc_mode="md", + resource_path=os.path.join(self.static_path, "potential"), )This keeps the tests focused on mode/units error handling instead of environment configuration.
pyiron_lammps/compatibility/file.py (3)
27-83: Docstring still documents unsupported API (extra args,vcsgc, typo)The docstring still describes the much richer
pyiron_atomistics.lammps.lammps.lammps_functionAPI: it listsvcsgcmode and many parameters (cutoff_radius,bonds_kwargs,server_kwargs, restart/executable options, etc.) that are neither in the signature nor handled in this implementation, and it contains a typo aroundjob_name("delta_press", job_name", "rotation_matrix").This is misleading for users of this function.
Either:
- Slim the docstring down to match the actual signature and the three supported modes (
static,md,minimize), and fix thejob_nametypo, or- Extend the function to accept and honour the documented parameters and
vcsgcif full drop‑in compatibility is required.
91-96:unitsparameter is ignored bylammps_file_initialization(simulation always usesmetal)You correctly pass
unitsintocalc_md,calc_minimize,write_lammps_datafile, andparse_lammps_output, but the LAMMPS input header is still hard‑coded to"units metal"via the default inlammps_file_initialization. This desynchronises the actual LAMMPS run from the unit system assumed by the helpers and output parsing whenunits != "metal".Forward
unitsinto the initializer:- lmp_str_lst = lammps_file_initialization(structure=structure) + lmp_str_lst = lammps_file_initialization(structure=structure, units=units)This keeps the input script, MD/minimize helpers, datafile writer, and output parser all on the same unit system.
Also applies to: 150-159
130-136:subprocess.check_output(..., shell=True)with user‑suppliedlmp_command
lmp_commandis a public parameter and is passed straight tosubprocess.check_outputwithshell=True, so any unsanitised value is executed by the shell. This is a classic S602‑style shell‑injection vector iflmp_commandis ever influenced by untrusted input.Safer options:
- Prefer
shell=Falseand acceptlmp_commandas a list of arguments, e.g.["mpiexec", "-n", "1", "--oversubscribe", "lmp_mpi", "-in", "lmp.in"], or- If you must keep
shell=Truefor HPC convenience, clearly document thatlmp_commandmust be trusted and not derived from user input, or add validation/allow‑listing around it.
🧹 Nitpick comments (2)
pyiron_lammps/compatibility/file.py (2)
84-107: Avoid mutating caller’scalc_kwargsin‑placeFor
"md"/"minimize"youpop("n_ionic_steps")and inject"units"directly intocalc_kwargs. If the caller reuses the same dict across calls, behaviour depends on the first call.Consider copying the dict once up front:
- if calc_kwargs is None: - calc_kwargs = {} + if calc_kwargs is None: + calc_kwargs = {} + else: + # Work on a shallow copy to avoid surprising side effects for callers + calc_kwargs = dict(calc_kwargs)The rest of the logic (pop,
"units"injection) can stay unchanged.
118-119: Rename loop variablelinwritelinesfor clarity and Ruff E741Ruff flags the ambiguous name
lhere. Renaming improves readability and silences E741 without changing behaviour:- with open(os.path.join(working_directory, "lmp.in"), "w") as f: - f.writelines([l + "\n" for l in lmp_str_lst]) + with open(os.path.join(working_directory, "lmp.in"), "w") as f: + f.writelines([line + "\n" for line in lmp_str_lst])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyiron_lammps/compatibility/file.py(1 hunks)tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pyiron_lammps/compatibility/file.py (4)
pyiron_lammps/compatibility/calculate.py (3)
calc_md(10-205)calc_minimize(208-305)calc_static(308-309)pyiron_lammps/output.py (1)
parse_lammps_output(73-135)pyiron_lammps/potential.py (1)
get_potential_by_name(349-355)pyiron_lammps/structure.py (1)
write_lammps_datafile(501-513)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-147)
🪛 GitHub Actions: Pipeline
pyiron_lammps/compatibility/file.py
[error] 88-88: lammps_file_interface_function raised ValueError: No resource_path found. Potential installation guide provided in stack trace.
tests/test_compatibility_file.py
[error] 47-47: Unit test failure during 'python -m unittest discover tests': ValueError: No resource_path found (see stack in pyiron_lammps/potential.py and compatibility/file.py).
🪛 Ruff (0.14.6)
pyiron_lammps/compatibility/file.py
114-116: Avoid specifying long messages outside the exception class
(TRY003)
119-119: Ambiguous variable name: l
(E741)
130-130: subprocess call with shell=True identified, security issue
(S602)
tests/test_compatibility_file.py
62-62: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
109-109: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
151-151: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
192-192: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
233-233: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
269-269: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
306-306: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
345-345: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
tests/test_compatibility_file.py (2)
63-74: Mark unusedshell_outputas intentionally ignored (Ruff RUF059)In all these tests,
shell_outputis unpacked but never used, which triggers Ruff’s RUF059 warning. Since you only assert onparsed_outputandjob_crashed, it’s clearer to mark the first element as intentionally ignored.For example, in
test_calc_md_npt:- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function( working_directory=self.working_dir, structure=self.structure, potential=self.potential, calc_mode="md", calc_kwargs=calc_kwargs, units=self.units, lmp_command="cp " + str(os.path.join(self.static_path, "compatibility_output")) + "/* .", resource_path=os.path.join(self.static_path, "potential"), )Apply the same pattern to the other calls at Lines 110, 152, 193, 234, 270, 308, and 347.
Also applies to: 110-121, 152-163, 193-204, 234-244, 270-281, 308-319, 347-358
78-80: Useos.path.joininstead of string concatenation for file pathsAll these blocks open
"lmp.in"via string concatenation (self.working_dir + "/lmp.in"). Usingos.path.joinis more idiomatic and portable.For example, in
test_calc_md_npt:- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f: content = f.readlines()Apply the same change in the other tests that read
lmp.in.Also applies to: 125-127, 168-169, 209-210, 248-249, 285-287, 323-325, 363-364
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-147)
🪛 GitHub Actions: Pipeline
tests/test_compatibility_file.py
[error] 305-305: Test failed: expected 'variable dumptime equal 1' not found in output; actual content contains 'variable dumptime equal 100'.
🪛 Ruff (0.14.6)
tests/test_compatibility_file.py
63-63: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
110-110: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
152-152: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
193-193: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
234-234: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
270-270: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
308-308: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (1)
tests/test_compatibility_file.py (1)
8-55: Good test scaffolding and error-path coverage
setUp/tearDowncorrectly manage a per-test working directory, andtest_calc_errorexercises both invalidcalc_modeand invalidunits, which is useful regression coverage for the new compatibility layer. No changes needed here.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
tests/test_compatibility_file.py (2)
63-64: Mark unusedshell_outputas intentionally ignored to satisfy linters
shell_outputis unpacked but never used in all these tests, and Ruff flags this (RUF059). Replace it with_(or_shell_output) to make the intent explicit:- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function(Apply the same change to each call at Lines 63, 110, 152, 193, 234, 270, 308, and 347.
Also applies to: 110-110, 152-152, 193-193, 234-234, 270-270, 308-308, 347-347
78-79: Useos.path.joininstead of string concatenation forlmp.inpathThe tests open
self.working_dir + "/lmp.in"in several places. This works on POSIX, but usingos.path.joinis more idiomatic and portable:- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f:Since
osis already imported, you can apply this at all eight usages (Lines 78, 125, 167, 208, 248, 285, 323, 362).Also applies to: 125-126, 167-168, 208-209, 248-249, 285-286, 323-324, 362-363
🧹 Nitpick comments (2)
tests/test_compatibility_file.py (2)
9-35: Improve test isolation and simplify path construction insetUpUsing
__file__directly inos.path.join(__file__, "..", "lmp")and mixing".."withos.path.dirname(__file__)forstatic_pathworks but is a bit indirect and ties all tests to a sharedtests/lmpdirectory.You could make this more robust and parallel‑test friendly by switching to a per‑run temp dir and a simpler static path, e.g.:
-import os +import os +import tempfile @@ - self.working_dir = os.path.abspath(os.path.join(__file__, "..", "lmp")) - self.static_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static") - ) + self.working_dir = tempfile.mkdtemp( + prefix="lmp_", dir=os.path.dirname(__file__) + ) + self.static_path = os.path.join(os.path.dirname(__file__), "static")
tearDownalready removesself.working_dir, so this drops in cleanly.
37-55: Optionally assert error messages (or avoid dependency on real LAMMPS) intest_calc_errorThe two branches correctly exercise invalid
calc_modeand unsupportedunits. If you want these to guard against regressions more tightly, consider:
- Using
assertRaisesRegex(or inspectingstr(cm.exception)) to ensure you’re getting the intended failure mode instead of some unrelatedValueError/NotImplementedError.- For the
units="error"case, optionally overridinglmp_command(like in the other tests) so this test never depends on the presence of a real LAMMPS executable.Not mandatory, but it makes the contract clearer and the test more self‑contained.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-147)
🪛 Ruff (0.14.6)
tests/test_compatibility_file.py
63-63: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
110-110: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
152-152: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
193-193: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
234-234: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
270-270: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
308-308: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
347-347: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (2)
tests/test_compatibility_file.py (2)
56-190: MD compatibility tests are comprehensive and consistent with the interfaceThe NPT/NVT (with and without Langevin) tests:
- Correctly wire
calc_kwargs(temperature, optionalpressure,n_ionic_steps,n_print,langevin) throughlammps_file_interface_function.- Rely on a copied static output instead of requiring a live LAMMPS run, which keeps tests fast and deterministic.
- Validate both the parsed
generickeys and key LAMMPS script lines (dumptimetied ton_print,runtied ton_ionic_stepsor default 1, appropriatefix ensemble/ Langevin configurations).This looks well aligned with the intended backwards‑compatible MD behavior.
233-383: Static and minimize tests correctly pin key LAMMPS script semanticsThe
staticandminimize*tests:
- Check parsed
genericoutput consistently againstself.keys.- Pin important parts of the generated input: default vs explicit
dumptime,thermotimebehavior (e.g., driven bymax_iterintest_calc_minimizevs default in pressure relax cases), and the expectedbox/relaxvariants for scalar vs 3‑componentpressure.- Reflect the updated expectation that minimize respects
n_printfordumptime(Line 294), which matches the currentlammps_file_interface_functionlogic.These tests give good coverage of the new minimize/static compatibility paths.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_compatibility_file.py (2)
64-64: Replace unused variable and useos.path.joinfor path construction.Two portability and code quality improvements:
- The
shell_outputvariable is unpacked but never used. Replace it with_to indicate it's intentionally ignored.- Line 79 uses string concatenation for path construction, which is not portable across operating systems.
Apply this diff:
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function( working_directory=self.working_dir, structure=self.structure, potential=self.potential, calc_mode="md", calc_kwargs=calc_kwargs, units=self.units, lmp_command="cp " + str(os.path.join(self.static_path, "compatibility_output")) + "/* .", resource_path=os.path.join(self.static_path, "potential"), ) self.assertFalse(job_crashed) for key in self.keys: self.assertIn(key, parsed_output["generic"]) - with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f: content = f.readlines()Apply the same changes to the other test methods at lines 111, 126, 153, 168, 194, 209, 235, 249, 271, 286, 309, 324, 348, and 363.
Also applies to: 79-79
57-384: Consider reducing test code duplication.All eight test methods follow a nearly identical pattern:
- Set up calc_kwargs
- Call lammps_file_interface_function
- Assert job_crashed is False
- Validate keys in parsed_output
- Read and validate lmp.in content
While each test validates different configurations, the structure duplication makes maintenance harder. Consider extracting a helper method for the common validation pattern.
For example:
def _run_and_validate_lammps(self, calc_mode, calc_kwargs, expected_lines): """Helper to run LAMMPS and validate output.""" _, parsed_output, job_crashed = lammps_file_interface_function( working_directory=self.working_dir, structure=self.structure, potential=self.potential, calc_mode=calc_mode, calc_kwargs=calc_kwargs, units=self.units, lmp_command="cp " + str(os.path.join(self.static_path, "compatibility_output")) + "/* .", resource_path=os.path.join(self.static_path, "potential"), ) self.assertFalse(job_crashed) for key in self.keys: self.assertIn(key, parsed_output["generic"]) with open(os.path.join(self.working_dir, "lmp.in"), "r") as f: content = f.readlines() for line in expected_lines: self.assertIn(line, content)Then each test becomes more concise:
def test_calc_md_npt(self): calc_kwargs = {"temperature": 500.0, "pressure": 0.0, "n_ionic_steps": 1000, "n_print": 100} expected_lines = [ "units metal\n", "fix ensemble all npt temp 500.0 500.0 0.1 iso 0.0 0.0 1.0\n", # ... other expected lines ] self._run_and_validate_lammps("md", calc_kwargs, expected_lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-147)
🪛 Ruff (0.14.6)
tests/test_compatibility_file.py
64-64: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
111-111: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
153-153: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
194-194: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
235-235: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
271-271: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
309-309: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
348-348: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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). (1)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
🔇 Additional comments (2)
tests/test_compatibility_file.py (2)
1-35: LGTM!The test class setup and teardown are well-structured. The fixture initialization in
setUpproperly prepares common test data, andtearDownensures proper cleanup.
37-55: LGTM!The error handling tests properly validate that invalid
calc_modeandunitsvalues raise appropriate exceptions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_compatibility_file.py (1)
74-74: Replace unusedshell_outputwith underscore.The
shell_outputreturn value is unpacked but never used in any test method. While a past review comment indicated this was addressed, the issue persists and is flagged by Ruff. Since the shell output is not needed for test assertions, mark it as intentionally unused.Apply this pattern to all nine affected test methods:
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function(This applies to test methods:
test_calc_md_npt,test_calc_md_npt_langevin,test_calc_md_nvt,test_calc_md_nvt_langevin,test_calc_md_nve,test_calc_static,test_calc_minimize,test_calc_minimize_pressure, andtest_calc_minimize_pressure_3d.Also applies to: 121-121, 163-163, 204-204, 246-246, 285-285, 321-321, 359-359, 398-398
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-147)
🪛 Ruff (0.14.6)
tests/test_compatibility_file.py
74-74: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
121-121: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
163-163: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
204-204: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
246-246: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
285-285: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
321-321: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
359-359: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
398-398: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (4)
tests/test_compatibility_file.py (4)
1-31: LGTM! Well-structured test setup.The imports, class structure, and setUp method are well-organized. The comprehensive
keyslist for validating parsed output is good practice for ensuring consistent test coverage across all calculation modes.
33-35: LGTM! Proper test cleanup.The tearDown method correctly removes the test working directory, ensuring a clean state between test runs.
37-65: LGTM! Comprehensive error handling tests.The error tests cover invalid
calc_mode, invalidunits, and invalidcalc_kwargs, ensuring the interface properly validates inputs and raises appropriate exceptions.
67-434: Excellent test coverage across calculation modes.The test suite comprehensively covers error handling, MD variants (NPT, NPT with Langevin, NVT, NVT with Langevin, NVE), static calculations, and multiple minimize configurations. Using static output files via the
cpcommand is a pragmatic approach that allows testing the interface without requiring a LAMMPS installation.The assertion pattern using
assertInto verify expected LAMMPS commands are present in the generatedlmp.infile is reasonable for unit testing, though it doesn't enforce command order or completeness. This trade-off is acceptable given the test's focus on validating interface behavior rather than LAMMPS execution semantics.
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.