Add additional file-based interface for LAMMPS #409
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
for more information, see https://pre-commit.ci
WalkthroughThe pull request primarily updates the naming convention for LAMMPS-related functions across multiple modules, notebooks, and tests. Functions previously suffixed with “with_lammps” are renamed to “with_lammpslib”, and corresponding import statements and export lists are updated. Additionally, new dependencies (e.g. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Caller
participant Eval as evaluate_with_lammpsfile
participant Init as _lammps_file_initialization
participant Writer as _write_lammps_input_file
participant Exec as LAMMPS Executable
participant Output as GenericOutput
User->>Eval: Request evaluation with task parameters
Eval->>Init: Generate initialization commands
Init-->>Eval: Return config commands
Eval->>Writer: Compose LAMMPS input file
Writer-->>Eval: Write input file to directory
Eval->>Exec: Invoke executable_function with input file
Exec-->>Eval: Return simulation output
Eval->>Output: Parse output to obtain forces, energy, stress, etc.
Output-->>Eval: Return processed results
Eval-->>User: Provide evaluation results
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
atomistics/calculators/__init__.py (1)
55-68:⚠️ Potential issueUpdate
__all__list to match the new function names.The
__all__list still contains the old function names with "with_lammps" suffix, while the imports have been updated to use "with_lammpslib". This mismatch will cause import errors.Apply this diff to update the
__all__list:__all__ += [ - "calc_molecular_dynamics_thermal_expansion_with_lammps", - "calc_molecular_dynamics_nph_with_lammps", - "calc_molecular_dynamics_npt_with_lammps", - "calc_molecular_dynamics_nvt_with_lammps", - "calc_molecular_dynamics_langevin_with_lammps", - "calc_static_with_lammps", - "evaluate_with_lammps", - "evaluate_with_lammps_library", + "calc_molecular_dynamics_thermal_expansion_with_lammpslib", + "calc_molecular_dynamics_nph_with_lammpslib", + "calc_molecular_dynamics_npt_with_lammpslib", + "calc_molecular_dynamics_nvt_with_lammpslib", + "calc_molecular_dynamics_langevin_with_lammpslib", + "calc_static_with_lammpslib", + "evaluate_with_lammpslib", + "evaluate_with_lammpslib_library_interface", "get_potential_dataframe", "get_potential_by_name", - "optimize_positions_and_volume_with_lammps", - "optimize_positions_with_lammps", + "optimize_positions_and_volume_with_lammpslib", + "optimize_positions_with_lammpslib", ]
🧹 Nitpick comments (3)
tests/test_hessian_lammps.py (1)
18-19: Remove unused import.The function
evaluate_with_lammpslib_library_interfaceis imported but never used in this file.Apply this diff:
from atomistics.calculators import ( evaluate_with_lammpslib, - evaluate_with_lammpslib_library_interface, get_potential_by_name, )🧰 Tools
🪛 Ruff (0.8.2)
19-19:
atomistics.calculators.evaluate_with_lammpslib_library_interfaceimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
notebooks/free_energy_calculation.ipynb (2)
33-39: Ensure Consistency in Import Naming.
The import block now includes the newevaluate_with_lammpslib_library_interfacealongside the existingevaluate_with_lammps. Please verify that both functions are intended to coexist. If the goal is a complete rename, ensure that all references to the old function (evaluate_with_lammps) are updated accordingly or removed to avoid any confusion.
641-644: Update Function Invocation for Renamed Interface.
The function call at this hunk now usesevaluate_with_lammpslib_library_interface, which aligns with the new naming convention. Confirm that the parameters provided (task_dict,potential_dataframe,lmp) are fully compatible with the renamed function’s definition and that any remaining calls to the old interface are updated throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
atomistics/calculators/__init__.py(2 hunks)atomistics/calculators/lammps/__init__.py(2 hunks)atomistics/calculators/lammps/libcalculator.py(13 hunks)atomistics/calculators/lammps/phonon.py(1 hunks)notebooks/free_energy_calculation.ipynb(2 hunks)notebooks/lammps_workflows.ipynb(22 hunks)notebooks/thermal_expansion_with_lammps.ipynb(3 hunks)tests/test_calc_stress_lammps.py(2 hunks)tests/test_elastic_lammps.py(3 hunks)tests/test_elastic_lammps_functional.py(3 hunks)tests/test_evcurve_lammps.py(3 hunks)tests/test_evcurve_lammps_function_parallel.py(3 hunks)tests/test_evcurve_lammps_functional.py(3 hunks)tests/test_hessian_lammps.py(3 hunks)tests/test_lammps_md.py(7 hunks)tests/test_lammps_md_phonons.py(2 hunks)tests/test_langevin_lammps.py(2 hunks)tests/test_molecular_dynamics_thermal_expansion_lammps.py(3 hunks)tests/test_optimize_positions_lammps.py(2 hunks)tests/test_phonons_lammps.py(3 hunks)tests/test_phonons_lammps_functional.py(3 hunks)tests/test_quasiharmonic_lammps.py(3 hunks)tests/test_quasiharmonic_lammps_functional.py(3 hunks)
🔥 Files not summarized due to errors (1)
- notebooks/free_energy_calculation.ipynb: Error: Server error: no LLM provider could handle the message
✅ Files skipped from review due to trivial changes (8)
- tests/test_phonons_lammps_functional.py
- tests/test_optimize_positions_lammps.py
- atomistics/calculators/lammps/phonon.py
- tests/test_phonons_lammps.py
- tests/test_quasiharmonic_lammps.py
- tests/test_molecular_dynamics_thermal_expansion_lammps.py
- notebooks/thermal_expansion_with_lammps.ipynb
- tests/test_lammps_md_phonons.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_hessian_lammps.py
19-19: atomistics.calculators.evaluate_with_lammpslib_library_interface imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
atomistics/calculators/__init__.py
41-41: atomistics.calculators.lammps.calc_molecular_dynamics_langevin_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
42-42: atomistics.calculators.lammps.calc_molecular_dynamics_nph_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
43-43: atomistics.calculators.lammps.calc_molecular_dynamics_npt_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
44-44: atomistics.calculators.lammps.calc_molecular_dynamics_nvt_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
45-45: atomistics.calculators.lammps.calc_molecular_dynamics_thermal_expansion_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
46-46: atomistics.calculators.lammps.calc_static_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
47-47: atomistics.calculators.lammps.evaluate_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
48-48: atomistics.calculators.lammps.evaluate_with_lammpslib_library_interface imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
51-51: atomistics.calculators.lammps.optimize_positions_and_volume_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
52-52: atomistics.calculators.lammps.optimize_positions_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
74-74: atomistics.calculators.lammps.phonon.calc_molecular_dynamics_phonons_with_lammpslib imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.10)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_siesta
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_qe
- GitHub Check: unittest_old
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (macos-latest, 3.12)
- GitHub Check: unittest_matgl
- GitHub Check: unittest_mace
- GitHub Check: unittest_gpaw
- GitHub Check: unittest_abinit
- GitHub Check: pip_check
- GitHub Check: coverage
- GitHub Check: notebooks
- GitHub Check: minimal
🔇 Additional comments (15)
atomistics/calculators/lammps/__init__.py (1)
1-43: LGTM! Consistent renaming of LAMMPS interface functions.The changes systematically rename all LAMMPS-related functions to use the "lammpslib" suffix, maintaining consistency across the module. The
__all__list has been properly updated to reflect these changes.tests/test_calc_stress_lammps.py (1)
10-10: LGTM! Consistent function name updates in test file.All instances of
evaluate_with_lammpshave been properly updated toevaluate_with_lammpslib, maintaining consistency with the renamed interface.Also applies to: 28-28, 33-33, 40-40
tests/test_langevin_lammps.py (1)
12-12: LGTM! Consistent function name updates in Langevin test.All instances of
evaluate_with_lammps_libraryhave been properly updated toevaluate_with_lammpslib_library_interface, maintaining consistency with the renamed interface.Also applies to: 52-52
tests/test_evcurve_lammps.py (1)
13-13: LGTM! Consistent function name updates in EV curve test.All instances of
evaluate_with_lammpshave been properly updated toevaluate_with_lammpslib, maintaining consistency with the renamed interface.Also applies to: 31-31, 45-45
tests/test_evcurve_lammps_functional.py (1)
14-14: LGTM! Function renaming is consistent.The imports and function calls have been correctly updated to use the new
evaluate_with_lammpslibname.Also applies to: 31-34, 42-45
tests/test_evcurve_lammps_function_parallel.py (1)
16-16: LGTM! Function renaming is consistent in parallel execution context.The imports and function calls have been correctly updated to use the new
evaluate_with_lammpslibname in the parallel execution context.Also applies to: 35-36, 48-49
tests/test_hessian_lammps.py (1)
40-43: LGTM! Function renaming is consistent.The function calls have been correctly updated to use the new
evaluate_with_lammpslibname.Also applies to: 54-57
tests/test_quasiharmonic_lammps_functional.py (1)
15-15: LGTM! Function renaming is consistent.The renaming from
evaluate_with_lammpstoevaluate_with_lammpslibis applied consistently across the import statement and function calls, maintaining the test's functionality.Also applies to: 32-32, 48-48
tests/test_elastic_lammps.py (1)
10-10: LGTM! Function renaming is consistent.The renaming from
evaluate_with_lammpstoevaluate_with_lammpslibis applied consistently across the import statement and function calls, maintaining the test's functionality.Also applies to: 28-28, 40-40
tests/test_elastic_lammps_functional.py (1)
13-13: LGTM! Function renaming is consistent.The renaming from
evaluate_with_lammpstoevaluate_with_lammpslibis applied consistently across the import statement and function calls, maintaining the test's functionality.Also applies to: 30-30, 41-41
tests/test_lammps_md.py (1)
10-13: LGTM! Function renaming is consistent.The renaming from
*_with_lammpsto*_with_lammpslibis applied consistently across:
- Import statements for all molecular dynamics functions
- Function calls in test methods
- Signature check in
test_calc_molecular_dynamics_signatureThe test's functionality is maintained.
Also applies to: 32-32, 62-62, 87-87, 120-120, 151-151, 177-177
notebooks/lammps_workflows.ipynb (4)
24-24: LGTM! Import statements consistently updated.The import statements have been systematically updated to use the new "lammpslib" naming convention, maintaining consistency across the notebook.
Also applies to: 63-63, 113-113, 231-232, 285-285, 338-338, 394-394, 448-449, 498-499, 711-711, 777-777, 997-997, 1130-1130, 1176-1176
29-29: LGTM! Function calls to evaluate_with_lammps consistently updated.All calls to the evaluation function have been properly renamed to
evaluate_with_lammpslib, maintaining consistency throughout the notebook.Also applies to: 77-77, 129-129, 529-529, 578-578, 794-794, 1015-1015, 1137-1137, 1183-1183
238-238: LGTM! Molecular dynamics function calls consistently updated.All molecular dynamics related function calls have been properly renamed to use the "lammpslib" suffix, including:
- Langevin dynamics
- NVT ensemble
- NPT ensemble
- NPH ensemble
- Thermal expansion
- Phonon calculations
Also applies to: 292-292, 345-345, 401-401, 456-456, 600-600
738-738: LGTM! Library interface function call properly updated.The library interface evaluation function has been correctly renamed to
evaluate_with_lammpslib_library_interface, maintaining consistency with the new naming scheme while preserving its distinct identity as a library interface.
atomistics/calculators/__init__.py
Outdated
| @@ -71,7 +71,7 @@ | |||
|
|
|||
| try: | |||
| from atomistics.calculators.lammps.phonon import ( | |||
| calc_molecular_dynamics_phonons_with_lammps, | |||
| calc_molecular_dynamics_phonons_with_lammpslib, | |||
| ) | |||
|
|
|||
| __all__ += ["calc_molecular_dynamics_phonons_with_lammps"] | |||
There was a problem hiding this comment.
Update phonon function name in __all__ list.
The phonon function name in __all__ list needs to be updated to match the new naming convention.
Apply this diff:
- __all__ += ["calc_molecular_dynamics_phonons_with_lammps"]
+ __all__ += ["calc_molecular_dynamics_phonons_with_lammpslib"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| __all__ += ["calc_molecular_dynamics_phonons_with_lammps"] | |
| __all__ += ["calc_molecular_dynamics_phonons_with_lammpslib"] |
| @@ -40,7 +40,7 @@ | |||
| from atomistics.calculators.interface import TaskName | |||
|
|
|||
|
|
|||
| def optimize_positions_and_volume_with_lammps( | |||
| def optimize_positions_and_volume_with_lammpslib( | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
LGTM! Consistent renaming of LAMMPS-related functions.
The renaming from *_with_lammps to *_with_lammpslib is consistent across all functions and aligns with the PR objective to rename the LAMMPS interface.
Let's verify that all LAMMPS-related functions have been renamed consistently across the codebase:
Also applies to: 85-85, 118-118, 146-146, 200-200, 260-260, 315-315, 371-371, 424-424, 475-475, 498-498
🏁 Script executed:
#!/bin/bash
# Description: Check for any remaining instances of old function names.
# Search for any remaining instances of old function names
echo "Checking for any remaining instances of old function names..."
rg "with_lammps[^l]" -g '!*.md'
# Search for any potential inconsistencies in the new naming
echo "Checking for any inconsistencies in the new naming..."
rg "with_lammps(?:lib|[^l])" -g '!*.md'Length of output: 25167
Refactor Approved in libcalculator.py – Inconsistent References Elsewhere
The refactoring in atomistics/calculators/lammps/libcalculator.py looks good: all LAMMPS-related functions have been consistently renamed to include the lib suffix. However, our repository-wide search shows that several files (notably in atomistics/calculators/__init__.py, documentation, and various notebooks) still reference the old function names (e.g. calc_molecular_dynamics_thermal_expansion_with_lammps, evaluate_with_lammps, etc.). Please review those occurrences to either update them for consistency with the new naming convention or confirm that their retention is intentional (for backward compatibility or documentation purposes).
- Ensure that string references and exports in
__init__.pyreflect the renaming if full consistency is desired. - Verify and, if needed, update the notebooks and docs (e.g.,
thermal_expansion_with_lammps.ipynb) accordingly.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #409 +/- ##
==========================================
+ Coverage 81.07% 81.52% +0.45%
==========================================
Files 39 40 +1
Lines 2309 2382 +73
==========================================
+ Hits 1872 1942 +70
- Misses 437 440 +3 ☔ View full report in Codecov by Sentry. |
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
jan-janssen
changed the title
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (11)
binder/environment.yml (1)
18-18: Ensure Newline at End-of-File
YAML lint has flagged that line 18 does not end with a newline. To adhere to formatting guidelines, please add a newline after the last dependency.- - pyiron_lammps =0.3.2 + - pyiron_lammps =0.3.2 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
.ci_support/environment-old.yml (1)
22-22: Add newline at end of file.Add a newline character at the end of the file to comply with YAML best practices.
- pyiron_lammps =0.3.2 +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 22-22: no new line character at the end of file
(new-line-at-end-of-file)
tests/test_hessian_lammpslib.py (1)
18-19: Remove unused import.The
evaluate_with_lammpslib_library_interfacefunction is imported but never used in this test file.from atomistics.calculators import ( evaluate_with_lammpslib, - evaluate_with_lammpslib_library_interface, get_potential_by_name, )🧰 Tools
🪛 Ruff (0.8.2)
19-19:
atomistics.calculators.evaluate_with_lammpslib_library_interfaceimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
tests/test_evcurve_lammpsfile.py (1)
43-86: Consider parameterizing the test with different materials.The test only verifies aluminum's energy-volume curve. Adding more materials would increase test coverage.
Consider parameterizing the test:
@pytest.mark.parametrize("element,potential_name", [ ("Al", "1999--Mishin-Y--Al--LAMMPS--ipr1"), ("Cu", "2001--Mishin-Y--Cu--LAMMPS--ipr1"), # Add more material-potential pairs ]) def test_calc_evcurve(self, element, potential_name): structure = bulk(element, cubic=True) # ... rest of the testtests/test_phonons_lammpsfile.py (1)
100-115: Consider using pytest's approx for floating-point comparisons.Using multiple assertTrue with range checks is verbose and harder to maintain.
Consider using pytest's approx:
from pytest import approx # Replace multiple assertions with assert thermal_dict["free_energy"][0] == approx(0.15, abs=0.05) assert thermal_dict["free_energy"][-1] == approx(-2.65, abs=0.05) # ... similar for other checksatomistics/calculators/lammps/filecalculator.py (5)
26-41: Add test coverage for stress and volume extraction.
get_stress()(line 37) andget_volume()(line 40) are uncovered according to code coverage. Ensuring these methods are tested increases confidence in simulation output verification.Would you like assistance in adding a unit test for these methods, perhaps verifying correct handling of parsing edge cases or empty data?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 37-37: atomistics/calculators/lammps/filecalculator.py#L37
Added line #L37 was not covered by tests
[warning] 40-40: atomistics/calculators/lammps/filecalculator.py#L40
Added line #L40 was not covered by tests
43-54: Consider parameterizing dimension for flexible use.
Currently, the dimension is hard-coded to 3 (line 44). This is fine for typical 3D simulations, but if 2D or 1D systems ever become relevant, a parameter or assertion might help avoid confusion.
56-76: Add basic file writing error handling.
While_write_lammps_input_file()is well-structured, adding minimal error handling (e.g., ensuring the working directory is valid) can make the code more robust.
173-210: Validate run success before parsing.
calc_static_with_lammpsfile()depends on_parse_lammps_output. If the run fails or the file is missing, parsing will fail. Consider verifying run success and providing more descriptive error handling.
212-255: Add a test for unsupported task coverage.
The raise statement at line 253 is uncovered by tests. Including a test that passes an unsupported task (e.g.,"nonsensical_task") can ensure any usage error is correctly handled.If you want a snippet demonstrating how to provoke this error and assert that it’s raised, I can prepare it.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 253-253: atomistics/calculators/lammps/filecalculator.py#L253
Added line #L253 was not covered by teststests/test_optimize_positions_lammpsfile.py (1)
11-17: Consider catching specific exceptions instead of using a broadImportError.While the current implementation works, it's a good practice to catch specific exceptions. This helps in better error handling and debugging.
try: from atomistics.calculators.lammps.potential import get_potential_by_name from atomistics.calculators.lammps.filecalculator import evaluate_with_lammpsfile skip_lammps_test = False -except ImportError: +except (ImportError, ModuleNotFoundError) as e: + print(f"LAMMPS import failed: {e}") skip_lammps_test = True
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.ci_support/environment-docs.yml(1 hunks).ci_support/environment-lammps.yml(1 hunks).ci_support/environment-notebooks.yml(1 hunks).ci_support/environment-old.yml(2 hunks)atomistics/calculators/lammps/filecalculator.py(1 hunks)binder/environment.yml(1 hunks)pyproject.toml(1 hunks)tests/test_calc_stress_lammpslib.py(2 hunks)tests/test_elastic_lammpslib.py(3 hunks)tests/test_elastic_lammpslib_functional.py(3 hunks)tests/test_evcurve_lammpsfile.py(1 hunks)tests/test_evcurve_lammpslib.py(3 hunks)tests/test_evcurve_lammpslib_function_parallel.py(3 hunks)tests/test_evcurve_lammpslib_functional.py(3 hunks)tests/test_hessian_lammpslib.py(3 hunks)tests/test_lammpslib_md.py(7 hunks)tests/test_lammpslib_md_phonons.py(2 hunks)tests/test_langevin_lammpslib.py(2 hunks)tests/test_molecular_dynamics_thermal_expansion_lammpslib.py(3 hunks)tests/test_optimize_positions_lammpsfile.py(1 hunks)tests/test_optimize_positions_lammpslib.py(2 hunks)tests/test_phonons_lammpsfile.py(1 hunks)tests/test_phonons_lammpslib.py(3 hunks)tests/test_phonons_lammpslib_functional.py(3 hunks)tests/test_quasiharmonic_lammpslib.py(3 hunks)tests/test_quasiharmonic_lammpslib_functional.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_lammpslib_md.py
🧰 Additional context used
🪛 YAMLlint (1.35.1)
binder/environment.yml
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
.ci_support/environment-notebooks.yml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
.ci_support/environment-old.yml
[error] 22-22: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Ruff (0.8.2)
tests/test_hessian_lammpslib.py
19-19: atomistics.calculators.evaluate_with_lammpslib_library_interface imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
🪛 GitHub Check: codecov/patch
atomistics/calculators/lammps/filecalculator.py
[warning] 37-37: atomistics/calculators/lammps/filecalculator.py#L37
Added line #L37 was not covered by tests
[warning] 40-40: atomistics/calculators/lammps/filecalculator.py#L40
Added line #L40 was not covered by tests
[warning] 253-253: atomistics/calculators/lammps/filecalculator.py#L253
Added line #L253 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.10)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (macos-latest, 3.12)
🔇 Additional comments (21)
.ci_support/environment-notebooks.yml (1)
12-12: Dependency Addition Verification forpyiron_lammps
The new dependencypyiron_lammps =0.3.2has been correctly added to the notebooks environment configuration. This update is consistent with similar changes in related environment files.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
.ci_support/environment-lammps.yml (1)
11-11: Ensure Consistent Dependency Addition
The dependencypyiron_lammps =0.3.2is correctly introduced here, aligning this environment with the other configurations supporting the LAMMPS interface..ci_support/environment-docs.yml (1)
22-22: Verify Dependency Update in Documentation Environment
The inclusion ofpyiron_lammps =0.3.2ensures that the docs environment now supports the LAMMPS-based interfaces. This change is consistent across the project.binder/environment.yml (1)
18-18: Dependency Addition Verification
The new dependencypyiron_lammps =0.3.2is properly added in the binder environment file, ensuring consistency with the other environment configurations.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 18-18: no new line character at the end of file
(new-line-at-end-of-file)
tests/test_langevin_lammpslib.py (1)
12-12: LGTM! Consistent function renaming.The changes align with the broader effort to standardize LAMMPS-related function names across the codebase.
Also applies to: 52-52
tests/test_quasiharmonic_lammpslib_functional.py (1)
15-15: LGTM! Consistent function renaming.The changes align with the broader effort to standardize LAMMPS-related function names across the codebase.
Also applies to: 32-32, 48-48
tests/test_evcurve_lammpslib_functional.py (1)
14-14: LGTM! Consistent function renaming.The changes align with the broader effort to standardize LAMMPS-related function names across the codebase.
Also applies to: 31-31, 42-42
.ci_support/environment-old.yml (1)
12-13:❓ Verification inconclusive
Verify compatibility with numpy 1.26.0.
The update to numpy 1.26.0 is a significant version jump. Please ensure compatibility with other dependencies.
🌐 Web query:
What are the known compatibility issues with numpy 1.26.0?💡 Result:
NumPy 1.26.0 introduces several compatibility considerations across different environments and dependencies:
Build System Compatibility
The switch from
distutils/setup.pyto Meson build system creates breaking changes:
- Environment variables like
NPY_*for BLAS/LAPACK/SIMD configuration no longer work[1][3]- Custom
site.cfgfiles for BLAS/LAPACK selection are deprecated[3]- Requires new build flags via
pip config-settingsinterface[1]- Temporary workaround: Use
pyproject.toml.setuppyfor legacy builds (Python ≤3.11 only)[1][3]Python Version Constraints
- Officially supports Python 3.9-3.12[1][3]
- Blocks installation on Python ≥3.13 via version specifier
<3.13, causing conflicts in unconstrained environments[9]Platform-Specific Issues
Apple Silicon (M1/M2):
- Conda installations may fail with module recognition errors
- Solution: Use native Python.org install + pip3 instead of Conda[7]
Intel Environments:
- Potential binary incompatibility with older Intel Math Kernel Library (MKL) distributions[4]
- Error pattern:
ValueError: numpy.ufunc size changedindicating version mismatch[4]Dependency Conflicts
With Numerical Libraries:
- Gurobi's matrix API incompatible with NumPy 2.x when using v11.0.x (requires NumPy <2)[10]
- Workaround: Pin to
numpy<2until library updates[10]Common Installation Pitfalls:
# Error pattern when mixing package managers ImportError: Numpy not available (PyTorch DataLoader)[8] # Solution sequence pip uninstall numpy && pip install numpy==1.26.0[4][8]Key Recommendations
- Verify build customization uses Meson flags instead of legacy methods
- Constrain Python versions to <3.13 in environments using NumPy 1.26.x[9]
- Prefer pip over conda for ARM64 installations[7]
- Audit dependencies relying on NumPy C-API (e.g., ML libraries)[10]
These issues primarily stem from major infrastructure changes in NumPy's build system and expanded Python version support[1][3][9]. Most can be resolved through dependency pinning and build process updates[4][10].
Citations:
- 1: https://numpy.org/doc/2.1/release/1.26.0-notes.html
- 2: https://numpy.org/devdocs/user/how-to-verify-bug.html
- 3: https://numpy.org/doc/2.0/release/1.26.0-notes.html
- 4: https://community.intel.com/t5/Intel-Distribution-for-Python/numpy-incompatibility/td-p/1304098
- 5: https://community.home-assistant.io/t/compile-error-installing-numpy/107918
- 6: https://community.render.com/t/no-matching-distribution-found-for-numpy/15790
- 7: https://discuss.python.org/t/numpy-wont-run-on-apple-imac-m1-using-arm64/38335
- 8: https://discuss.pytorch.org/t/numpy-not-available/120409
- 9: ENH: Don't restrict the upper Python version or use "<4" numpy/numpy#24810
- 10: https://support.gurobi.com/hc/en-us/articles/25787048531601-Resolved-in-gurobipy-v11-0-3-Compatibility-issues-with-numpy-2-0
Ensure NumPy 1.26.0 Compatibility & Fix File Formatting
The upgrade to NumPy 1.26.0 is a major jump that introduces several compatibility considerations. Please verify the following before merging:
Build & Configuration:
Confirm that our CI and build configurations do not rely on legacy settings (e.g.,NPY_*environment variables or customsite.cfgfiles) that are incompatible with NumPy’s new Meson build process.Python Version Constraints:
Ensure the environment uses Python 3.9–3.12 since NumPy 1.26.0 supports these versions and explicitly prevents installation on Python ≥3.13.Dependency Management:
Audit dependencies that might interact with NumPy’s C-API or have strict version requirements (e.g., potential conflicts with libraries like Gurobi) to avoid runtime issues.Formatting:
Address the minor formatting issue by ensuring there is a newline at the end of.ci_support/environment-old.yml.tests/test_evcurve_lammpslib.py (1)
13-13: LGTM! Consistent function renaming.The changes correctly update the import statement and function calls from
evaluate_with_lammpstoevaluate_with_lammpslib, maintaining consistency with the broader refactoring effort.Also applies to: 31-31, 45-45
tests/test_quasiharmonic_lammpslib.py (1)
10-10: LGTM! Consistent function renaming.The changes correctly update the import statement and function calls from
evaluate_with_lammpstoevaluate_with_lammpslib, maintaining consistency with the broader refactoring effort.Also applies to: 28-28, 44-44
tests/test_phonons_lammpslib.py (1)
10-10: LGTM! Consistent function renaming.The changes correctly update the import statement and function calls from
evaluate_with_lammpstoevaluate_with_lammpslib, maintaining consistency with the broader refactoring effort.Also applies to: 28-28, 42-42
tests/test_phonons_lammpslib_functional.py (1)
15-15: LGTM! Consistent function renaming.The changes correctly update the import statement and function calls from
evaluate_with_lammpstoevaluate_with_lammpslib, maintaining consistency with the broader refactoring effort.Also applies to: 32-32, 44-44
tests/test_elastic_lammpslib.py (1)
10-10: LGTM! Function renaming is consistent.The renaming from
evaluate_with_lammpstoevaluate_with_lammpslibis consistent with the PR objectives to update the naming convention.Also applies to: 28-28, 40-40
tests/test_elastic_lammpslib_functional.py (1)
13-13: LGTM! Function renaming is consistent.The renaming from
evaluate_with_lammpstoevaluate_with_lammpslibis consistent with the PR objectives to update the naming convention.Also applies to: 30-30, 41-41
pyproject.toml (1)
52-52: LGTM! New dependency added consistently.The addition of
pyiron_lammps==0.3.2to bothlammpsandlammps_phononssections is consistent and uses version pinning for reproducibility.Also applies to: 59-59
tests/test_evcurve_lammpslib_function_parallel.py (1)
16-16: LGTM! Function renaming is consistent.The renaming from
evaluate_with_lammpstoevaluate_with_lammpslibis consistent with the PR objectives to update the naming convention.Also applies to: 35-35, 48-48
tests/test_optimize_positions_lammpslib.py (1)
10-10: LGTM! Function renaming is consistent.The changes align with the PR's objective of standardizing LAMMPS function names across the codebase.
Also applies to: 23-23
tests/test_calc_stress_lammpslib.py (1)
10-10: LGTM! Function renaming is consistent across multiple calls.The changes maintain consistency in the LAMMPS function naming convention across all three function calls.
Also applies to: 28-28, 33-33, 40-40
tests/test_lammpslib_md_phonons.py (1)
12-13: LGTM! Function renaming is consistent.The changes maintain consistency in the LAMMPS function naming convention for both molecular dynamics and evaluation functions.
Also applies to: 42-42, 47-47
tests/test_molecular_dynamics_thermal_expansion_lammpslib.py (1)
78-100:❓ Verification inconclusive
Verify ASE calculator configuration.
The test uses hardcoded Morse potential parameters which might not be optimal for aluminum.
🌐 Web query:
What are the recommended Morse potential parameters for aluminum in LAMMPS?💡 Result:
The recommended Morse potential parameters for aluminum in LAMMPS depend on the specific application and interaction type (e.g., pure Al vs. cross-species interactions). Based on research papers and practical implementations:
For Al-Al interactions:
- EAM/alloy potentials are generally preferred over pairwise potentials like Morse due to their ability to model metallic bonding more accurately[1][5][6].
- If using a Morse potential explicitly:
- Parameters derived from experimental data or quantum calculations include:
- D₀ (well depth): ~0.4824 eV
- α (width): ~1.322 Å⁻¹
- r₀ (equilibrium distance): ~2.92 Å[8].
For cross-species interactions (e.g., Al-SiC interfaces):
- Example implementations use:
- Al-Si:
- D₀ = 0.4824 eV
- α = 1.322 Å⁻¹
- r₀ = 2.92 Å
- Al-C:
- D₀ = 0.4691 eV
- α = 1.738 Å⁻¹
- r₀ = 2.246 Å[8].
Key considerations:
EAM vs. Morse:
- EAM/alloy potentials (example) better capture metallic properties like thermal expansion and defect energies[1][6].
- Use
pair_style hybridwhen combining EAM/Al with other potentials[8].Parameter derivation:
- Parameters can be fitted using quantum calculations or experimental data (e.g., lattice constants)[4][6].
For detailed implementation examples in LAMMPS scripts involving hybrid potentials like
tersoff,eam/alloy, andmorse, see[8].Citations:
- 1: https://www.ctcms.nist.gov/potentials/system/Al/
- 2: https://matsci.org/t/oxidation-of-aluminum-with-streitz-mintmire-potential/31754
- 3: https://matsci.org/t/lammps-users-obtain-lj-potential-from-diatomic-interactions/36065
- 4: https://lammpstube.com/wp-content/uploads/2019/10/Calculation-of-Potential-Energy-Parameters-from-1.pdf
- 5: https://docs.lammps.org/pair_eam.html
- 6: https://www.researchgate.net/publication/230306563_Evaluation_of_morse_parameters_for_metals
- 7: https://docs.lammps.org/pair_morse.html
- 8: https://matsci.org/t/mode-ii-fracture-test-of-al-sic/49396
- 9: https://docs.lammps.org/Packages_details.html
- 10: https://matsci.org/t/lammps-users-obtain-lj-potential-from-diatomic-interactions/36065/4
Attention: Verify ASE Calculator Configuration
It appears that the hardcoded Morse potential parameters in the test case might not align with commonly recommended values for aluminum. Currently, the test is configured with:
- D₀: 0.5
- α: 1.8
- r₀: 2.95
According to recent literature and practical implementations for Al–Al interactions:
- D₀ should be around 0.4824 eV
- α should be near 1.322 Å⁻¹
- r₀ is recommended as approximately 2.92 Å
Please verify whether these differences are intentional for testing purposes. If not, consider updating the values or adding an explanatory comment to clarify why the current parameters were chosen.
atomistics/calculators/lammps/filecalculator.py (1)
1-25: Imports and constants look good.
These imports, constants, and configurations seem structured and appropriate for a file-based LAMMPS interface. No immediate concerns on correctness, security, or maintainability.
| def optimize_positions_and_volume_with_lammpsfile( | ||
| structure: Atoms, | ||
| potential_dataframe: pandas.DataFrame, | ||
| working_directory: str, | ||
| executable_function: callable, | ||
| min_style: str = "cg", | ||
| etol: float = 0.0, | ||
| ftol: float = 0.0001, | ||
| maxiter: int = 100000, | ||
| maxeval: int = 10000000, | ||
| thermo: int = 10, | ||
| ) -> Atoms: | ||
| template_str = ( | ||
| LAMMPS_MINIMIZE_VOLUME | ||
| + "\n" | ||
| + LAMMPS_THERMO_STYLE | ||
| + "\n" | ||
| + LAMMPS_THERMO | ||
| + "\n" | ||
| + LAMMPS_MINIMIZE | ||
| ) | ||
| input_template = Template(template_str).render( | ||
| min_style=min_style, | ||
| etol=etol, | ||
| ftol=ftol, | ||
| maxiter=maxiter, | ||
| maxeval=maxeval, | ||
| thermo=thermo, | ||
| ) | ||
| _write_lammps_input_file( | ||
| working_directory=working_directory, | ||
| structure=structure, | ||
| potential_dataframe=potential_dataframe, | ||
| input_template=input_template, | ||
| ) | ||
| executable_function(working_directory) | ||
| output = _parse_lammps_output( | ||
| working_directory=working_directory, | ||
| structure=structure, | ||
| potential_elements=potential_dataframe["Species"], | ||
| units="metal", | ||
| prism=None, | ||
| dump_h5_file_name="dump.h5", | ||
| dump_out_file_name="dump.out", | ||
| log_lammps_file_name="log.lammps", | ||
| ) | ||
| structure_copy = structure.copy() | ||
| structure_copy.set_cell(output["generic"]["cells"][-1], scale_atoms=True) | ||
| structure_copy.positions = output["generic"]["positions"][-1] | ||
| return structure_copy | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential empty output errors.
optimize_positions_and_volume_with_lammpsfile() (lines 78–128) sets the cell/positions from the last step of the parsed output. If the output is empty (e.g., if LAMMPS fails), an IndexError or KeyError may occur. Consider gracefully handling or raising a meaningful exception.
| def optimize_positions_with_lammpsfile( | ||
| structure: Atoms, | ||
| potential_dataframe: pandas.DataFrame, | ||
| working_directory: str, | ||
| executable_function: callable, | ||
| min_style: str = "cg", | ||
| etol: float = 0.0, | ||
| ftol: float = 0.0001, | ||
| maxiter: int = 100000, | ||
| maxeval: int = 10000000, | ||
| thermo: int = 10, | ||
| ) -> Atoms: | ||
| template_str = LAMMPS_THERMO_STYLE + "\n" + LAMMPS_THERMO + "\n" + LAMMPS_MINIMIZE | ||
| input_template = Template(template_str).render( | ||
| min_style=min_style, | ||
| etol=etol, | ||
| ftol=ftol, | ||
| maxiter=maxiter, | ||
| maxeval=maxeval, | ||
| thermo=thermo, | ||
| ) | ||
| _write_lammps_input_file( | ||
| working_directory=working_directory, | ||
| structure=structure, | ||
| potential_dataframe=potential_dataframe, | ||
| input_template=input_template, | ||
| ) | ||
| executable_function(working_directory) | ||
| output = _parse_lammps_output( | ||
| working_directory=working_directory, | ||
| structure=structure, | ||
| potential_elements=potential_dataframe["Species"], | ||
| units="metal", | ||
| prism=None, | ||
| dump_h5_file_name="dump.h5", | ||
| dump_out_file_name="dump.out", | ||
| log_lammps_file_name="log.lammps", | ||
| ) | ||
| structure_copy = structure.copy() | ||
| structure_copy.positions = output["generic"]["positions"][-1] | ||
| return structure_copy | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Similar error handling consideration.
optimize_positions_with_lammpsfile() (lines 130–171) suffers from the same potential empty output scenario as the previous function. Adding the same protective checks is advisable.
| def evaluate_lammps(working_directory): | ||
| command = "mpiexec -n 1 --oversubscribe lmp_mpi -in lmp.in" | ||
| output = subprocess.check_output( | ||
| command, cwd=working_directory, shell=True, universal_newlines=True, env=os.environ.copy() | ||
| ) | ||
| print(output) | ||
| return output |
There was a problem hiding this comment.
Address security and robustness concerns in LAMMPS execution.
The current implementation has several issues:
- Using
shell=Trueis a security risk as it's vulnerable to shell injection - Hard-coded command string reduces flexibility
- Missing error handling for subprocess failures
def evaluate_lammps(working_directory):
- command = "mpiexec -n 1 --oversubscribe lmp_mpi -in lmp.in"
- output = subprocess.check_output(
- command, cwd=working_directory, shell=True, universal_newlines=True, env=os.environ.copy()
- )
- print(output)
- return output
+ try:
+ command = ["mpiexec", "-n", "1", "--oversubscribe", "lmp_mpi", "-in", "lmp.in"]
+ output = subprocess.check_output(
+ command,
+ cwd=working_directory,
+ shell=False,
+ text=True,
+ env=os.environ.copy(),
+ stderr=subprocess.PIPE
+ )
+ print(output)
+ return output
+ except subprocess.CalledProcessError as e:
+ print(f"LAMMPS execution failed with return code {e.returncode}")
+ print(f"Error output: {e.stderr}")
+ raise📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def evaluate_lammps(working_directory): | |
| command = "mpiexec -n 1 --oversubscribe lmp_mpi -in lmp.in" | |
| output = subprocess.check_output( | |
| command, cwd=working_directory, shell=True, universal_newlines=True, env=os.environ.copy() | |
| ) | |
| print(output) | |
| return output | |
| def evaluate_lammps(working_directory): | |
| try: | |
| command = ["mpiexec", "-n", "1", "--oversubscribe", "lmp_mpi", "-in", "lmp.in"] | |
| output = subprocess.check_output( | |
| command, | |
| cwd=working_directory, | |
| shell=False, | |
| text=True, | |
| env=os.environ.copy(), | |
| stderr=subprocess.PIPE | |
| ) | |
| print(output) | |
| return output | |
| except subprocess.CalledProcessError as e: | |
| print(f"LAMMPS execution failed with return code {e.returncode}") | |
| print(f"Error output: {e.stderr}") | |
| raise |
| def optimize_structure(structure, potential_name, working_directory, resource_path=None): | ||
| df_pot_selected = get_potential_by_name( | ||
| potential_name=potential_name, | ||
| resource_path=resource_path, | ||
| ) | ||
| task_dict = optimize_positions(structure=structure) | ||
| result_dict = evaluate_with_lammpsfile( | ||
| task_dict=task_dict, | ||
| potential_dataframe=df_pot_selected, | ||
| working_directory=working_directory, | ||
| executable_function=evaluate_lammps, | ||
| lmp_optimizer_kwargs={"ftol": 0.000001}, | ||
| ) | ||
| return result_dict["structure_with_optimized_positions"] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling to the optimization function.
The function lacks input validation and error handling for potential retrieval failures.
def optimize_structure(structure, potential_name, working_directory, resource_path=None):
+ if not structure or not potential_name or not working_directory:
+ raise ValueError("structure, potential_name, and working_directory are required")
+
+ if not os.path.isdir(working_directory):
+ raise ValueError(f"working_directory '{working_directory}' does not exist")
+
df_pot_selected = get_potential_by_name(
potential_name=potential_name,
resource_path=resource_path,
)
+
+ if df_pot_selected is None or df_pot_selected.empty:
+ raise ValueError(f"Potential '{potential_name}' not found")
+
task_dict = optimize_positions(structure=structure)
result_dict = evaluate_with_lammpsfile(
task_dict=task_dict,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def optimize_structure(structure, potential_name, working_directory, resource_path=None): | |
| df_pot_selected = get_potential_by_name( | |
| potential_name=potential_name, | |
| resource_path=resource_path, | |
| ) | |
| task_dict = optimize_positions(structure=structure) | |
| result_dict = evaluate_with_lammpsfile( | |
| task_dict=task_dict, | |
| potential_dataframe=df_pot_selected, | |
| working_directory=working_directory, | |
| executable_function=evaluate_lammps, | |
| lmp_optimizer_kwargs={"ftol": 0.000001}, | |
| ) | |
| return result_dict["structure_with_optimized_positions"] | |
| def optimize_structure(structure, potential_name, working_directory, resource_path=None): | |
| if not structure or not potential_name or not working_directory: | |
| raise ValueError("structure, potential_name, and working_directory are required") | |
| if not os.path.isdir(working_directory): | |
| raise ValueError(f"working_directory '{working_directory}' does not exist") | |
| df_pot_selected = get_potential_by_name( | |
| potential_name=potential_name, | |
| resource_path=resource_path, | |
| ) | |
| if df_pot_selected is None or df_pot_selected.empty: | |
| raise ValueError(f"Potential '{potential_name}' not found") | |
| task_dict = optimize_positions(structure=structure) | |
| result_dict = evaluate_with_lammpsfile( | |
| task_dict=task_dict, | |
| potential_dataframe=df_pot_selected, | |
| working_directory=working_directory, | |
| executable_function=evaluate_lammps, | |
| lmp_optimizer_kwargs={"ftol": 0.000001}, | |
| ) | |
| return result_dict["structure_with_optimized_positions"] |
| @unittest.skipIf( | ||
| skip_lammps_test, "LAMMPS is not installed, so the LAMMPS tests are skipped." | ||
| ) | ||
| class TestOptimizePositionsLAMMPS(unittest.TestCase): | ||
| def setUp(self): | ||
| self.working_directory = os.path.abspath(os.path.join(__file__, "..", "lammps")) | ||
| os.makedirs(self.working_directory, exist_ok=True) | ||
|
|
||
| def tearDown(self): | ||
| if os.path.exists(self.working_directory): | ||
| shutil.rmtree(self.working_directory) | ||
|
|
||
| def test_optimize_positions_with_resource_path(self): | ||
| structure = bulk("Al", cubic=True) | ||
| positions_before_displacement = structure.positions.copy() | ||
| structure.positions[0] += [0.01, 0.01, 0.01] | ||
| structure_optimized = optimize_structure( | ||
| structure=structure, | ||
| potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1", | ||
| resource_path=os.path.join(os.path.dirname(__file__), "static", "lammps"), | ||
| working_directory=self.working_directory, | ||
| ) | ||
| self.assertTrue( | ||
| all( | ||
| np.isclose( | ||
| positions_before_displacement, | ||
| structure_optimized.positions - structure_optimized.positions[0], | ||
| ).flatten() | ||
| ) | ||
| ) | ||
|
|
||
| def test_optimize_positions_without_resource_path(self): | ||
| structure = bulk("Al", cubic=True) | ||
| positions_before_displacement = structure.positions.copy() | ||
| structure.positions[0] += [0.01, 0.01, 0.01] | ||
| structure_optimized = optimize_structure( | ||
| structure=structure, | ||
| potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1", | ||
| resource_path=None, | ||
| working_directory=self.working_directory, | ||
| ) | ||
| self.assertTrue( | ||
| all( | ||
| np.isclose( | ||
| positions_before_displacement, | ||
| structure_optimized.positions - structure_optimized.positions[0], | ||
| ).flatten() | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve test coverage and reduce code duplication.
The test class has several areas for improvement:
- Test methods share identical logic, leading to code duplication
- Missing edge case tests (invalid potentials, invalid structures, etc.)
- Test data path construction could be more robust
Consider refactoring the tests to reduce duplication and improve coverage:
@unittest.skipIf(
skip_lammps_test, "LAMMPS is not installed, so the LAMMPS tests are skipped."
)
class TestOptimizePositionsLAMMPS(unittest.TestCase):
def setUp(self):
self.working_directory = os.path.abspath(os.path.join(__file__, "..", "lammps"))
+ self.test_data_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "static", "lammps"))
os.makedirs(self.working_directory, exist_ok=True)
+ self.structure = bulk("Al", cubic=True)
+ self.positions_before_displacement = self.structure.positions.copy()
+ self.structure.positions[0] += [0.01, 0.01, 0.01]
def tearDown(self):
if os.path.exists(self.working_directory):
shutil.rmtree(self.working_directory)
- def test_optimize_positions_with_resource_path(self):
- structure = bulk("Al", cubic=True)
- positions_before_displacement = structure.positions.copy()
- structure.positions[0] += [0.01, 0.01, 0.01]
+ def _run_optimization(self, resource_path=None):
structure_optimized = optimize_structure(
- structure=structure,
+ structure=self.structure,
potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1",
- resource_path=os.path.join(os.path.dirname(__file__), "static", "lammps"),
+ resource_path=resource_path,
working_directory=self.working_directory,
)
self.assertTrue(
all(
np.isclose(
- positions_before_displacement,
+ self.positions_before_displacement,
structure_optimized.positions - structure_optimized.positions[0],
).flatten()
)
)
+ return structure_optimized
+ def test_optimize_positions_with_resource_path(self):
+ self._run_optimization(resource_path=self.test_data_path)
+
def test_optimize_positions_without_resource_path(self):
- structure = bulk("Al", cubic=True)
- positions_before_displacement = structure.positions.copy()
- structure.positions[0] += [0.01, 0.01, 0.01]
- structure_optimized = optimize_structure(
- structure=structure,
- potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1",
- resource_path=None,
- working_directory=self.working_directory,
- )
- self.assertTrue(
- all(
- np.isclose(
- positions_before_displacement,
- structure_optimized.positions - structure_optimized.positions[0],
- ).flatten()
- )
- )
+ self._run_optimization()
+
+ def test_invalid_potential(self):
+ with self.assertRaises(ValueError):
+ optimize_structure(
+ structure=self.structure,
+ potential_name="non_existent_potential",
+ working_directory=self.working_directory,
+ )
+
+ def test_invalid_structure(self):
+ with self.assertRaises(ValueError):
+ optimize_structure(
+ structure=None,
+ potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1",
+ working_directory=self.working_directory,
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @unittest.skipIf( | |
| skip_lammps_test, "LAMMPS is not installed, so the LAMMPS tests are skipped." | |
| ) | |
| class TestOptimizePositionsLAMMPS(unittest.TestCase): | |
| def setUp(self): | |
| self.working_directory = os.path.abspath(os.path.join(__file__, "..", "lammps")) | |
| os.makedirs(self.working_directory, exist_ok=True) | |
| def tearDown(self): | |
| if os.path.exists(self.working_directory): | |
| shutil.rmtree(self.working_directory) | |
| def test_optimize_positions_with_resource_path(self): | |
| structure = bulk("Al", cubic=True) | |
| positions_before_displacement = structure.positions.copy() | |
| structure.positions[0] += [0.01, 0.01, 0.01] | |
| structure_optimized = optimize_structure( | |
| structure=structure, | |
| potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1", | |
| resource_path=os.path.join(os.path.dirname(__file__), "static", "lammps"), | |
| working_directory=self.working_directory, | |
| ) | |
| self.assertTrue( | |
| all( | |
| np.isclose( | |
| positions_before_displacement, | |
| structure_optimized.positions - structure_optimized.positions[0], | |
| ).flatten() | |
| ) | |
| ) | |
| def test_optimize_positions_without_resource_path(self): | |
| structure = bulk("Al", cubic=True) | |
| positions_before_displacement = structure.positions.copy() | |
| structure.positions[0] += [0.01, 0.01, 0.01] | |
| structure_optimized = optimize_structure( | |
| structure=structure, | |
| potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1", | |
| resource_path=None, | |
| working_directory=self.working_directory, | |
| ) | |
| self.assertTrue( | |
| all( | |
| np.isclose( | |
| positions_before_displacement, | |
| structure_optimized.positions - structure_optimized.positions[0], | |
| ).flatten() | |
| ) | |
| ) | |
| @unittest.skipIf( | |
| skip_lammps_test, "LAMMPS is not installed, so the LAMMPS tests are skipped." | |
| ) | |
| class TestOptimizePositionsLAMMPS(unittest.TestCase): | |
| def setUp(self): | |
| self.working_directory = os.path.abspath(os.path.join(__file__, "..", "lammps")) | |
| self.test_data_path = os.path.abspath(os.path.join(os.path.dirname(__file__), "static", "lammps")) | |
| os.makedirs(self.working_directory, exist_ok=True) | |
| self.structure = bulk("Al", cubic=True) | |
| self.positions_before_displacement = self.structure.positions.copy() | |
| self.structure.positions[0] += [0.01, 0.01, 0.01] | |
| def tearDown(self): | |
| if os.path.exists(self.working_directory): | |
| shutil.rmtree(self.working_directory) | |
| def _run_optimization(self, resource_path=None): | |
| structure_optimized = optimize_structure( | |
| structure=self.structure, | |
| potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1", | |
| resource_path=resource_path, | |
| working_directory=self.working_directory, | |
| ) | |
| self.assertTrue( | |
| all( | |
| np.isclose( | |
| self.positions_before_displacement, | |
| structure_optimized.positions - structure_optimized.positions[0], | |
| ).flatten() | |
| ) | |
| ) | |
| return structure_optimized | |
| def test_optimize_positions_with_resource_path(self): | |
| self._run_optimization(resource_path=self.test_data_path) | |
| def test_optimize_positions_without_resource_path(self): | |
| self._run_optimization() | |
| def test_invalid_potential(self): | |
| with self.assertRaises(ValueError): | |
| optimize_structure( | |
| structure=self.structure, | |
| potential_name="non_existent_potential", | |
| working_directory=self.working_directory, | |
| ) | |
| def test_invalid_structure(self): | |
| with self.assertRaises(ValueError): | |
| optimize_structure( | |
| structure=None, | |
| potential_name="1999--Mishin-Y--Al--LAMMPS--ipr1", | |
| working_directory=self.working_directory, | |
| ) |
Summary by CodeRabbit