Test write structure #199
Test write structure #199
Conversation
for more information, see https://pre-commit.ci
|
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 47 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)
WalkthroughThis pull request modifies the Changes
Possibly related PRs
Poem
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #199 +/- ##
==========================================
+ Coverage 53.07% 62.04% +8.96%
==========================================
Files 5 5
Lines 893 901 +8
==========================================
+ Hits 474 559 +85
+ Misses 419 342 -77 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_structure.py (1)
6-10: Remove unused importLammpsStructure.
Static analysis indicates thatLammpsStructureis never used. Eliminating it keeps the test file clean.-from pyiron_lammps.structure import ( - structure_to_lammps, - LammpsStructure, - write_lammps_datafile, -) +from pyiron_lammps.structure import ( + structure_to_lammps, + write_lammps_datafile, +)🧰 Tools
🪛 Ruff (0.8.2)
8-8:
pyiron_lammps.structure.LammpsStructureimported but unusedRemove unused import:
pyiron_lammps.structure.LammpsStructure(F401)
pyiron_lammps/structure.py (2)
721-739: Combine nested if statements for cleaner logic.
Adopting a single condition improves readability while preserving functionality.def is_skewed(structure, tolerance=1.0e-8): volume = structure.get_volume() prod = np.linalg.norm(structure.cell, axis=-1).prod() - if volume > 0: - if abs(volume - prod) / volume < tolerance: - return False - return True + if volume > 0 and abs(volume - prod) / volume < tolerance: + return False + else: + return True🧰 Tools
🪛 Ruff (0.8.2)
735-736: Use a single
ifstatement instead of nestedifstatementsCombine
ifstatements usingand(SIM102)
741-753: Add explanatory docstring for new parameters.
The expanded signature (el_eam_lst,bond_dict,units) benefits from in-code documentation aligning with how these arguments affect the data file generation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyiron_lammps/structure.py(9 hunks)tests/test_structure.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_structure.py
8-8: pyiron_lammps.structure.LammpsStructure imported but unused
Remove unused import: pyiron_lammps.structure.LammpsStructure
(F401)
pyiron_lammps/structure.py
735-736: Use a single if statement instead of nested if statements
Combine if statements using and
(SIM102)
🪛 GitHub Check: codecov/patch
pyiron_lammps/structure.py
[warning] 252-252: pyiron_lammps/structure.py#L252
Added line #L252 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
🔇 Additional comments (12)
tests/test_structure.py (5)
3-4: Imports look fine.
No issues found; importingosandrmtreeis appropriate for directory management in tests.
14-16: Good test directory setup.
UsingsetUpto create a dedicated directory for test data is a reasonable approach to isolate files per test.
18-20: Proper test directory cleanup.
Tearing down the directory at the class level minimizes leftover artifacts. This is consistent withsetUpusage.
69-105: Comprehensive test for a non-cubic structure.
Hard-checking file contents ensures correctness, though consider verifying only key lines if future LAMMPS data format changes are anticipated.
106-146: Thorough coverage of cubic structure output.
Testing a 4-atom cubic configuration solidifies confidence in thewrite_lammps_datafileoutput.pyiron_lammps/structure.py (7)
13-13: Import statement approved.
Pullingatomic_massesandatomic_numbersfromase.datais necessary for new header-generation logic.
191-201: Constructor usesunitsinstead ofjob.
Replacing_jobwith_unitsaligns with the newly added velocity conversions. Ensure any references to the old parameter have been fully removed.
241-245: Velocity scaling is handled correctly.
UsingUnitConverterto adjust velocities according to the chosen units is a sound approach for consistent simulations.
248-252: 2D and 3D velocity output logic looks good.
Supporting both 2D and 3D assures broader applicability, although dimension=1 remains unimplemented.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 252-252: pyiron_lammps/structure.py#L252
Added line #L252 was not covered by tests
349-350: Conditional application of skewed cell.
Usingis_skewedproperly toggles the triclinic parameters when needed.
658-658: Dimension handling.
Retrieving the dimension by examining the first position is straightforward and consistent with subsequent logic.
703-703: Velocity rotation for LAMMPS coordinate frame.
Applying the same coordinate transform to velocities maintains consistency between position and velocity spaces.
…cture # Conflicts: # tests/test_structure.py
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/test_structure.py (2)
6-10: Remove unused import.The
LammpsStructureimport is not used in this file.Apply this diff to remove the unused import:
from pyiron_lammps.structure import ( structure_to_lammps, - LammpsStructure, write_lammps_datafile, )🧰 Tools
🪛 Ruff (0.8.2)
8-8:
pyiron_lammps.structure.LammpsStructureimported but unusedRemove unused import:
pyiron_lammps.structure.LammpsStructure(F401)
69-108: Improve readability by using a descriptive variable name.The list comprehension uses an ambiguous variable name
l. Consider using a more descriptive name likeline.Apply this diff to improve readability:
[ - l + line for l in f.readlines() if "xlo xhi" not in l and "ylo yhi" not in l and "zlo zhi" not in l and "xy xz yz" not in l ],🧰 Tools
🪛 Ruff (0.8.2)
81-81: Ambiguous variable name:
l(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_structure.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_structure.py
8-8: pyiron_lammps.structure.LammpsStructure imported but unused
Remove unused import: pyiron_lammps.structure.LammpsStructure
(F401)
81-81: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
🔇 Additional comments (3)
tests/test_structure.py (3)
14-20: LGTM! Well-structured test setup and teardown.The setup and teardown methods follow best practices by:
- Creating a dedicated output folder for test data
- Ensuring proper cleanup of test artifacts
109-149: LGTM! Comprehensive test for cubic structures.The test thoroughly verifies:
- Correct atom count and types
- Box dimensions
- Atomic positions
- Velocities
1-150: LGTM! Excellent test coverage.The test suite provides comprehensive coverage:
- Structure conversion with and without velocities
- Non-cubic and cubic structure handling
- LAMMPS data file generation and content verification
🧰 Tools
🪛 Ruff (0.8.2)
8-8:
pyiron_lammps.structure.LammpsStructureimported but unusedRemove unused import:
pyiron_lammps.structure.LammpsStructure(F401)
81-81: Ambiguous variable name:
l(E741)
Summary by CodeRabbit
New Features
Tests