Conversation
for more information, see https://pre-commit.ci
WalkthroughThe PR introduces charge-aware structure generation to LammpsStructure by adding an optional Changes
Sequence DiagramsequenceDiagram
participant User
participant LammpsStructure
participant StructureBuilder
User->>LammpsStructure: __init__(atom_type="charge")
LammpsStructure->>LammpsStructure: Store _atom_type="charge"
User->>LammpsStructure: structure = atoms
LammpsStructure->>LammpsStructure: Check _atom_type
rect rgb(200, 240, 255)
Note over LammpsStructure: atom_type="charge" path
LammpsStructure->>StructureBuilder: structure_charge()
StructureBuilder->>StructureBuilder: Map species to IDs
StructureBuilder->>StructureBuilder: Include per-atom charges
StructureBuilder-->>LammpsStructure: Return charged atom data
end
rect rgb(240, 240, 200)
Note over LammpsStructure: atom_type="atomic" path (default)
LammpsStructure->>StructureBuilder: structure_atomic()
StructureBuilder-->>LammpsStructure: Return atomic data
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #269 +/- ##
==========================================
+ Coverage 95.76% 95.85% +0.09%
==========================================
Files 5 5
Lines 637 652 +15
==========================================
+ Hits 610 625 +15
Misses 27 27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyiron_lammps/structure.py (1)
201-217: Confusing dualatom_typeattributes confirmed.Verification shows
self.atom_type(line 211) is initialized toNonebut never used in actual code—only in a comment on line 249. The active logic usesself._atom_type(line 217), making the public attribute unused legacy code that should be removed to reduce confusion.
🧹 Nitpick comments (2)
pyiron_lammps/structure.py (2)
394-431: Consider refactoring to reduce duplication withstructure_atomic().The methods
structure_charge()andstructure_atomic()(lines 360-392) share ~80% identical logic. Consider extracting common logic into a helper method that accepts a format callback or charge list parameter to reduce maintenance burden.
409-409: Minor inconsistency in dimension access.Line 409 uses
self._structure.positions.shape[1]while the equivalent line instructure_atomic()(line 375) useslen(self._structure.positions[0]). Both are correct, but consistency would improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pyiron_lammps/structure.py(4 hunks)tests/test_structure.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/test_structure.py (1)
pyiron_lammps/structure.py (3)
LammpsStructure(194-478)structure(228-234)structure(237-251)
⏰ 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)
🔇 Additional comments (2)
tests/test_structure.py (1)
230-257: LGTM! Charge-aware structure generation is properly tested.The test correctly validates that
LammpsStructurewithatom_type="charge"generates LAMMPS input including per-atom charges in the expected format.pyiron_lammps/structure.py (1)
247-250: LGTM! Branching logic correctly routes to charge-aware or atomic structure generation.The conditional logic appropriately selects
structure_charge()whenatom_type="charge"is specified, preserving backward compatibility with the default atomic mode.
| def structure_charge(self): | ||
| """ | ||
| Create atom structure including the atom charges. | ||
|
|
||
| By convention the LAMMPS atom type numbers are chose alphabetically for the chemical species. | ||
|
|
||
| Returns: LAMMPS readable structure. | ||
|
|
||
| """ | ||
| species_lammps_id_dict = self.get_lammps_id_dict(self.el_eam_lst) | ||
| atoms = "Atoms\n\n" | ||
| coords = self.rotate_positions(self._structure) | ||
| el_charge_lst = self._structure.get_initial_charges() | ||
| el_lst = self._structure.get_chemical_symbols() | ||
| for id_atom, (el, coord) in enumerate(zip(el_lst, coords)): | ||
| dim = self._structure.positions.shape[1] | ||
| c = np.zeros(3) | ||
| c[:dim] = coord | ||
| atoms += ( | ||
| "{0:d} {1:d} {2:f} {3:.15f} {4:.15f} {5:.15f}".format( | ||
| id_atom + 1, | ||
| species_lammps_id_dict[el], | ||
| el_charge_lst[id_atom], | ||
| c[0], | ||
| c[1], | ||
| c[2], | ||
| ) | ||
| + "\n" | ||
| ) | ||
| return ( | ||
| self.lammps_header( | ||
| structure=self.structure, | ||
| cell_dimensions=self.simulation_cell(), | ||
| species_lammps_id_dict=species_lammps_id_dict, | ||
| ) | ||
| + atoms | ||
| + "\n" | ||
| ) |
There was a problem hiding this comment.
Missing structure validation check.
Unlike structure_atomic() (lines 367-368), this method doesn't check if self._structure is None before accessing it, which could cause an AttributeError.
Apply this diff to add the validation:
def structure_charge(self):
"""
Create atom structure including the atom charges.
By convention the LAMMPS atom type numbers are chose alphabetically for the chemical species.
Returns: LAMMPS readable structure.
"""
+ if self._structure is None:
+ raise ValueError("Structure not set")
species_lammps_id_dict = self.get_lammps_id_dict(self.el_eam_lst)🤖 Prompt for AI Agents
In pyiron_lammps/structure.py around lines 394 to 431, add the same validation
present in structure_atomic (around lines 367-368): check if self._structure is
None at the start of structure_charge and raise a clear exception (or otherwise
handle the missing structure) before any access to self._structure to prevent
AttributeError; ensure the new check mirrors the project’s existing error
type/message used in structure_atomic.
1 participant
Summary by CodeRabbit
New Features
atom_typeparameter, enabling selection between standard "atomic" mode and "charge" mode for per-atom charge handling.Tests
✏️ Tip: You can customize this high-level summary in your review settings.