Add Lammps function for backwards compatibility to pyiron_atomistics #262
Conversation
WalkthroughAdds a LAMMPS compatibility layer: new calculation helpers (calc_md, calc_minimize, calc_static), file-based runner and initializer (lammps_file_interface_function, lammps_file_initialization), LAMMPS-compatible structure utilities, updates to write_lammps_datafile/structure handling, and a comprehensive test suite with static log fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Interface as lammps_file_interface_function
participant Init as lammps_file_initialization
participant CalcHelpers as calc_md/calc_minimize/calc_static
participant DataUtil as write_lammps_datafile
participant LAMMPS as LAMMPS_Process
participant Parser as parse_lammps_output
User->>Interface: call(working_directory, structure, potential, calc_mode, calc_kwargs)
Interface->>Interface: create working directory
Interface->>Init: lammps_file_initialization(structure, units, dimension)
Interface->>CalcHelpers: select & build calc lines based on calc_mode
CalcHelpers-->>Interface: calc lines (md/minimize/static)
Interface->>DataUtil: write_lammps_datafile(structure, atom_type=...)
Interface->>Interface: write lmp.in (initial + potential + calc lines)
Interface->>LAMMPS: execute shell command (runs LAMMPS)
LAMMPS-->>Interface: raw log/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) | ⏱️ ~50 minutes
Files/areas needing extra attention:
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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% 91.07% -4.78%
==========================================
Files 5 7 +2
Lines 652 852 +200
==========================================
+ Hits 625 776 +151
- Misses 27 76 +49 ☔ 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
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/test_compatibility_file.py (1)
34-165: Consider reducing test code duplication.The three test methods share significant structural similarity (setup structure, call function, validate keys, validate file content). Consider refactoring with a helper method or using parameterized tests (e.g., with
pytest.mark.parametrizeorunittest.subTest) to reduce duplication and improve maintainability.Example pattern:
def _run_and_validate_test(self, calc_mode, calc_kwargs, content_expected): structure = bulk("Al", cubic=True).repeat([2, 2, 2]) potential = "1999--Mishin-Y--Al--LAMMPS--ipr1" _, parsed_output, job_crashed = lammps_file_interface_function( working_directory=self.working_dir, structure=structure, potential=potential, calc_mode=calc_mode, calc_kwargs=calc_kwargs, units="metal", 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 content_expected: self.assertIn(line, content)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/static/compatibility_output/dump.outis excluded by!**/*.out
📒 Files selected for processing (2)
tests/static/compatibility_output/log.lammps(1 hunks)tests/test_compatibility_file.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/static/compatibility_output/log.lammps
🧰 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)
🪛 Ruff (0.14.5)
tests/test_compatibility_file.py
44-44: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
88-88: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
131-131: 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)
| "n_ionic_steps": 1000, | ||
| "n_print": 100, | ||
| } | ||
| shell_output, parsed_output, job_crashed = lammps_file_interface_function( |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace unused shell_output with underscore.
The shell_output variable is unpacked but never used in any of the three test methods. Replace it with _ to indicate it's intentionally unused.
Apply this diff to mark the variable as unused:
- shell_output, parsed_output, job_crashed = lammps_file_interface_function(
+ _, parsed_output, job_crashed = lammps_file_interface_function(Apply the same change to lines 88 and 131 in the other test methods.
Also applies to: 88-88, 131-131
🧰 Tools
🪛 Ruff (0.14.5)
44-44: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 Prompt for AI Agents
In tests/test_compatibility_file.py around lines 44, 88, and 131, the unpacked
variable shell_output is never used; replace shell_output with _ in each of the
three lammps_file_interface_function unpackings so the unused value is clearly
ignored (i.e., change the tuple unpack to (_, parsed_output, job_crashed) at
those lines).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
tests/test_compatibility_file.py (2)
44-44: Replace unusedshell_outputwith underscore.The
shell_outputvariable is unpacked but never used. This issue was previously flagged and marked as addressed, but the code still contains the unused variable.Apply this diff to mark the variable as unused:
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function(Apply the same change at lines 88, 131, and 170.
Also applies to: 88-88, 131-131, 170-170
59-59: Useos.path.joinfor portable path construction.String concatenation with
+for file paths is not portable across operating systems. This was previously flagged and marked as addressed, but the code still uses string concatenation.Apply this diff:
- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f:Apply the same change at lines 103, 145, and 184.
Also applies to: 103-103, 145-145, 184-184
pyiron_lammps/compatibility/calculate.py (5)
161-161: Critical: NVT(Langevin) ensemble line is missing thefix ensembleprefix.The NVT branch with
langevin=Truegeneratesfix_ensemble_str = "all nve", which lacks the requiredfix ensembleprefix. This was flagged in a previous review and marked as addressed, but the bug is still present. This will produce invalid LAMMPS input that breaks MD runs with Langevin dynamics at constant volume.Apply this diff:
if langevin: # NVT(Langevin) - fix_ensemble_str = "all nve" + fix_ensemble_str = "fix ensemble all nve" thermo_str = "fix langevin all langevin {0} {1} {2} {3} zero yes".format(
9-23: Critical:time_stepparameter documented but not implemented, causing TypeError.The docstring documents a
time_stepparameter (line 40: "Step size in fs between two steps"), and it's listed inlammps_file_interface_function's documentation for MD mode. However:
time_stepis not in thecalc_mdsignature (lines 9-23)- The timestep is hardcoded to
0.001at line 187- When
time_stepis passed viacalc_kwargs(which uses**calc_kwargs), it will raiseTypeError: unexpected keyword argument 'time_step'This was flagged in a previous review but remains unfixed.
Add
time_stepas a parameter with a default value and apply unit conversion:def calc_md( temperature=None, pressure=None, n_print=100, + time_step=1.0, # fs temperature_damping_timescale=100.0,Then convert and use it:
line_lst.append("variable thermotime equal {} ".format(n_print)) - line_lst.append("timestep 0.001") + line_lst.append(f"timestep {time_step * time_units}")Also applies to: 40-40, 187-187
103-106: Major: Explicitinitial_temperatureis never unit-converted.When
initial_temperatureis explicitly provided by the caller, it is not scaled bytemperature_units, whiletemperatureis scaled (line 101). This creates a unit mismatch whentemperature_units != 1(non-metal units). Only the implicit default (initial_temperature = 2 * temperature[0]) correctly inherits the converted units.This was flagged in a previous review and marked as addressed, but the issue persists.
Apply this diff:
# 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
220-221: Major: Docstring documentse_tol/f_tolparameters that are not accepted.The docstring mentions deprecated parameters
e_tolandf_tol(lines 220-221), but the function signature (lines 202-210) does not accept them. Any caller using these deprecated names will getTypeError: unexpected keyword argument. This was flagged in a previous review but remains unaddressed.Either remove the deprecated parameters from the docstring, or add backwards compatibility:
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", + **kwargs, ): + # Handle deprecated parameter names + 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)}")
317-347: Major:_set_initial_velocitydoesn't honortemperature=0behavior from docstring.The
calc_mddocstring (lines 55-56) 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. This was flagged in a previous review but remains unfixed.Update
_set_initial_velocityto return an empty string when temperature is 0:def _set_initial_velocity( temperature, seed=80996, gaussian=False, append_value=False, zero_lin_momentum=True, zero_rot_momentum=True, ): + if temperature == 0: + return "" + arg = "" if gaussian:Then update
calc_mdto only append non-empty velocity commands:- 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)
🧹 Nitpick comments (3)
pyiron_lammps/compatibility/calculate.py (3)
74-74: Optional: Addstacklevel=2towarnings.warncalls for clearer stack traces.The deprecation and advisory warnings lack
stacklevel=2, which would make the warning point to the caller's code rather than this function's internals, improving debuggability.Apply this pattern to all four warnings:
- warnings.warn( - "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`." - ) + warnings.warn( + "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`.", + DeprecationWarning, + stacklevel=2, + )Also applies to: 83-83, 176-176, 243-243
Also applies to: 83-83, 176-176, 243-243
137-137: Optional: Remove unusedforce_skewedassignment.The variable
force_skewedis assigned at line 137 but never used. Either remove it or implement the intended behavior.coord, value, pressure_damping_timescale ) - if ii > 2: - force_skewed = TrueAlso applies to: 118-118
270-270: Optional: Rename unused loop variableiito_or_ii.The loop control variable
iiis not used within the loop body at line 270.str_press = "" - 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)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-143)
pyiron_lammps/compatibility/calculate.py (1)
pyiron_lammps/structure.py (1)
UnfoldingPrism(38-191)
🪛 Ruff (0.14.5)
tests/test_compatibility_file.py
44-44: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
88-88: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
131-131: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
170-170: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
pyiron_lammps/compatibility/calculate.py
74-74: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
83-83: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
108-108: Avoid specifying long messages outside the exception class
(TRY003)
114-116: Avoid specifying long messages outside the exception class
(TRY003)
137-137: Local variable force_skewed is assigned to but never used
Remove assignment to unused variable force_skewed
(F841)
156-158: Avoid specifying long messages outside the exception class
(TRY003)
176-176: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
243-243: No explicit stacklevel keyword argument found
Set stacklevel=2
(B028)
258-261: Avoid specifying long messages outside the exception class
(TRY003)
270-270: Loop control variable ii not used within loop body
Rename unused ii to _ii
(B007)
297-297: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
389-389: Avoid specifying long messages outside the exception class
(TRY003)
397-404: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
tests/test_compatibility_file.py (2)
44-44: [Duplicate] Replace unusedshell_outputwith underscore.
shell_outputis unpacked but never used in any test method. Mark it as intentionally unused by replacing with_.Apply this pattern to all five test methods (lines 44, 88, 131, 170, 210):
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _, parsed_output, job_crashed = lammps_file_interface_function(
59-59: [Duplicate] Useos.path.joinfor cross-platform path construction.String concatenation (
self.working_dir + "/lmp.in") is not portable across operating systems. Useos.path.joininstead.Apply this pattern to all five occurrences (lines 59, 103, 145, 184, 225):
- with open(self.working_dir + "/lmp.in", "r") as f: + with open(os.path.join(self.working_dir, "lmp.in"), "r") as f:pyiron_lammps/compatibility/calculate.py (4)
161-162: [Duplicate] NVT Langevin ensemble line is missing thefixcommand.The NVT Langevin branch generates
fix_ensemble_str = "all nve"without the leadingfix ensemble, producing invalid LAMMPS input.This issue was flagged in a previous review (lines 140-169) and remains unaddressed. Apply the fix:
- if langevin: # NVT(Langevin) - fix_ensemble_str = "all nve" + if langevin: # NVT(Langevin) + fix_ensemble_str = "fix ensemble all nve"
28-49: [Duplicate]time_stepparameter documented but not accepted bycalc_md.The docstring (line 41) documents
time_stepandlammps_file_interface_functionlists it incalc_kwargs, butcalc_mddoes not accept this parameter. Passingtime_stepvia**calc_kwargswill raiseTypeError.This issue was flagged in a previous review (lines 28-49) and remains unaddressed. Either add
time_stepto the signature and use it (replacing the hard-coded0.001at line 188), or remove it from all docstrings.
256-287: [Duplicate]calc_minimizehas unbound variable and nested list issues.Two issues in
calc_minimize:
str_pressis only defined whenpressure is not None, but is used unconditionally (line 287), causingUnboundLocalErrorfor isochoric minimization.line_lst += _get_thermo()is correct, but it was previously wrapped in a list literal causing nesting.This issue was flagged in a previous review (lines 202-299). Initialize
str_press = ""before the conditional:line_lst = ["variable thermotime equal {} ".format(n_print)] line_lst += _get_thermo() + str_press = "" if pressure is not None:
218-224: [Duplicate] Docstring mentions deprecatede_tol/f_tolnot accepted by function.The docstring documents
e_tolandf_tolas deprecated aliases (lines 222-223), but the function signature does not accept them. Callers using these names will encounterTypeError.This issue was flagged in a previous review (lines 218-224) and remains unaddressed. Either accept these via
**kwargswith deprecation warnings, or remove them from the docstring.pyiron_lammps/compatibility/file.py (3)
91-91: [Duplicate]lammps_file_initializationignores theunitsargument.You accept
unitsinlammps_file_interface_functionand pass it towrite_lammps_datafileandparse_lammps_output, but you calllammps_file_initialization(structure=structure)without forwardingunits. This causes the LAMMPS input to always use"metal"units regardless of the caller's request, desynchronizing the simulation from datafile/output parsing.This issue was flagged in a previous review (lines 87-97). Apply this fix:
- lmp_str_lst = lammps_file_initialization(structure=structure) + lmp_str_lst = lammps_file_initialization(structure=structure, units=units)
48-79: [Duplicate] Docstring documents parameters not in function signature.The docstring lists 18+ parameters (
cutoff_radius,bonds_kwargs,server_kwargs,enable_h5md, restart options, executable options, etc.) that the function does not accept, and mentions"vcsgc"calc_mode which is not implemented (only"static","md","minimize"are handled at lines 99-114).This issue was flagged in a previous review (lines 48-79). Update the docstring to match the actual 8-parameter signature, remove unsupported parameters and calc_modes, or extend the function to accept them.
128-134: [Duplicate]shell=Truewith user-controllablelmp_commandposes security risk.
lmp_commandis a user-controllable parameter passed directly tosubprocess.check_output(..., shell=True), allowing shell injection. This was flagged in a previous review (lines 128-134).Consider switching to
shell=Falsewith a list of arguments, or add input validation and document thatlmp_commandmust be trusted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pyiron_lammps/compatibility/calculate.py(1 hunks)pyiron_lammps/compatibility/file.py(1 hunks)tests/test_compatibility_file.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-145)
pyiron_lammps/compatibility/calculate.py (1)
pyiron_lammps/structure.py (2)
UnfoldingPrism(38-191)is_skewed(433-450)
pyiron_lammps/compatibility/file.py (4)
pyiron_lammps/compatibility/calculate.py (3)
calc_md(10-200)calc_minimize(203-300)calc_static(303-304)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)
🪛 GitHub Actions: Pipeline
tests/test_compatibility_file.py
[error] 131-131: test_calc_static failed: TypeError: calc_static() missing 1 required positional argument: 'structure'
pyiron_lammps/compatibility/file.py
[error] 100-100: TypeError: calc_static() missing 1 required positional argument: 'structure'
🪛 Ruff (0.14.5)
tests/test_compatibility_file.py
44-44: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
88-88: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
131-131: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
170-170: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
210-210: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
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)
303-303: Unused function argument: structure
(ARG001)
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)
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)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
tests/test_compatibility_file.py (4)
93-132: Apply the same refactors as intest_calc_md_npt.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()
134-168: Apply the same refactors as intest_calc_md_npt.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="static", 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()
170-205: Apply the same refactors as intest_calc_md_npt.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="minimize", 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()
207-244: Apply the same refactors as intest_calc_md_npt.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="minimize", units=self.units, calc_kwargs={"pressure": 0.0}, 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()pyiron_lammps/compatibility/calculate.py (4)
161-168: Fix invalid LAMMPS syntax in NVT(Langevin) ensemble.Line 162 generates
"all nve"without the requiredfixcommand prefix. This will produce invalid LAMMPS input and break Langevin NVT runs.Apply this diff:
if langevin: # NVT(Langevin) - fix_ensemble_str = "all nve" + fix_ensemble_str = "fix ensemble all nve" thermo_str = "fix langevin all langevin {0} {1} {2} {3} zero yes".format(
10-67: Documentedtime_stepparameter is not accepted by the function.The docstring (line 41) and caller documentation describe a
time_stepparameter in fs, but:
- It's not in the function signature
- Timestep is hard-coded to
0.001(line 188)- Passing
time_stepviacalc_kwargswill raiseTypeErrorEither add
time_stepto the signature with proper unit conversion (consistent withtemperature_damping_timescaleandpressure_damping_timescale), or remove it from the docstring if configurability isn't needed for backwards compatibility.Apply this diff to add the parameter:
def calc_md( temperature=None, pressure=None, n_print=100, + time_step=1.0, temperature_damping_timescale=100.0, pressure_damping_timescale=1000.0, seed=80996, @@ line_lst.append("variable thermotime equal {} ".format(n_print)) - line_lst.append("timestep 0.001") + line_lst.append(f"timestep {time_step * time_units}") line_lst.append(Also applies to: 188-188
203-238: Docstring mentions deprecatede_tol/f_tolparameters not accepted by the function.Lines 222-223 document
e_tolandf_tolas deprecated aliases, but the function signature doesn't accept them. Callers using these names will getTypeError.For backwards compatibility, consider accepting and mapping these via
**kwargs, or remove them from the docstring if they're no longer supported.
324-354:_set_initial_velocitydoesn't honor the documentedtemperature=0behavior.The
calc_mddocstring (line 56) 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, which will overwrite existing velocities.Apply this diff to honor the documented behavior:
def _set_initial_velocity( temperature, seed=80996, gaussian=False, append_value=False, zero_lin_momentum=True, zero_rot_momentum=True, ): """ Create initial velocities via velocity all create. More information can be found on LAMMPS website: https://lammps.sandia.gov/doc/velocity.html Args: temperature: (int or float) seed: (int) Seed for the initial random number generator gaussian: (True/False) Create velocity according to the Gaussian distribution (otherwise uniform) append_value: (True/False) Add the velocity values to the current velocities (probably not functional now) zero_lin_momentum: (True/False) Cancel the total linear momentum zero_rot_momentum: (True/False) Cancel the total angular momentum """ + if temperature == 0: + return "" arg = ""And in
calc_md, only append if non-empty:- 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)
🧹 Nitpick comments (6)
tests/test_compatibility_file.py (1)
47-91: Address two refactoring items from previous reviews.Two issues remain from prior reviews:
- Unused
shell_outputvariable (line 54): Replace with_to clearly indicate it's intentionally unused.- Non-portable path construction (line 69): Use
os.path.join(self.working_dir, "lmp.in")instead of string concatenation.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()pyiron_lammps/compatibility/calculate.py (5)
138-138: Remove unusedforce_skewedvariable.Apply this diff:
- force_skewed = False pressure = _pressure_to_lammps( @@ if ii > 2: - force_skewed = True + pass
74-77: Addstacklevel=2to warnings for better debugging.The warnings should include
stacklevel=2so the warning points to the caller's location rather than this function.Apply this diff:
warnings.warn( - "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`." + "WARNING: `delta_temp` is deprecated, please use `temperature_damping_timescale`.", + stacklevel=2, ) @@ warnings.warn( - "WARNING: `delta_press` is deprecated, please use `pressure_damping_timescale`." + "WARNING: `delta_press` is deprecated, please use `pressure_damping_timescale`.", + stacklevel=2, ) @@ if langevin: - warnings.warn("Temperature not set; Langevin ignored.") + warnings.warn("Temperature not set; Langevin ignored.", stacklevel=2)Also applies to: 83-86, 176-178
244-246: Addstacklevel=2to warning for better debugging.Apply this diff:
warnings.warn( - "n_print larger than max_iter, adjusting to n_print=max_iter" + "n_print larger than max_iter, adjusting to n_print=max_iter", + stacklevel=2, )
277-282: Rename unused loop variableiito_ii.Apply this diff:
- for ii, (press, str_axis) in enumerate( + for _ii, (press, str_axis) in enumerate( zip(pressure, ["x", "y", "z", "xy", "xz", "yz"]) ):
439-441: Addstacklevel=2to warnings for better debugging.Apply this diff:
- warnings.warn("No structure set, can not validate the simulation cell!") + warnings.warn("No structure set, can not validate the simulation cell!", stacklevel=2) @@ warnings.warn( "WARNING: Setting a calculation type which uses pressure before setting the structure risks " + "constraining your cell shape evolution if non-diagonal pressures are used but the structure " + "is not triclinic from the start of the calculation." + stacklevel=2, )Also applies to: 468-473
📜 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)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
lammps_file_interface_function(17-145)
pyiron_lammps/compatibility/calculate.py (1)
pyiron_lammps/structure.py (2)
UnfoldingPrism(38-191)is_skewed(433-450)
🪛 Ruff (0.14.5)
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)
95-95: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
135-135: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
171-171: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
208-208: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
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)
⏰ 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)
* feat: Increase test coverage for compatibility module This change adds a significant number of new test cases to the `compatibility` module to improve its test coverage. The new tests cover various scenarios for `calc_md` and `calc_minimize`, including different temperature and pressure settings, the use of the Langevin thermostat, and the handling of deprecated parameters. - Added tests for `calc_md` with various temperature and pressure inputs. - Added a test for the `langevin` flag in `calc_md`. - Added tests for the deprecation warnings of `delta_temp` and `delta_press`. - Added tests for `calc_minimize` with and without pressure. - Cleaned up the `.gitignore` file to include the `.coverage` file. * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
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.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.