abacus/stru: return spins only when atomic magnetic moment is specified #800
Conversation
📝 WalkthroughWalkthroughThis pull request updates the parsing logic for atomic magnetic moments in the STRU file. In Changes
Sequence Diagram(s)sequenceDiagram
participant Parser as parse_pos
participant File as STRU File
Parser->>Parser: Initialize define_atom_mag = False
File-->>Parser: Read atomic data (coordinates, imagmom values)
alt Atomic magnetic moment found
Parser->>Parser: Set define_atom_mag = True
Parser->>Parser: Append imagmom to mags array
else No magnetic moment detected
Parser->>Parser: Continue without updating mags array
end
alt define_atom_mag is True
Parser->>Parser: Convert mags to NumPy array
else
Parser->>Parser: Return empty list for mags
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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: 0
🧹 Nitpick comments (1)
dpdata/abacus/stru.py (1)
416-420: Return spins only when explicitly definedThis change properly implements the PR objective by conditionally returning spin information only when magnetic moments are explicitly defined.
Consider using a ternary operator for a slightly more concise implementation:
- # here return the magnetic moment only when the atom magnetic moment is specified. - if not define_atom_mag: - mags = [] - else: - mags = np.array(mags) + # here return the magnetic moment only when the atom magnetic moment is specified. + mags = [] if not define_atom_mag else np.array(mags)🧰 Tools
🪛 Ruff (0.8.2)
417-420: Use ternary operator
mags = [] if not define_atom_mag else np.array(mags)instead ofif-else-blockReplace
if-else-block withmags = [] if not define_atom_mag else np.array(mags)(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
dpdata/abacus/stru.py(5 hunks)tests/abacus.scf/stru_test(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
dpdata/abacus/stru.py
417-420: Use ternary operator mags = [] if not define_atom_mag else np.array(mags) instead of if-else-block
Replace if-else-block with mags = [] if not define_atom_mag else np.array(mags)
(SIM108)
🔇 Additional comments (5)
dpdata/abacus/stru.py (4)
365-365: LGTM: Good addition for tracking magnetic moment definitionThis Boolean flag is a clean way to track whether any atomic magnetic moments are explicitly defined in the input data.
393-395: Appropriate condition for magnetic moment trackingThis code correctly identifies when a magnetic moment is explicitly specified for an atom and updates the tracking flag accordingly.
449-456: Good documentation updateThe documentation now clearly explains when spin information is included in the returned data and provides additional context about delta spin calculations and non-spin jobs.
486-489: Correctly conditionally includes spins in returned dataThis change ensures that the "spins" key is only included in the returned data dictionary when magnetic moments are defined, aligned with the PR objective.
tests/abacus.scf/stru_test (1)
25-25: Test data updated to match new behaviorThe removal of the zero magnetic moment specifications (
mag 0.000000000000 0.000000000000 0.000000000000) from the test data is consistent with the PR objective. This ensures that the test cases properly verify the new behavior where spin information is only returned when explicitly defined with non-zero values.Also applies to: 29-32
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dpdata/abacus/stru.py (1)
416-420: Correctly implemented conditional return of magnetic momentsThis change properly implements the core functionality of the PR - only returning magnetic moment data when it's explicitly defined in the STRU file, which prevents inclusion of unnecessary spin information for non-spin jobs.
Consider using a ternary operator for slightly cleaner code:
- if not define_atom_mag: - mags = [] - else: - mags = np.array(mags) + mags = [] if not define_atom_mag else np.array(mags)🧰 Tools
🪛 Ruff (0.8.2)
417-420: Use ternary operator
mags = [] if not define_atom_mag else np.array(mags)instead ofif-else-blockReplace
if-else-block withmags = [] if not define_atom_mag else np.array(mags)(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdata/abacus/stru.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
dpdata/abacus/stru.py
417-420: Use ternary operator mags = [] if not define_atom_mag else np.array(mags) instead of if-else-block
Replace if-else-block with mags = [] if not define_atom_mag else np.array(mags)
(SIM108)
🔇 Additional comments (3)
dpdata/abacus/stru.py (3)
363-365: Good addition of tracking magnetic moment definitionThe addition of
define_atom_magflag is a clean way to track whether any atomic magnetic moment is explicitly defined in the STRU file.
394-395: Appropriate condition detectionSetting the flag when any atom has a non-None magnetic moment correctly identifies when spin information is relevant.
449-455: Clear documentation of the behavior changeThe updated documentation clearly explains when spin information will be included in the returned data and the implications for different job types.
CodSpeed Performance ReportMerging #800 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #800 +/- ##
==========================================
+ Coverage 85.33% 85.34% +0.01%
==========================================
Files 82 82
Lines 7515 7522 +7
==========================================
+ Hits 6413 6420 +7
Misses 1102 1102 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @wanghan-iapcm @njzjz , the bug fixed by this commit can cause the failure of using DPGEN+ABACUS. Could you please release a new version? |
2 participants
The old version will induce a bug that dpdata will always store the spin information, and when dpdata save it to lammps format, the spin information will also be written, which is invalid for a non-spin lammps job.
Now, the spin information is saved to dpdata only when the magnetic moment of one atom is specified, which is necessary for a spin job.
Summary by CodeRabbit