Fixes for manually specified potentials#279
Conversation
|
Warning Rate limit exceeded@pre-commit-ci[bot] has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughParses and normalizes LAMMPS potential input via a new helper, extracts overrides (units, atom_style, dimension) and an inferred atom_type, applies those overrides when assembling lmp.in, and passes atom_type into the datafile writer (write_lammps_datafile). Tests updated to exercise the new helper. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #279 +/- ##
==========================================
+ Coverage 92.58% 92.75% +0.17%
==========================================
Files 8 8
Lines 957 980 +23
==========================================
+ Hits 886 909 +23
Misses 71 71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pyiron_lammps/compatibility/file.py (1)
187-195: Verify thatatom_typevalues derived fromatom_styleare fully supported bywrite_lammps_datafileYou now pass:
write_lammps_datafile( ..., atom_type=atom_type, )where
atom_typeis taken as the last token of theatom_styleline (e.g."atomic","charge","full","bond", …).From the snippet in
pyiron_lammps/structure.py, thestructuremethod currently distinguishes only"charge"vs the default (treated as"atomic"). Ifwrite_lammps_datafileultimately relies on thatstructuremethod, thenatom_type="full"or"bond"would still follow the"atomic"path and not get the specialized full/bond formatting you probably intend.Please double‑check that:
write_lammps_datafile’s implementation actually handles allatom_typevalues you expect from potentials (at least"atomic"and"charge", and possibly"full"/"bond"), and- The mapping from
atom_style→atom_typeis aligned with that implementation.If not, consider either extending the underlying structure writer to support those additional types or normalizing
atom_typehere (e.g. mapping unsupported styles to a supported one or raising early for unsupported combinations).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pyiron_lammps/compatibility/file.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pyiron_lammps/compatibility/file.py (2)
pyiron_lammps/compatibility/structure.py (2)
structure(23-29)structure(32-51)pyiron_lammps/structure.py (2)
structure(228-234)structure(237-251)
🪛 Ruff (0.14.6)
pyiron_lammps/compatibility/file.py
104-104: Ambiguous variable name: l
(E741)
116-116: Ambiguous variable name: l
(E741)
⏰ 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)
|
|
||
| potential_replace = {} | ||
| potential_lst = [] | ||
| for l in potential_dataframe["Config"]: | ||
| if l.startswith("units"): | ||
| potential_replace["units"] = l | ||
| elif l.startswith("atom_style"): | ||
| potential_replace["atom_style"] = l | ||
| elif l.startswith("dimension"): | ||
| potential_replace["dimension"] = l | ||
| else: | ||
| potential_lst.append(l) | ||
|
|
||
| lmp_str_lst = [] | ||
| atom_type = "atomic" | ||
| for l in lammps_file_initialization(structure=structure): | ||
| if l.startswith("units") and "units" in potential_replace: | ||
| lmp_str_lst.append(potential_replace["units"]) | ||
| elif l.startswith("atom_style") and "atom_style" in potential_replace: | ||
| lmp_str_lst.append(potential_replace["atom_style"]) | ||
| atom_type = potential_replace["atom_style"].split()[-1] | ||
| elif l.startswith("dimension") and "dimension" in potential_replace: | ||
| lmp_str_lst.append(potential_replace["dimension"]) | ||
| else: | ||
| lmp_str_lst.append(l) | ||
|
|
||
| lmp_str_lst += potential_lst |
There was a problem hiding this comment.
Ensure LAMMPS units stay consistent between potential config, initialization, and Python code
Right now you parse a units line from potential_dataframe["Config"] and use it to replace the initialization line, but:
lammps_file_initializationis called withoutunits, so it always starts from its default"metal".- The local
unitsvariable (function argument) is not updated from the potential config. - The same
unitsvariable is still passed later intocalc_md,calc_minimize,write_lammps_datafile, andparse_lammps_output.
This can easily lead to inconsistent behavior, e.g. the input script runs with units real (from the potential) while the rest of the pipeline still assumes units="metal" (or a caller‑provided value). That silently corrupts physical results and parsing, so it’s a correctness issue, not just style.
You can fix this and address the E741 warning (“ambiguous variable name: l”) at the same time by:
- Renaming the loop variable to
line. - Parsing the potential’s units token and overwriting the local
unitsvariable when present. - Passing
unitsintolammps_file_initializationso user‑supplied units are honored when the potential does not specify them.
For example:
- potential_replace = {}
- potential_lst = []
- for l in potential_dataframe["Config"]:
- if l.startswith("units"):
- potential_replace["units"] = l
- elif l.startswith("atom_style"):
- potential_replace["atom_style"] = l
- elif l.startswith("dimension"):
- potential_replace["dimension"] = l
- else:
- potential_lst.append(l)
-
- lmp_str_lst = []
- atom_type = "atomic"
- for l in lammps_file_initialization(structure=structure):
- if l.startswith("units") and "units" in potential_replace:
- lmp_str_lst.append(potential_replace["units"])
- elif l.startswith("atom_style") and "atom_style" in potential_replace:
- lmp_str_lst.append(potential_replace["atom_style"])
- atom_type = potential_replace["atom_style"].split()[-1]
- elif l.startswith("dimension") and "dimension" in potential_replace:
- lmp_str_lst.append(potential_replace["dimension"])
- else:
- lmp_str_lst.append(l)
-
- lmp_str_lst += potential_lst
+ potential_replace = {}
+ potential_lst = []
+ potential_units = None
+ for line in potential_dataframe["Config"]:
+ stripped = line.lstrip()
+ if stripped.startswith("units"):
+ potential_replace["units"] = line
+ tokens = stripped.split()
+ if len(tokens) >= 2:
+ potential_units = tokens[1]
+ elif stripped.startswith("atom_style"):
+ potential_replace["atom_style"] = line
+ elif stripped.startswith("dimension"):
+ potential_replace["dimension"] = line
+ else:
+ potential_lst.append(line)
+
+ # If the potential defines units, make sure all downstream code uses the same units
+ if potential_units is not None:
+ units = potential_units
+
+ lmp_str_lst = []
+ atom_type = "atomic"
+ for line in lammps_file_initialization(structure=structure, units=units):
+ if line.startswith("units") and "units" in potential_replace:
+ lmp_str_lst.append(potential_replace["units"])
+ elif line.startswith("atom_style") and "atom_style" in potential_replace:
+ lmp_str_lst.append(potential_replace["atom_style"])
+ atom_type = potential_replace["atom_style"].split()[-1]
+ elif line.startswith("dimension") and "dimension" in potential_replace:
+ lmp_str_lst.append(potential_replace["dimension"])
+ else:
+ lmp_str_lst.append(line)
+
+ lmp_str_lst += potential_lstThis keeps behavior unchanged when the potential does not specify units, but guarantees that when it does, all subsequent code (MD/minimize input, datafile writing, output parsing) uses a consistent unit system.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.6)
104-104: Ambiguous variable name: l
(E741)
116-116: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
In pyiron_lammps/compatibility/file.py around lines 101 to 127, the code
extracts a "units" line from potential_dataframe["Config"] but does not update
the local units variable nor pass units into lammps_file_initialization, and
uses an ambiguous loop variable `l`; update this by renaming loop variables to
`line`, parse the units token from potential_replace["units"] (e.g. last token)
and assign it to the local units variable when present, and pass the (possibly
updated) units into lammps_file_initialization(structure=structure, units=units)
so initialization, downstream
calc_md/calc_minimize/write_lammps_datafile/parse_lammps_output, and the
generated script all use the same units; preserve the existing atom_type
extraction logic but use `line` instead of `l`.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
pyiron_lammps/compatibility/file.py (2)
244-254: Rename ambiguous loop variable.The variable
lis ambiguous and flagged by Ruff. Use a more descriptive name likeline.Apply this diff:
potential_replace = {} potential_lst = [] - for l in potential_dataframe["Config"]: - if l.startswith("units"): - potential_replace["units"] = l - elif l.startswith("atom_style"): - potential_replace["atom_style"] = l - elif l.startswith("dimension"): - potential_replace["dimension"] = l + for line in potential_dataframe["Config"]: + if line.startswith("units"): + potential_replace["units"] = line + elif line.startswith("atom_style"): + potential_replace["atom_style"] = line + elif line.startswith("dimension"): + potential_replace["dimension"] = line else: - potential_lst.append(l) + potential_lst.append(line)
93-106: Address ambiguous variable names and units consistency.The loop variable
lis ambiguous (flagged by Ruff). More importantly, the previous review comment about units consistency remains unaddressed: when the potential specifies units, the localunitsvariable should be updated to ensure consistency acrosslammps_file_initialization, MD/minimize calculations, datafile writing, and output parsing.Based on previous review comments, apply these changes to rename the variable and propagate units:
+ # Extract units from potential if specified + potential_units = None + if "units" in potential_replace: + tokens = potential_replace["units"].split() + if len(tokens) >= 2: + potential_units = tokens[1] + + # Override units if potential specifies them + if potential_units is not None: + units = potential_units + lmp_str_lst = [] atom_type = "atomic" - for l in lammps_file_initialization(structure=structure): - if l.startswith("units") and "units" in potential_replace: + for line in lammps_file_initialization(structure=structure, units=units): + if line.startswith("units") and "units" in potential_replace: lmp_str_lst.append(potential_replace["units"]) - elif l.startswith("atom_style") and "atom_style" in potential_replace: + elif line.startswith("atom_style") and "atom_style" in potential_replace: lmp_str_lst.append(potential_replace["atom_style"]) atom_type = potential_replace["atom_style"].split()[-1] - elif l.startswith("dimension") and "dimension" in potential_replace: + elif line.startswith("dimension") and "dimension" in potential_replace: lmp_str_lst.append(potential_replace["dimension"]) else: - lmp_str_lst.append(l) + lmp_str_lst.append(line) lmp_str_lst += potential_lstThis ensures that when a potential defines units, all downstream code (initialization, MD/minimize, datafile writing, output parsing) uses the same unit system.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyiron_lammps/compatibility/file.py(3 hunks)tests/test_compatibility_file.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_compatibility_file.py (1)
pyiron_lammps/compatibility/file.py (1)
_get_potential(232-256)
pyiron_lammps/compatibility/file.py (3)
pyiron_lammps/structure.py (4)
potential(220-221)potential(224-225)structure(228-234)structure(237-251)pyiron_lammps/compatibility/structure.py (2)
structure(23-29)structure(32-51)pyiron_lammps/potential.py (1)
get_potential_by_name(349-355)
🪛 GitHub Actions: Pipeline
tests/test_compatibility_file.py
[error] 507-507: Bouhadja test failed: assertion error - the item 'l' unexpectedly appears in potential_lst (test expectation not met).
pyiron_lammps/compatibility/file.py
[error] 91-91: NameError: name 'resource_path' is not defined in _get_potential. (Triggered while executing the test via 'python -m unittest discover tests')
[error] 235-235: NameError: name 'potential_dataframe' is not defined in lammps_file_interface_function.
🪛 Ruff (0.14.6)
tests/test_compatibility_file.py
503-503: Ambiguous variable name: l
(E741)
pyiron_lammps/compatibility/file.py
95-95: Ambiguous variable name: l
(E741)
235-235: Undefined name resource_path
(F821)
246-246: Ambiguous variable name: l
(E741)
🔇 Additional comments (1)
tests/test_compatibility_file.py (1)
6-9: LGTM!The import of
_get_potentialis necessary to test the new helper function.
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.