Migrate potential interface from atomistics to pyiron_lammps#261
Migrate potential interface from atomistics to pyiron_lammps#261jan-janssen merged 22 commits intomainfrom
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughAdds a new potential management module for LAMMPS (pyiron_lammps.potential), exposes two functions via the package init, and includes unit tests exercising dataframe loading, path resolution, potential lookup, and abstract/concrete potential classes. Changes
Sequence DiagramsequenceDiagram
participant User
participant API as get_potential_dataframe / get_potential_by_name
participant Conda as get_resource_path_from_conda
participant Loader as _get_potential_df / find_potential_file_base
participant Updater as update_potential_paths
participant DF as Potentials DataFrame
participant Finder as PotentialAbstract / LammpsPotentialFile
User->>API: call(structure or name, resource_path?)
API->>Conda: if no resource_path -> query env
Conda-->>API: return resource_path
API->>Loader: load CSV(s) (lazy if needed)
Loader-->>DF: return raw dataframe
API->>Updater: expand Filename -> absolute POSIX paths
Updater-->>DF: return enriched dataframe
API->>Finder: filter/find potentials (by elements or name)
Finder-->>API: return filtered dataframe / series
API-->>User: return potential(s)
Note over Loader,Updater: path resolution and CSV converters (Species, Config, Filename)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
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 |
for more information, see https://pre-commit.ci
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #261 +/- ##
==========================================
+ Coverage 95.17% 95.76% +0.59%
==========================================
Files 4 5 +1
Lines 497 637 +140
==========================================
+ Hits 473 610 +137
- Misses 24 27 +3 ☔ 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: 5
🧹 Nitpick comments (5)
pyiron_lammps/potential.py (5)
49-60: Consider explicitOptionaltype annotation.Line 53 uses
list[str] = Nonewhich implicitly creates an Optional type. PEP 484 recommends explicitOptional[list[str]]for clarity.Apply this diff if desired:
def __init__( self, potential_df: pandas.DataFrame, default_df: pandas.DataFrame = None, - selected_atoms: list[str] = None, + selected_atoms: Optional[list[str]] = None, ):
104-111: Consider spread operator for list construction.Line 106 concatenates lists using
+. The spread operator provides a more explicit alternative.Apply this diff if desired:
- selected_atoms = self._selected_atoms + [item] + selected_atoms = [*self._selected_atoms, item]
223-231: Consider spread operator for list construction.Similar to the parent class, line 225 could use the spread operator for list construction.
Apply this diff if desired:
- selected_atoms = self._selected_atoms + [item] + selected_atoms = [*self._selected_atoms, item]
287-304: Remove redundant backslash replacement.Line 303 calls
.replace("\\", "/")after.as_posix(), butas_posix()already converts all path separators to forward slashes, making the replacement redundant.Apply this diff:
return ( Path(path.strip()) .expanduser() .resolve() .absolute() .as_posix() - .replace("\\", "/") )
358-380: Consider shorter error messages or custom exception classes.The validation logic is comprehensive and correct. However, the error messages on lines 367-370, 372-375, and 377-380 are quite verbose. While they provide helpful context, consider either:
- Creating custom exception classes with descriptive names, or
- Moving detailed messages to exception docstrings
This aligns with best practices for exception handling (TRY003).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyiron_lammps/__init__.py(1 hunks)pyiron_lammps/potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pyiron_lammps/__init__.py (1)
pyiron_lammps/potential.py (2)
get_potential_by_name(349-355)get_potential_dataframe(340-346)
🪛 Ruff (0.14.3)
pyiron_lammps/potential.py
53-53: PEP 484 prohibits implicit Optional
Convert to Optional[T]
(RUF013)
78-78: Avoid specifying long messages outside the exception class
(TRY003)
89-89: Avoid specifying long messages outside the exception class
(TRY003)
106-106: Consider [*self._selected_atoms, item] instead of concatenation
Replace with [*self._selected_atoms, item]
(RUF005)
213-213: Avoid specifying long messages outside the exception class
(TRY003)
225-225: Consider [*self._selected_atoms, item] instead of concatenation
Replace with [*self._selected_atoms, item]
(RUF005)
263-267: Avoid specifying long messages outside the exception class
(TRY003)
367-370: Avoid specifying long messages outside the exception class
(TRY003)
372-375: Avoid specifying long messages outside the exception class
(TRY003)
377-380: 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)
| potential_installation = """ | ||
| Potential installation guide: | ||
|
|
||
| 1. Check whether iprpy-data is installed. If not, install it using: | ||
|
|
||
| `conda install -c conda-forge iprpy-data` | ||
|
|
||
| 2. Check whether the resource path is set via: | ||
|
|
||
| ```python | ||
| import os | ||
| print(os.environ["CONDA_PREFIX"]) | ||
| ``` | ||
|
|
||
| 3. If the resource path is set, you can call the potential using: | ||
|
|
||
| ```python | ||
| from atomistics.calculators import get_potential_by_name | ||
|
|
||
|
|
||
| get_potential_by_name( | ||
| potential_name=my_potential, | ||
| resource_path=os.path.join(os.environ["CONDA_PREFIX"], "share", "iprpy"), | ||
| ) | ||
| ``` | ||
|
|
||
| """ |
There was a problem hiding this comment.
Update module reference in installation guide.
The installation guide references atomistics.calculators on line 25, but this PR migrates the interface to pyiron_lammps. The import statement should be updated to reflect the correct module name.
Apply this diff:
-from atomistics.calculators import get_potential_by_name
+from pyiron_lammps import get_potential_by_name📝 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.
| potential_installation = """ | |
| Potential installation guide: | |
| 1. Check whether iprpy-data is installed. If not, install it using: | |
| `conda install -c conda-forge iprpy-data` | |
| 2. Check whether the resource path is set via: | |
| ```python | |
| import os | |
| print(os.environ["CONDA_PREFIX"]) | |
| ``` | |
| 3. If the resource path is set, you can call the potential using: | |
| ```python | |
| from atomistics.calculators import get_potential_by_name | |
| get_potential_by_name( | |
| potential_name=my_potential, | |
| resource_path=os.path.join(os.environ["CONDA_PREFIX"], "share", "iprpy"), | |
| ) | |
| ``` | |
| """ | |
| potential_installation = """ | |
| Potential installation guide: | |
| 1. Check whether iprpy-data is installed. If not, install it using: | |
| `conda install -c conda-forge iprpy-data` | |
| 2. Check whether the resource path is set via: | |
🤖 Prompt for AI Agents
In pyiron_lammps/potential.py around lines 8 to 34, the installation guide
string references the old module name "atomistics.calculators" on the import
example; update that import to the new module "pyiron_lammps" and adjust the
subsequent usage example to call get_potential_by_name from pyiron_lammps (i.e.,
replace the import line and any fully-qualified calls in the string), keeping
the rest of the textual instructions unchanged.
| def default(self): | ||
| if self._default_df is not None: | ||
| atoms_str = "_".join(sorted(self._selected_atoms)) | ||
| return self._default_df[ | ||
| (self._default_df["Name"] == self._default_df.loc[atoms_str].values[0]) | ||
| ] | ||
| return None |
There was a problem hiding this comment.
Add error handling for missing default potential.
Line 190 accesses self._default_df.loc[atoms_str] without checking if atoms_str exists in the index. This will raise a KeyError if the element combination has no default defined.
Apply this diff to add proper error handling:
def default(self):
if self._default_df is not None:
atoms_str = "_".join(sorted(self._selected_atoms))
+ if atoms_str not in self._default_df.index:
+ return None
return self._default_df[
(self._default_df["Name"] == self._default_df.loc[atoms_str].values[0])
]
return None🤖 Prompt for AI Agents
In pyiron_lammps/potential.py around lines 186 to 192, the code directly indexes
self._default_df.loc[atoms_str] which will raise a KeyError if atoms_str is not
present; update the method to first check whether self._default_df is not None
and atoms_str exists in self._default_df.index (or catch KeyError) and if
missing return None (or raise a clear ValueError with context), otherwise
proceed to select rows matching the stored default Name; ensure no unguarded
.loc access remains so the method never throws an uncaught KeyError.
| def find_default( | ||
| self, element: Union[set[str], list[str], str] | ||
| ) -> pandas.DataFrame: | ||
| """ | ||
| Find the potentials | ||
|
|
||
| Args: | ||
| element (set, str): element or set of elements for which you want the possible LAMMPS potentials | ||
| path (bool): choose whether to return the full path to the potential or just the potential name | ||
|
|
||
| Returns: | ||
| list: of possible potentials for the element or the combination of elements | ||
|
|
||
| """ | ||
| if isinstance(element, list): | ||
| element = set(element) | ||
| elif isinstance(element, str): | ||
| element = {element} | ||
| elif not isinstance(element, set): | ||
| raise TypeError("Only, str, list and set supported!") | ||
| element_lst = list(element) | ||
| if self._default_df is not None: | ||
| merged_lst = list(set(self._selected_atoms + element_lst)) | ||
| atoms_str = "_".join(sorted(merged_lst)) | ||
| return self._default_df[ | ||
| (self._default_df["Name"] == self._default_df.loc[atoms_str].values[0]) | ||
| ] | ||
| return None |
There was a problem hiding this comment.
Add error handling for missing default potential.
Line 219 has the same issue as the default() method - accessing self._default_df.loc[atoms_str] without checking if the key exists in the index.
Apply this diff:
def find_default(
self, element: Union[set[str], list[str], str]
) -> pandas.DataFrame:
"""
Find the potentials
Args:
element (set, str): element or set of elements for which you want the possible LAMMPS potentials
path (bool): choose whether to return the full path to the potential or just the potential name
Returns:
list: of possible potentials for the element or the combination of elements
"""
if isinstance(element, list):
element = set(element)
elif isinstance(element, str):
element = {element}
elif not isinstance(element, set):
raise TypeError("Only, str, list and set supported!")
element_lst = list(element)
if self._default_df is not None:
merged_lst = list(set(self._selected_atoms + element_lst))
atoms_str = "_".join(sorted(merged_lst))
+ if atoms_str not in self._default_df.index:
+ return None
return self._default_df[
(self._default_df["Name"] == self._default_df.loc[atoms_str].values[0])
]
return None📝 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 find_default( | |
| self, element: Union[set[str], list[str], str] | |
| ) -> pandas.DataFrame: | |
| """ | |
| Find the potentials | |
| Args: | |
| element (set, str): element or set of elements for which you want the possible LAMMPS potentials | |
| path (bool): choose whether to return the full path to the potential or just the potential name | |
| Returns: | |
| list: of possible potentials for the element or the combination of elements | |
| """ | |
| if isinstance(element, list): | |
| element = set(element) | |
| elif isinstance(element, str): | |
| element = {element} | |
| elif not isinstance(element, set): | |
| raise TypeError("Only, str, list and set supported!") | |
| element_lst = list(element) | |
| if self._default_df is not None: | |
| merged_lst = list(set(self._selected_atoms + element_lst)) | |
| atoms_str = "_".join(sorted(merged_lst)) | |
| return self._default_df[ | |
| (self._default_df["Name"] == self._default_df.loc[atoms_str].values[0]) | |
| ] | |
| return None | |
| def find_default( | |
| self, element: Union[set[str], list[str], str] | |
| ) -> pandas.DataFrame: | |
| """ | |
| Find the potentials | |
| Args: | |
| element (set, str): element or set of elements for which you want the possible LAMMPS potentials | |
| path (bool): choose whether to return the full path to the potential or just the potential name | |
| Returns: | |
| list: of possible potentials for the element or the combination of elements | |
| """ | |
| if isinstance(element, list): | |
| element = set(element) | |
| elif isinstance(element, str): | |
| element = {element} | |
| elif not isinstance(element, set): | |
| raise TypeError("Only, str, list and set supported!") | |
| element_lst = list(element) | |
| if self._default_df is not None: | |
| merged_lst = list(set(self._selected_atoms + element_lst)) | |
| atoms_str = "_".join(sorted(merged_lst)) | |
| if atoms_str not in self._default_df.index: | |
| return None | |
| return self._default_df[ | |
| (self._default_df["Name"] == self._default_df.loc[atoms_str].values[0]) | |
| ] | |
| return None |
🧰 Tools
🪛 Ruff (0.14.3)
213-213: Avoid specifying long messages outside the exception class
(TRY003)
🤖 Prompt for AI Agents
In pyiron_lammps/potential.py around lines 194 to 221, the code accesses
self._default_df.loc[atoms_str] without verifying that atoms_str is a valid
index key; update the logic to check whether atoms_str exists in
self._default_df.index (or wrap the access in a try/except KeyError) before
attempting to read .loc; if the key is missing, return None (or raise a clear
KeyError with a descriptive message) instead of letting an unhandled exception
occur, ensuring behavior matches the existing default() method.
| def get_potential_by_name(potential_name: str, resource_path=None): | ||
| if resource_path is None: | ||
| resource_path = get_resource_path_from_conda() | ||
| df = LammpsPotentialFile(resource_path=resource_path).list() | ||
| return update_potential_paths( | ||
| df_pot=df[df.Name == potential_name], resource_path=resource_path | ||
| ).iloc[0] |
There was a problem hiding this comment.
Add validation before accessing filtered result.
Line 354 filters by potential_name and immediately accesses .iloc[0] without checking if any results were found. If no matching potential exists, this will raise an IndexError instead of a meaningful error message.
Apply this diff:
def get_potential_by_name(potential_name: str, resource_path=None):
if resource_path is None:
resource_path = get_resource_path_from_conda()
df = LammpsPotentialFile(resource_path=resource_path).list()
- return update_potential_paths(
- df_pot=df[df.Name == potential_name], resource_path=resource_path
- ).iloc[0]
+ filtered_df = df[df.Name == potential_name]
+ if len(filtered_df) == 0:
+ raise ValueError(f"Potential '{potential_name}' not found in database.")
+ return update_potential_paths(
+ df_pot=filtered_df, resource_path=resource_path
+ ).iloc[0]🤖 Prompt for AI Agents
pyiron_lammps/potential.py around lines 349 to 355: the function filters df by
potential_name and directly returns .iloc[0], which will raise IndexError if no
match is found; modify the function to check if the filtered dataframe is empty
before accessing iloc[0], and if empty raise a clear ValueError (or custom
exception) stating the requested potential_name was not found and include either
a short list of available potential names or the resource_path used for lookup
so callers can diagnose the issue; if not empty, proceed to return the first row
as before.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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 (2)
pyiron_lammps/__init__.py(2 hunks)tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_potential.py (1)
pyiron_lammps/potential.py (2)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)
pyiron_lammps/__init__.py (1)
pyiron_lammps/potential.py (2)
get_potential_by_name(349-355)get_potential_dataframe(340-346)
🪛 GitHub Actions: Pipeline
tests/test_potential.py
[error] 28-28: test_get_potential_dataframe failed due to a ValueError raised in potential.py (missing potential files).
🔇 Additional comments (2)
pyiron_lammps/__init__.py (1)
3-3: LGTM! Public API correctly expanded.The new potential management functions are properly imported and exported via
__all__, making them part of the public API alongside existing exports.Also applies to: 19-20
tests/test_potential.py (1)
13-25: LGTM! Comprehensive validation test coverage.The test method correctly validates all error paths (empty DataFrame, multiple rows, wrong type) and the success path (single-row DataFrame returning Series).
| resource_path=os.path.abspath( | ||
| os.path.join("..", os.path.dirname(__file__), "static", "lammps") | ||
| ), |
There was a problem hiding this comment.
Fix the resource path construction causing test failure.
The path construction includes an extra ".." that navigates up from the repository root, causing the test to look for resources at ../tests/static/lammps instead of tests/static/lammps. This is why the pipeline reports missing potential files.
Apply this diff to fix the path:
- resource_path=os.path.abspath(
- os.path.join("..", os.path.dirname(__file__), "static", "lammps")
- ),
+ resource_path=os.path.abspath(
+ os.path.join(os.path.dirname(__file__), "static", "lammps")
+ ),📝 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.
| resource_path=os.path.abspath( | |
| os.path.join("..", os.path.dirname(__file__), "static", "lammps") | |
| ), | |
| resource_path=os.path.abspath( | |
| os.path.join(os.path.dirname(__file__), "static", "lammps") | |
| ), |
🤖 Prompt for AI Agents
In tests/test_potential.py around lines 30 to 32, the resource_path incorrectly
includes an extra ".." which causes it to resolve to ../tests/static/lammps;
remove the leading ".." from the join so the path is built relative to the test
file itself—for example, use
os.path.abspath(os.path.join(os.path.dirname(__file__), "static", "lammps"))—so
the tests look for tests/static/lammps instead of ../tests/static/lammps.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tests/test_potential.py (1)
27-28: Fix the resource path construction causing test failures.The path construction includes an extra
".."that navigates up from the repository root. This is causing multiple test failures in the pipeline.Apply this diff to fix the path:
- cls.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ) + cls.resource_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + )
🧹 Nitpick comments (1)
tests/test_potential.py (1)
69-77: Usetempfilefor cross-platform compatibility.The hardcoded
/tmp/condapath will fail on Windows and is flagged by static analysis. Usetempfile.TemporaryDirectory()for a portable solution.Apply this diff:
- @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True) - def test_get_resource_path_from_conda(self): - with self.assertRaises(ValueError): - get_resource_path_from_conda() - - os.makedirs("/tmp/conda/share/iprpy") - self.assertEqual(get_resource_path_from_conda(), "/tmp/conda/share/iprpy") - shutil.rmtree("/tmp/conda") + def test_get_resource_path_from_conda(self): + with tempfile.TemporaryDirectory() as tmpdir: + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + with self.assertRaises(ValueError): + get_resource_path_from_conda() + + conda_path = os.path.join(tmpdir, "share", "iprpy") + os.makedirs(conda_path) + self.assertEqual(get_resource_path_from_conda(), conda_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_potential.py (1)
pyiron_lammps/potential.py (16)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)get_potential_by_name(349-355)convert_path_to_abs_posix(287-304)get_resource_path_from_conda(328-337)find_potential_file_base(254-267)view_potentials(270-284)update_potential_paths(307-325)PotentialAbstract(37-153)LammpsPotentialFile(156-231)PotentialAvailable(234-251)find(62-84)find_by_name(86-90)_get_potential_df(117-153)find_default(194-221)default(186-192)
🪛 GitHub Actions: Pipeline
tests/test_potential.py
[error] 217-217: Assertion failed: len(potential.list()) == 1 but got 0; list() returns no items when exactly one item is expected.
[error] 140-140: Assertion failed: Updated potential paths do not include the resource_path in df_updated.Config.values[0][0].
[error] 233-233: Directory listing mismatch: expected ['pot_pot1', 'pot_pot_2', 'pot_pot_3'] but got ['pot_1', 'pot_2', 'pot_3'].
🪛 Ruff (0.14.4)
tests/test_potential.py
69-69: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
74-74: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
76-76: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
⏰ 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). (10)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.12)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.11)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.14)
- GitHub Check: unittest_matrix (ubuntu-latest, 3.13)
- GitHub Check: pip_check
- GitHub Check: unittest_old
- GitHub Check: minimal
- GitHub Check: notebooks
- GitHub Check: coverage
🔇 Additional comments (21)
tests/test_potential.py (21)
1-21: LGTM!The imports are appropriate and comprehensive for testing the potential module.
35-48: LGTM!The test comprehensively covers all validation branches: empty dataframe, multi-row dataframe, invalid type, and valid single-row input.
49-51: LGTM!Simple and effective test for dataframe retrieval. This will pass once the path construction issue in
setUpClassis resolved.
52-58: LGTM!The test correctly verifies retrieval of a specific potential by name.
59-67: LGTM!The test effectively validates path conversion including home directory expansion and POSIX formatting.
78-81: LGTM!The test correctly verifies that
ValueErroris raised when no conda environment variables are present.
83-129: LGTM!Excellent test coverage for
find_potential_file_base, including nested paths, direct paths, and error cases. Proper use oftempfile.TemporaryDirectory()for cleanup.
130-136: LGTM!The test appropriately validates the view_potentials function for a bulk Al structure.
137-141: Test logic is correct but fails due to path issue.The pipeline failure on line 140 is caused by the incorrect resource path construction in
setUpClass. Once that issue is fixed, this test should pass.
144-151: LGTM!Clean test setup with appropriate test data for exercising
PotentialAbstract.
153-171: LGTM!Thorough test coverage for the
findmethod, including string, list, set inputs and type error handling.
172-178: LGTM!The test correctly validates both successful lookup and error handling for missing potentials.
180-181: LGTM!Simple and effective test for the
list()method.
183-186: LGTM!The test effectively validates chained element selection through
__getitem__.
188-189: LGTM!Correct validation of the
__str__method.
191-195: LGTM!The test appropriately validates error handling when potential files cannot be located.
205-207: LGTM!The test correctly validates the inheritance hierarchy.
209-212: LGTM!The test appropriately verifies behavior when no default dataframe is configured.
214-218: Test logic is correct but fails due to path issue.The pipeline failure on line 217 (expecting 1 item but getting 0) is caused by the incorrect resource path in
setUp. Once the path construction is fixed, this test should pass.
221-223: LGTM!The setup correctly initializes
PotentialAvailablewith test data.
237-238: LGTM!The test correctly validates that
__repr__returns the string representation of the directory listing. This will work correctly oncetest_diris fixed.
| self.resource_path = os.path.abspath( | ||
| os.path.join("..", os.path.dirname(__file__), "static", "potential") |
There was a problem hiding this comment.
Fix the resource path construction.
The same path construction error from TestPotential.setUpClass is repeated here. The extra ".." will cause tests to fail.
Apply this diff:
- self.resource_path = os.path.abspath(
- os.path.join("..", os.path.dirname(__file__), "static", "potential")
- )
+ self.resource_path = os.path.abspath(
+ os.path.join(os.path.dirname(__file__), "static", "potential")
+ )📝 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.
| self.resource_path = os.path.abspath( | |
| os.path.join("..", os.path.dirname(__file__), "static", "potential") | |
| self.resource_path = os.path.abspath( | |
| os.path.join(os.path.dirname(__file__), "static", "potential") | |
| ) |
🤖 Prompt for AI Agents
In tests/test_potential.py around lines 200-201, the resource path construction
incorrectly includes an extra ".." segment which builds the wrong path; remove
the leading ".." and build the absolute path from the test file directory
instead (i.e., call os.path.abspath on os.path.join(os.path.dirname(__file__),
"static", "potential") ), making sure parentheses are balanced so the resulting
self.resource_path points to the correct static/potential directory.
| def test_getattr(self): | ||
| self.assertEqual(self.available.pot_1, "pot1") | ||
| self.assertEqual(self.available.pot_2, "pot-2") | ||
| self.assertEqual(self.available.pot_3, "pot.3") | ||
| with self.assertRaises(AttributeError): | ||
| _ = self.available.not_a_potential |
There was a problem hiding this comment.
Fix attribute access to match implementation.
The test expects pot_1, pot_2, pot_3, but PotentialAvailable prepends "pot_" to each potential name. For input ["pot1", "pot-2", "pot.3"], the keys should be pot_pot1, pot_pot_2, pot_pot_3.
Apply this diff:
def test_getattr(self):
- self.assertEqual(self.available.pot_1, "pot1")
- self.assertEqual(self.available.pot_2, "pot-2")
- self.assertEqual(self.available.pot_3, "pot.3")
+ self.assertEqual(self.available.pot_pot1, "pot1")
+ self.assertEqual(self.available.pot_pot_2, "pot-2")
+ self.assertEqual(self.available.pot_pot_3, "pot.3")
with self.assertRaises(AttributeError):
_ = self.available.not_a_potential🤖 Prompt for AI Agents
In tests/test_potential.py around lines 225 to 230, the assertions assume
attribute names pot_1, pot_2, pot_3 but PotentialAvailable prepends "pot_" to
each potential name, so for input ["pot1","pot-2","pot.3"] the actual attributes
are pot_pot1, pot_pot_2, pot_pot_3; update the three assertEqual lines to check
self.available.pot_pot1 == "pot1", self.available.pot_pot_2 == "pot-2",
self.available.pot_pot_3 == "pot.3" and keep the AttributeError check for an
attribute that truly doesn't exist (e.g., self.available.not_a_potential).
| def test_dir(self): | ||
| self.assertEqual( | ||
| sorted(dir(self.available)), sorted(["pot_1", "pot_2", "pot_3"]) | ||
| ) |
There was a problem hiding this comment.
Fix directory listing expectation.
The pipeline failure confirms that the expected keys should be ['pot_pot1', 'pot_pot_2', 'pot_pot_3'] to match the implementation, not ['pot_1', 'pot_2', 'pot_3'].
Apply this diff:
def test_dir(self):
self.assertEqual(
- sorted(dir(self.available)), sorted(["pot_1", "pot_2", "pot_3"])
+ sorted(dir(self.available)), sorted(["pot_pot1", "pot_pot_2", "pot_pot_3"])
)📝 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 test_dir(self): | |
| self.assertEqual( | |
| sorted(dir(self.available)), sorted(["pot_1", "pot_2", "pot_3"]) | |
| ) | |
| def test_dir(self): | |
| self.assertEqual( | |
| sorted(dir(self.available)), sorted(["pot_pot1", "pot_pot_2", "pot_pot_3"]) | |
| ) |
🧰 Tools
🪛 GitHub Actions: Pipeline
[error] 233-233: Directory listing mismatch: expected ['pot_pot1', 'pot_pot_2', 'pot_pot_3'] but got ['pot_1', 'pot_2', 'pot_3'].
🤖 Prompt for AI Agents
In tests/test_potential.py around lines 232 to 235, the test asserts the
directory listing uses keys ["pot_1","pot_2","pot_3"] but the implementation
produces ["pot_pot1","pot_pot_2","pot_pot_3"]; update the expected list in the
assertion to sorted(["pot_pot1","pot_pot_2","pot_pot_3"]) so the test matches
the actual keys returned by dir(self.available).
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/test_potential.py (2)
27-29: Fix the resource path construction causing test failure.The extra
".."in the path construction causes tests to look for resources at../tests/static/potentialinstead oftests/static/potential, as previously flagged.Apply this diff:
- cls.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ) + cls.resource_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + )
195-197: Fix the resource path construction.The same path construction error from line 27-29 is repeated here, as previously flagged.
Apply this diff:
- self.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ) + self.resource_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + )
🧹 Nitpick comments (1)
tests/test_potential.py (1)
193-208: Consider adding test for getitem override.
LammpsPotentialFileoverrides__getitem__to return aLammpsPotentialFileinstance rather thanPotentialAbstract. This behavior should be tested to ensure the subclass chain is preserved.Add this test method to the class:
def test_getitem(self): potential = self.potential["Al"] self.assertIsInstance(potential, LammpsPotentialFile) self.assertIsInstance(potential, PotentialAbstract) self.assertEqual(potential._resource_path, self.resource_path)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_potential.py (1)
pyiron_lammps/potential.py (16)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)get_potential_by_name(349-355)convert_path_to_abs_posix(287-304)get_resource_path_from_conda(328-337)find_potential_file_base(254-267)view_potentials(270-284)update_potential_paths(307-325)PotentialAbstract(37-153)LammpsPotentialFile(156-231)PotentialAvailable(234-251)find(62-84)find_by_name(86-90)_get_potential_df(117-153)find_default(194-221)default(186-192)
🪛 Ruff (0.14.4)
tests/test_potential.py
69-69: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
74-74: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
76-76: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
⏰ 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). (2)
- GitHub Check: unittest_matrix (windows-latest, 3.14)
- GitHub Check: unittest_matrix (macos-latest, 3.14)
🔇 Additional comments (2)
tests/test_potential.py (2)
83-129: Good use of tempfile.TemporaryDirectory for test isolation.The test properly uses
tempfile.TemporaryDirectoryas a context manager, ensuring automatic cleanup and avoiding platform-specific paths.
138-191: Comprehensive test coverage for PotentialAbstract.The test suite thoroughly validates input handling (str, list, set, invalid types), error cases (missing potentials, missing files), and core functionality (find, find_by_name, list operations).
| @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True) | ||
| def test_get_resource_path_from_conda(self): | ||
| with self.assertRaises(ValueError): | ||
| get_resource_path_from_conda() | ||
|
|
||
| os.makedirs("/tmp/conda/share/iprpy") | ||
| self.assertEqual(get_resource_path_from_conda(), "/tmp/conda/share/iprpy") | ||
| shutil.rmtree("/tmp/conda") | ||
|
|
There was a problem hiding this comment.
Replace hardcoded /tmp path with tempfile for cross-platform compatibility.
The hardcoded /tmp/conda path will fail on Windows and may conflict with existing directories on Unix systems. Additionally, if the assertion at line 75 fails, the cleanup at line 76 won't execute, leaving test artifacts.
Apply this diff:
- @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True)
def test_get_resource_path_from_conda(self):
- with self.assertRaises(ValueError):
- get_resource_path_from_conda()
-
- os.makedirs("/tmp/conda/share/iprpy")
- self.assertEqual(get_resource_path_from_conda(), "/tmp/conda/share/iprpy")
- shutil.rmtree("/tmp/conda")
+ with tempfile.TemporaryDirectory() as tmpdir:
+ # Test ValueError when directory doesn't exist
+ with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True):
+ with self.assertRaises(ValueError):
+ get_resource_path_from_conda()
+
+ # Test success when directory exists
+ iprpy_path = os.path.join(tmpdir, "share", "iprpy")
+ os.makedirs(iprpy_path)
+ with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True):
+ self.assertEqual(get_resource_path_from_conda(), iprpy_path)🧰 Tools
🪛 Ruff (0.14.4)
69-69: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
74-74: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
76-76: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
🤖 Prompt for AI Agents
In tests/test_potential.py around lines 69 to 77, replace the hardcoded
"/tmp/conda" usage with a temporary directory (via tempfile.mkdtemp() or
tempfile.TemporaryDirectory()) and point the CONDA_PREFIX mock to that temp
path; create the expected subpath (tempdir/share/iprpy) for the assertion, and
ensure cleanup always runs by using a try/finally or Python's TemporaryDirectory
context manager so the temp directory is removed even if the assertion fails.
| class TestPotentialAvailable(unittest.TestCase): | ||
| def setUp(self): | ||
| self.potentials = ["pot1", "pot-2", "pot.3"] | ||
| self.available = PotentialAvailable(list_of_potentials=self.potentials) | ||
|
|
||
| def test_repr(self): | ||
| self.assertEqual(repr(self.available), str(dir(self.available))) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add missing tests for PotentialAvailable.getattr and dir.
The test class only covers __repr__ but not __getattr__ and __dir__ methods. Based on past review comments and the implementation, these methods need test coverage.
Add these test methods to validate the dynamic attribute behavior:
def test_getattr(self):
self.assertEqual(self.available.pot_pot1, "pot1")
self.assertEqual(self.available.pot_pot_2, "pot-2")
self.assertEqual(self.available.pot_pot_3, "pot.3")
with self.assertRaises(AttributeError):
_ = self.available.not_a_potential
def test_dir(self):
self.assertEqual(
sorted(dir(self.available)), sorted(["pot_pot1", "pot_pot_2", "pot_pot_3"])
)Note: The implementation prepends "pot_" and normalizes - and . to _, so "pot1" → "pot_pot1", "pot-2" → "pot_pot_2", "pot.3" → "pot_pot_3".
🤖 Prompt for AI Agents
In tests/test_potential.py around lines 210 to 216, the TestPotentialAvailable
class only tests __repr__; add two new test methods: test_getattr to verify
dynamic attribute access maps potentials to attributes (e.g. available.pot_pot1
== "pot1", available.pot_p_2 == "pot-2", available.pot_p_3 == "pot.3") and
raises AttributeError for unknown attributes, and test_dir to assert
dir(available) returns the normalized attribute names (sorted equals
["pot_pot1","pot_p_2","pot_p_3"]). Implement these methods in the
TestPotentialAvailable class using self.assertEqual and self.assertRaises as
shown in the review comment.
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: 1
♻️ Duplicate comments (4)
tests/test_potential.py (4)
26-28: Fix the resource path construction causing test failure.The path construction includes an extra
".."that navigates up from the repository root, causing the test to look for resources at../tests/static/potentialinstead oftests/static/potential.Apply this diff to fix the path:
- cls.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ) + cls.resource_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + )
70-83: Replace hardcoded /tmp path with tempfile for cross-platform compatibility.The hardcoded
/tmp/condapath will fail on Windows and may conflict with existing directories on Unix systems. Additionally, if the assertion at lines 76-78 fails, the cleanup at line 83 won't execute, leaving test artifacts.Apply this diff:
- @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True) def test_get_resource_path_from_conda(self): - with self.assertRaises(ValueError): - get_resource_path_from_conda() - - os.makedirs("/tmp/conda/share/iprpy") - self.assertEqual( - get_resource_path_from_conda().replace("\\", "/"), "/tmp/conda/share/iprpy" - ) - df = get_potential_dataframe( - structure=bulk("Al"), - ) - self.assertEqual(len(df), 0) - shutil.rmtree("/tmp/conda") + with tempfile.TemporaryDirectory() as tmpdir: + # Test ValueError when directory doesn't exist + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + with self.assertRaises(ValueError): + get_resource_path_from_conda() + + # Test success when directory exists + iprpy_path = os.path.join(tmpdir, "share", "iprpy") + os.makedirs(iprpy_path) + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + self.assertEqual(get_resource_path_from_conda(), iprpy_path) + + # Test get_potential_dataframe with empty resource + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + df = get_potential_dataframe(structure=bulk("Al")) + self.assertEqual(len(df), 0)
202-204: Fix the resource path construction.The same path construction error from
TestPotential.setUpClassis repeated here. The extra".."will cause tests to fail.Apply this diff:
- self.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") - ) + self.resource_path = os.path.abspath( + os.path.join(os.path.dirname(__file__), "static", "potential") + )
217-223: Add missing tests for PotentialAvailable.getattr and dir.The test class only covers
__repr__but not__getattr__and__dir__methods. Based on past review comments and the implementation, these methods need test coverage.Add these test methods to validate the dynamic attribute behavior:
def test_getattr(self): self.assertEqual(self.available.pot_pot1, "pot1") self.assertEqual(self.available.pot_pot_2, "pot-2") self.assertEqual(self.available.pot_pot_3, "pot.3") with self.assertRaises(AttributeError): _ = self.available.not_a_potential def test_dir(self): self.assertEqual( sorted(dir(self.available)), sorted(["pot_pot1", "pot_pot_2", "pot_pot_3"]) )Note: The implementation prepends
"pot_"and normalizes-and.to_, so"pot1"→"pot_pot1","pot-2"→"pot_pot_2","pot.3"→"pot_pot_3".
🧹 Nitpick comments (1)
tests/test_compatibility_file.py (1)
52-61: Consider prefixing unused variables with underscore.The
shell_outputandjob_crashedreturn values are not used in this test. Following Python convention, prefix unused variables with_to signal intent.Apply this diff:
- shell_output, parsed_output, job_crashed = lammps_file_interface_function( + _shell_output, parsed_output, _job_crashed = lammps_file_interface_function(Note: Apply the same pattern to all test methods in this class (lines 64, 76, 89, 117, 145, 174).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/test_compatibility_file.py(1 hunks)tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/test_compatibility_file.py (1)
pyiron_lammps/potential.py (1)
get_potential_by_name(349-355)
tests/test_potential.py (1)
pyiron_lammps/potential.py (15)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)get_potential_by_name(349-355)convert_path_to_abs_posix(287-304)get_resource_path_from_conda(328-337)find_potential_file_base(254-267)view_potentials(270-284)PotentialAbstract(37-153)LammpsPotentialFile(156-231)PotentialAvailable(234-251)find(62-84)find_by_name(86-90)_get_potential_df(117-153)find_default(194-221)default(186-192)
🪛 Ruff (0.14.5)
tests/test_compatibility_file.py
53-53: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
53-53: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
64-64: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
64-64: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
76-76: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
76-76: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
89-89: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
89-89: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
117-117: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
117-117: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
145-145: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
145-145: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
174-174: Unpacked variable shell_output is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
174-174: Unpacked variable job_crashed is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
tests/test_potential.py
70-70: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
77-77: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
83-83: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
🔇 Additional comments (14)
tests/test_potential.py (7)
34-48: LGTM!Comprehensive test coverage for
validate_potential_dataframeerror handling and type conversions.
53-58: LGTM!Correctly validates potential lookup by name.
60-68: LGTM!Properly tests path normalization for both home directory and current directory cases.
85-88: LGTM!Correctly validates error handling when conda environment variables are absent.
90-135: LGTM!Excellent test coverage for
find_potential_file_basewith proper use oftempfile.TemporaryDirectoryfor cross-platform compatibility and automatic cleanup.
145-197: LGTM!Comprehensive test suite for
PotentialAbstractcovering element filtering, name lookup, list operations, item access, and error handling.
207-214: LGTM!Tests correctly validate
LammpsPotentialFileinitialization and default lookup behavior.tests/test_compatibility_file.py (7)
48-50: LGTM!Proper cleanup of test artifacts.
63-72: LGTM!Correctly tests minimize mode for positions.
74-85: LGTM!Correctly tests minimize mode with volume relaxation.
87-108: LGTM!Comprehensive validation of MD trajectory output shapes and keys for NVT ensemble.
110-136: LGTM!Comprehensive validation of MD trajectory output for NPT ensemble.
138-164: LGTM!Comprehensive validation of MD trajectory output for Langevin NVT ensemble.
166-193: LGTM!Comprehensive validation of MD trajectory output for Langevin NPT ensemble.
| resource_path=os.path.abspath( | ||
| os.path.join("..", os.path.dirname(__file__), "static", "lammps") | ||
| ), |
There was a problem hiding this comment.
Fix the resource path construction.
The path construction includes an extra ".." that will cause the test to look for resources at ../tests/static/lammps instead of tests/static/lammps.
Apply this diff:
self.potential = get_potential_by_name(
potential_name="2009--Mendelev-M-I--Al-Mg--LAMMPS--ipr1",
resource_path=os.path.abspath(
- os.path.join("..", os.path.dirname(__file__), "static", "lammps")
+ os.path.join(os.path.dirname(__file__), "static", "lammps")
),
)📝 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.
| resource_path=os.path.abspath( | |
| os.path.join("..", os.path.dirname(__file__), "static", "lammps") | |
| ), | |
| resource_path=os.path.abspath( | |
| os.path.join(os.path.dirname(__file__), "static", "lammps") | |
| ), |
🤖 Prompt for AI Agents
In tests/test_compatibility_file.py around lines 16 to 18, the resource_path is
built with an extra ".." which makes it resolve to ../tests/static/lammps
instead of tests/static/lammps; remove the leading ".." from the os.path.join
call so the path is os.path.join(os.path.dirname(__file__), "static", "lammps")
wrapped with os.path.abspath, ensuring the test looks up tests/static/lammps
correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
tests/test_potential.py (4)
26-28: Fix the resource path construction causing test failures.The extra
".."in the path construction navigates up from the repository root, causing the test to look for../tests/static/potentialinstead oftests/static/potential.Apply this diff:
cls.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") + os.path.join(os.path.dirname(__file__), "static", "potential") )
70-84: Replace hardcoded /tmp path with tempfile for cross-platform compatibility.The hardcoded
/tmp/condapath will fail on Windows and may conflict with existing directories. Additionally, if assertions fail, the cleanup at line 83 won't execute.Apply this diff:
- @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True) def test_get_resource_path_from_conda(self): - with self.assertRaises(ValueError): - get_resource_path_from_conda() - - os.makedirs("/tmp/conda/share/iprpy") - self.assertEqual( - get_resource_path_from_conda().replace("\\", "/"), "/tmp/conda/share/iprpy" - ) - df = get_potential_dataframe( - structure=bulk("Al"), - ) - self.assertEqual(len(df), 0) - shutil.rmtree("/tmp/conda") + with tempfile.TemporaryDirectory() as tmpdir: + # Test ValueError when directory doesn't exist + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + with self.assertRaises(ValueError): + get_resource_path_from_conda() + + # Test success when directory exists + iprpy_path = os.path.join(tmpdir, "share", "iprpy") + os.makedirs(iprpy_path) + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + self.assertEqual( + get_resource_path_from_conda().replace("\\", "/"), + iprpy_path.replace("\\", "/") + ) + df = get_potential_dataframe(structure=bulk("Al")) + self.assertEqual(len(df), 0)
202-204: Fix the resource path construction.Same issue as in
TestPotential.setUpClass: the extra".."causes the path to resolve incorrectly.Apply this diff:
self.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") + os.path.join(os.path.dirname(__file__), "static", "potential") )
217-226: Add missing test coverage for__getattr__and__dir__.The test only covers
__repr__and one successful__getattr__call. Missing coverage for theAttributeErrorcase and the__dir__method.Add these test methods:
def test_get_attr_error(self): with self.assertRaises(AttributeError): _ = self.available.not_a_potential def test_dir(self): self.assertEqual( sorted(dir(self.available)), sorted(["pot_pot1", "pot_pot2", "pot_pot3"]) )
🧹 Nitpick comments (1)
tests/test_potential.py (1)
218-220: Pass a list instead of a dict for clarity.The parameter name is
list_of_potentialsbut a dict is passed. While this works (iterating over a dict gives keys), the dict values are never used, making the intent unclear.Apply this diff:
def setUp(self): - self.potentials = {"pot1": 1, "pot2": 2, "pot3": 3} - self.available = PotentialAvailable(list_of_potentials=self.potentials) + self.available = PotentialAvailable(list_of_potentials=["pot1", "pot2", "pot3"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_potential.py (1)
pyiron_lammps/potential.py (15)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)get_potential_by_name(349-355)convert_path_to_abs_posix(287-304)get_resource_path_from_conda(328-337)find_potential_file_base(254-267)view_potentials(270-284)PotentialAbstract(37-153)LammpsPotentialFile(156-231)PotentialAvailable(234-251)find(62-84)find_by_name(86-90)_get_potential_df(117-153)find_default(194-221)default(186-192)
🪛 Ruff (0.14.5)
tests/test_potential.py
70-70: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
77-77: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
83-83: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
tests/test_potential.py (3)
198-200: Fix the resource path construction causing test failure.Same path construction error as lines 26–28: the extra
".."causes the path to resolve incorrectly.Apply this diff:
self.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") + os.path.join(os.path.dirname(__file__), "static", "potential") )
213-222: Complete the TestPotentialAvailable test methods.The test setup uses a simple dict input that doesn't fully exercise the normalization logic (replacing
-and.with_). Additionally,test_getattrappears incomplete andtest_diris missing entirely. Update the test class to fully validate attribute access and directory listing.Apply this diff to complete the test methods:
class TestPotentialAvailable(unittest.TestCase): def setUp(self): - self.potentials = {"pot1": 1, "pot2": 2, "pot3": 3} - self.available = PotentialAvailable(list_of_potentials=self.potentials) + self.available = PotentialAvailable( + list_of_potentials=["pot1", "pot-2", "pot.3"] + ) def test_repr(self): self.assertEqual(repr(self.available), str(dir(self.available))) - def test_get_attr(self): - self.assertEqual(str(self.available.pot_pot1), "pot1") + def test_getattr(self): + self.assertEqual(self.available.pot_pot1, "pot1") + self.assertEqual(self.available.pot_pot_2, "pot-2") + self.assertEqual(self.available.pot_pot_3, "pot.3") + with self.assertRaises(AttributeError): + _ = self.available.not_a_potential + + def test_dir(self): + self.assertEqual( + sorted(dir(self.available)), + sorted(["pot_pot1", "pot_pot_2", "pot_pot_3"]), + )
70-79: Replace hardcoded /tmp path with tempfile for cross-platform compatibility and guaranteed cleanup.The hardcoded
/tmp/condapath is Unix-specific and fails on Windows. Additionally, if the assertion fails, the cleanup code doesn't execute, leaving test artifacts behind. Usetempfile.TemporaryDirectory()for safe, automatic cleanup.Apply this diff:
- @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True) def test_get_resource_path_from_conda(self): - with self.assertRaises(ValueError): - get_resource_path_from_conda() - - os.makedirs("/tmp/conda/share/iprpy") - self.assertEqual( - get_resource_path_from_conda().replace("\\", "/"), "/tmp/conda/share/iprpy" - ) - shutil.rmtree("/tmp/conda") + with tempfile.TemporaryDirectory() as tmpdir: + # Test ValueError when directory doesn't exist + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + with self.assertRaises(ValueError): + get_resource_path_from_conda() + + # Test success when directory exists + iprpy_path = os.path.join(tmpdir, "share", "iprpy") + os.makedirs(iprpy_path) + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + self.assertEqual( + get_resource_path_from_conda().replace("\\", "/"), + iprpy_path.replace("\\", "/") + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_potential.py (1)
pyiron_lammps/potential.py (15)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)get_potential_by_name(349-355)convert_path_to_abs_posix(287-304)get_resource_path_from_conda(328-337)find_potential_file_base(254-267)view_potentials(270-284)PotentialAbstract(37-153)LammpsPotentialFile(156-231)PotentialAvailable(234-251)find(62-84)find_by_name(86-90)_get_potential_df(117-153)find_default(194-221)default(186-192)
🪛 Ruff (0.14.5)
tests/test_potential.py
70-70: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
77-77: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
79-79: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
⏰ 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)
| cls.resource_path = os.path.abspath( | ||
| os.path.join("..", os.path.dirname(__file__), "static", "potential") | ||
| ) |
There was a problem hiding this comment.
Fix the resource path construction causing test failures.
The extra ".." prefix in the path join causes the resource path to resolve to ../tests/static/potential instead of tests/static/potential, which breaks resource discovery.
Apply this diff:
cls.resource_path = os.path.abspath(
- os.path.join("..", os.path.dirname(__file__), "static", "potential")
+ os.path.join(os.path.dirname(__file__), "static", "potential")
)🤖 Prompt for AI Agents
In tests/test_potential.py around lines 26 to 28, the resource path is built
with an extra ".." which causes the path to point to ../tests/static/potential
instead of tests/static/potential; fix by removing the leading ".." from the
os.path.join call so the path is constructed as
os.path.join(os.path.dirname(__file__), "static", "potential") and then wrapped
with os.path.abspath (i.e., compute the absolute path based on the test file
directory without the extra parent directory step).
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
tests/test_potential.py (3)
26-28: Fix the resource path construction causing test failure.The extra
".."in the path construction causes it to resolve to../tests/static/potentialinstead oftests/static/potential, breaking resource discovery.Apply this diff:
cls.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") + os.path.join(os.path.dirname(__file__), "static", "potential") )
70-80: Replace hardcoded /tmp path with tempfile for cross-platform compatibility.The hardcoded
/tmp/condapath will fail on Windows and may conflict with existing directories. If the assertion at line 76 fails, cleanup at line 79 won't execute.Apply this diff:
- @mock.patch.dict(os.environ, {"CONDA_PREFIX": "/tmp/conda"}, clear=True) def test_get_resource_path_from_conda(self): - with self.assertRaises(ValueError): - get_resource_path_from_conda() - - os.makedirs("/tmp/conda/share/iprpy") - self.assertEqual( - get_resource_path_from_conda().replace("\\", "/"), "/tmp/conda/share/iprpy" - ) - shutil.rmtree("/tmp/conda") + with tempfile.TemporaryDirectory() as tmpdir: + # Test ValueError when directory doesn't exist + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + with self.assertRaises(ValueError): + get_resource_path_from_conda() + + # Test success when directory exists + iprpy_path = os.path.join(tmpdir, "share", "iprpy") + os.makedirs(iprpy_path) + with mock.patch.dict(os.environ, {"CONDA_PREFIX": tmpdir}, clear=True): + self.assertEqual( + get_resource_path_from_conda().replace("\\", "/"), + iprpy_path.replace("\\", "/") + )
198-200: Fix the resource path construction.Same path construction error as in
TestPotential.setUpClass. The extra".."will cause tests to fail.Apply this diff:
self.resource_path = os.path.abspath( - os.path.join("..", os.path.dirname(__file__), "static", "potential") + os.path.join(os.path.dirname(__file__), "static", "potential") )
🧹 Nitpick comments (3)
tests/test_potential.py (3)
210-221: Consider adding test for LammpsPotentialFile.getitem override.
LammpsPotentialFileoverrides__getitem__to return aLammpsPotentialFileinstance instead ofPotentialAbstract. This behavioral difference should be tested to ensure type consistency.Add this test method:
def test_getitem(self): potential = self.potential["Al"] self.assertIsInstance(potential, LammpsPotentialFile) self.assertEqual(len(potential.list()), 1)
226-227: Consider using a list instead of a dict for semantic clarity.While iterating over a dict yields keys (making the test functional),
list_of_potentialsis semantically a list of potential names, not a dict.Apply this diff:
- self.potentials = {"pot1": 1, "pot2": 2, "pot3": 3} - self.available = PotentialAvailable(list_of_potentials=self.potentials) + self.available = PotentialAvailable(list_of_potentials=["pot1", "pot2", "pot3"])
229-233: Expand test coverage for PotentialAvailable.Good progress adding
test_get_attr, but the test coverage for__getattr__and__dir__remains incomplete. Consider adding more assertions and a dedicatedtest_dirmethod.Expand the test coverage:
def test_get_attr(self): self.assertEqual(str(self.available.pot_pot1), "pot1") + self.assertEqual(self.available.pot_pot2, "pot2") + self.assertEqual(self.available.pot_pot3, "pot3") + with self.assertRaises(AttributeError): + _ = self.available.not_a_potential + + def test_dir(self): + self.assertEqual( + sorted(dir(self.available)), + sorted(["pot_pot1", "pot_pot2", "pot_pot3"]) + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_potential.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_potential.py (1)
pyiron_lammps/potential.py (15)
validate_potential_dataframe(358-380)get_potential_dataframe(340-346)get_potential_by_name(349-355)convert_path_to_abs_posix(287-304)get_resource_path_from_conda(328-337)find_potential_file_base(254-267)view_potentials(270-284)PotentialAbstract(37-153)LammpsPotentialFile(156-231)PotentialAvailable(234-251)find(62-84)find_by_name(86-90)_get_potential_df(117-153)find_default(194-221)default(186-192)
🪛 Ruff (0.14.5)
tests/test_potential.py
70-70: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
75-75: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
77-77: Probable insecure usage of temporary file or directory: "/tmp/conda/share/iprpy"
(S108)
79-79: Probable insecure usage of temporary file or directory: "/tmp/conda"
(S108)
⏰ 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)
1 participant
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.