Reduce pyiron_lammps package #200
Conversation
for more information, see https://pre-commit.ci
WalkthroughThe changes remove the dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as User/Test
participant LS as LammpsStructure
participant AS as AtomicStructureHandler
participant VSI as VelocityStringHandler
Caller ->> LS: Call structure() method
LS ->> AS: Invoke structure_atomic()
AS -->> LS: Return atomic structure string
LS ->> VSI: Invoke _get_velocities_input_string()
VSI -->> LS: Return velocities string
LS ->> Caller: Return combined input string
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
===========================================
+ Coverage 62.04% 86.45% +24.41%
===========================================
Files 5 4 -1
Lines 901 480 -421
===========================================
- Hits 559 415 -144
+ Misses 342 65 -277 ☔ 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: 1
🧹 Nitpick comments (3)
pyproject.toml (1)
2-2: Dependency Version Consistency Check
Notice that the build-system requirement fornumpydoes not pin a version while the project dependencies explicitly pin it as"numpy==2.2.3". If this divergence is intentional (e.g., to allow flexibility during build while enforcing a fixed runtime version), please document the rationale; otherwise, consider aligning the version specifications across sections.Also applies to: 29-29
tests/test_structure.py (2)
70-70: Add a test with multiple atoms or negative velocities.
This single-atom velocity test is valid but might not fully exercise velocity handling for larger systems or diverse velocity vectors.
106-106: Consider using a tolerance-based check rather than an exact string match.
Relying on a literal numeric match for floating-point data can be fragile and might cause spurious test failures if small differences in numeric precision occur.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.ci_support/environment-old.yml(0 hunks).ci_support/environment.yml(0 hunks)pyiron_lammps/control.py(0 hunks)pyiron_lammps/structure.py(2 hunks)pyproject.toml(1 hunks)tests/test_control.py(0 hunks)tests/test_structure.py(2 hunks)
💤 Files with no reviewable changes (4)
- .ci_support/environment-old.yml
- tests/test_control.py
- .ci_support/environment.yml
- pyiron_lammps/control.py
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: unittest_matrix (windows-latest, 3.12)
- GitHub Check: unittest_matrix (macos-latest, 3.12)
🔇 Additional comments (4)
pyproject.toml (2)
2-2: Cleaned Build-System Dependencies
The removal of"pyiron_base"from the build-systemrequiresarray is in line with the objective to reduce unnecessary dependencies. Please ensure that any build scripts or processes that might have referencedpyiron_baseare updated accordingly.
26-32: Streamlined Project Dependencies
The removal of"pyiron_base==0.11.5"from the[project]dependencies helps slim down the package and reduce dependency overhead. Verify that none of the project modules rely on features provided bypyiron_base, or that suitable alternatives are in place.pyiron_lammps/structure.py (2)
232-234: Straightforward approach to embed velocity data in the main input string.
This concatenation makes sense to maintain a single_string_inputfor the final data file.
257-257: No issues found.
The return statement is correct and neatly ends the velocity block generation logic.
| def _get_velocities_input_string(self): | ||
| input_str = "" | ||
| if not np.all( | ||
| np.isclose( | ||
| self._structure.get_velocities(), | ||
| np.array([[0.0, 0.0, 0.0]] * len(self._structure)), | ||
| ) | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle the case when velocities are not defined or partially zero.
If structure.get_velocities() is None, this check will fail. Consider adding a null check or defaulting to zero velocities to avoid runtime errors and ensure robust handling when velocities are unset.
Summary by CodeRabbit
Chores
Refactor