add spin for abacus/stru #751
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces modifications to several functions across multiple files to enhance the handling of magnetic moment data in atomic simulations. Key changes include the addition of a Changes
Possibly related PRs
Suggested reviewers
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 (
|
CodSpeed Performance ReportMerging #751 will not alter performanceComparing Summary
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (10)
tests/abacus.spin/STRU.spin (1)
14-24: Document magnetic moment specification format.Please add comments explaining:
- The expected format for magnetic moment specifications
- The meaning of each parameter (mag, angle1, angle2)
- The coordinate system used (Cartesian vs. spherical)
- Valid ranges for angles and magnitudes
This documentation is crucial for maintaining and extending the test suite.
Here's a suggested format for the documentation:
ATOMIC_POSITIONS Direct + +# Magnetic moment specification formats: +# 1. Cartesian: mag x y z +# 2. Magnitude only: mag m +# 3. Spherical: mag m angle1 theta [angle2 phi] +# where: +# - x,y,z: Cartesian components in μB +# - m: Magnitude in μB +# - theta: Polar angle in degrees [0-180] +# - phi: Azimuthal angle in degrees [0-360]dpdata/plugins/abacus.py (2)
Line range hint
48-71: Add docstring documentation forregister_mag_dataThe function needs documentation to explain its purpose, parameters, and behavior.
Add this docstring:
def register_mag_data(data): + """Register magnetic data types if present in the input data. + + Parameters + ---------- + data : dict + Dictionary containing system data that may include 'spins' and 'mag_forces' + + Notes + ----- + Registers the following data types if present: + - spins: (nframes, natoms, 3) array for magnetic moments + - mag_forces: (nframes, natoms, 3) array for magnetic forces + """
Line range hint
48-71: Consider a more generic data type registration mechanismThe current implementation has separate functions for registering different types of data (
register_mag_data,register_move_data). As more data types are added, this approach might lead to code duplication.Consider creating a more generic registration mechanism that could handle different data types through configuration, making it easier to add new data types in the future.
Example approach:
def register_data_type(data, type_config): """Generic data type registration. type_config = { 'key': 'spins', 'shape': (Axis.NFRAMES, Axis.NATOMS, 3), 'deepmd_name': 'spin', # other configuration... } """tests/test_abacus_spin.py (3)
162-162: Remove debug print statement.The print statement appears to be debugging code and should be removed for cleaner test output.
- print(mysys.data["spins"])
164-169: Improve documentation structure.The embedded documentation could be more structured to clearly indicate the test cases.
- """ - 0.0000000000 0.000000000 0.000000000 mag 0 0 2 - 0.1000000000 0.1000000000 0.1000000000 mag 3 - 0.2000000000 0.2000000000 0.2000000000 mag 3 angle1 90 - 0.3000000000 0.3000000000 0.3000000000 mag 3 4 0 angle1 90 angle2 90 - """ + """Test cases for spin data: + 1. Cartesian spin vector: [0, 0, 2] + 2. Magnitude only: mag 3 + 3. Magnitude with angle1: mag 3 angle1 90 + 4. Magnitude with two angles: mag 3 4 0 angle1 90 angle2 90 + """
170-173: Enhance test coverage with additional assertions.While the current assertions verify the spin values, consider adding assertions for:
- The total number of spins
- Error cases (invalid angles, magnitudes)
- Coordinate transformations
# Add these assertions: self.assertEqual(len(mysys.data["spins"][0]), 4, "Expected 4 spin vectors")dpdata/abacus/relax.py (1)
Line range hint
31-31: Consider enhancing the documentation.The docstring for
get_coords_from_logcould be updated to include information about magnetic moment handling and clarify which return values are optional.Here's a suggested improvement:
def get_coords_from_log(loglines, natoms): - """NOTICE: unit of coords and cells is Angstrom + """Process ABACUS log file to extract structural and magnetic data. + + NOTICE: unit of coords and cells is Angstrom + order: coordinate cell (no output if cell is not changed) energy (no output, if SCF is not converged) force (no output, if cal_force is not setted or abnormal ending) - stress (no output, if set cal_stress is not setted or abnormal ending). + stress (no output, if set cal_stress is not setted or abnormal ending) + magnetic moments (optional, included when spin calculations are enabled) """dpdata/abacus/md.py (1)
170-172: Consider adding a docstring to document the magmom parameter.Since this is a significant change in the function's interface, it would be helpful to add documentation explaining the format and units of the magnetic moment data.
Example docstring:
""" Get coordinates and related data from ABACUS output. Returns: tuple: (atom_names, natoms, types, coords, move, magmom) - atom_names: List of atomic species - natoms: Number of atoms per species - types: Atom type indices - coords: Atomic coordinates - move: Mobility flags - magmom: Magnetic moments (optional) """dpdata/abacus/scf.py (2)
280-280: Simplifyisinstancechecks foratommag.You can merge the multiple
isinstancechecks into a single call to enhance readability.Apply this diff:
- if not (isinstance(atommag, list) or isinstance(atommag, float)): + if not isinstance(atommag, (list, float)):🧰 Tools
🪛 Ruff
280-280: Multiple
isinstancecalls foratommag, merge into a single callMerge
isinstancecalls foratommag(SIM101)
295-298: Use a ternary operator for concise assignment ofmag_norm.Simplify the assignment by using a ternary operator, which improves code clarity.
Apply this diff:
- if isinstance(atommag, list): - mag_norm = np.linalg.norm(atommag) - else: - mag_norm = atommag + mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag🧰 Tools
🪛 Ruff
295-298: Use ternary operator
mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommaginstead ofif-else-blockReplace
if-else-block withmag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
dpdata/abacus/md.py(1 hunks)dpdata/abacus/relax.py(1 hunks)dpdata/abacus/scf.py(6 hunks)dpdata/plugins/abacus.py(3 hunks)tests/abacus.spin/STRU.spin(1 hunks)tests/test_abacus_spin.py(1 hunks)
🧰 Additional context used
🪛 Ruff
dpdata/abacus/scf.py
280-280: Multiple isinstance calls for atommag, merge into a single call
Merge isinstance calls for atommag
(SIM101)
295-298: Use ternary operator mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag instead of if-else-block
Replace if-else-block with mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag
(SIM108)
🔇 Additional comments (8)
tests/abacus.spin/STRU.spin (3)
7-12: LGTM! Lattice parameters are appropriate for bcc Fe.
The cubic structure with lattice parameter ~5.316 Å (after conversion from Bohr) is reasonable for a bcc Fe system.
1-6: Verify the existence of referenced input files.
The structure references external files that are crucial for the test:
Fe.upf: Pseudopotential fileFe_gga_10au_200.0Ry_4s2p2d1f.orb: Orbital file
20-23:
Standardize magnetic moment specifications.
The magnetic moment specifications use inconsistent formats across atoms:
- Line 20:
mag 0 0 2(Cartesian format?) - Line 21:
mag 3(magnitude only?) - Line 22:
mag 3 angle1 90(spherical coordinates?) - Line 23:
mag 3 4 0 angle1 90 angle2 90(mixed format?)
This inconsistency could lead to confusion and potential errors in interpreting the magnetic structure.
dpdata/plugins/abacus.py (2)
Line range hint 92-120: LGTM! Consistent implementation across format classes
The changes maintain a consistent pattern across all format classes:
- Get frame data
- Register magnetic data
- Register move data
- Return data
This consistency makes the code more maintainable and predictable.
Line range hint 48-71: Verify magnetic data shape consistency
The code assumes 3D vectors for both spins and magnetic forces. Let's verify this assumption across the codebase.
✅ Verification successful
Let's gather more specific information about the magnetic data handling by examining the test file and searching for any other relevant implementations.
Based on the comprehensive test files and implementations examined, I can now provide the final response:
3D vector representation for magnetic data is consistently implemented and verified
The codebase shows consistent implementation and thorough testing of 3D vectors for both spins and magnetic forces:
- The DataType registration in
abacus.pycorrectly defines bothspinsandmag_forceswith shape(Axis.NFRAMES, Axis.NATOMS, 3) - Test files demonstrate extensive validation of 3D vectors:
test_abacus_spin.pyverifies x,y,z components for both spins and magnetic forcestest_lammps_spin.pyconfirms 3D vector handling across different formats- All test cases use explicit 3D vector assertions with
np.testing.assert_almost_equal
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for spin/magnetic data shape definitions or usages
echo "Searching for spin data shape definitions..."
rg -l 'spin.*shape|shape.*spin'
echo "Searching for magnetic force array operations..."
rg -l 'mag_forces.*\[|mag_force.*shape'
Length of output: 307
Script:
#!/bin/bash
# Check the test file content for magnetic data shape validation
echo "Examining test file content:"
cat tests/test_abacus_spin.py
# Search for any spin or magnetic force related implementations
echo -e "\nSearching for spin/magnetic force implementations:"
rg -A 5 "spins|mag_forces"
# Look for any array shape assertions in tests
echo -e "\nSearching for shape assertions in tests:"
rg -A 2 "assert.*shape"
Length of output: 27315
tests/test_abacus_spin.py (1)
159-160: LGTM! Test method is well-integrated.
The test method follows the established pattern and uses appropriate system initialization.
dpdata/abacus/relax.py (1)
186-186: LGTM! Verify related function implementations.
The addition of magmom to the get_coords return value unpacking is correctly implemented and well-integrated with the downstream data handling.
Let's verify the consistency of this change across the codebase:
✅ Verification successful
The initial results show that get_coords is called consistently with magmom across files, but we need to verify:
- The complete implementation to ensure it properly returns magmom
- How magmom is handled in the caller files to ensure consistency
Changes are consistent and correctly implemented across the codebase
The verification confirms:
get_coordsfunction properly implements magmom handling and returns it as part of its tuple- All callers (MD and RELAX) consistently:
- Unpack the magmom return value from
get_coords - Process magmom data similarly using
get_mag_force - Include magmom in the data dictionary as "spins" when available
- Unpack the magmom return value from
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify get_coords implementation and usage
# Test 1: Check get_coords implementation in scf.py
echo "Checking get_coords implementation:"
rg -A 10 "def get_coords" "dpdata/abacus/scf.py"
# Test 2: Find other callers of get_coords to ensure they're updated
echo "Checking other get_coords callers:"
rg "get_coords\(" "dpdata/abacus/"
Length of output: 1315
Script:
#!/bin/bash
# Let's check the implementation details of get_coords to verify it returns magmom
rg -A 30 "def get_coords" "dpdata/abacus/scf.py"
# Also check how magmom is processed in other files
echo "Checking magmom handling in MD:"
rg -B 2 -A 2 "magmom" "dpdata/abacus/md.py"
echo "Checking magmom handling in RELAX:"
rg -B 2 -A 2 "magmom" "dpdata/abacus/relax.py"
Length of output: 2688
dpdata/abacus/md.py (1)
170-172: LGTM! Verify get_coords usage across the codebase.
The addition of the magmom parameter aligns with the PR objective of adding spin support. The implementation correctly handles the magnetic moment data and maintains backward compatibility.
Let's verify that all callers of get_coords have been updated to handle the new return value:
✅ Verification successful
Let me analyze the get_coords usage across the codebase to verify if there are any inconsistencies:
-
dpdata/qe/scf.py: Different implementation ofget_coordswith different signature and return values - this is fine as it's in a different module (QE). -
dpdata/abacus/module:
scf.py: Shows two usages, both updated to handle magmommd.py: Updated to handle magmom (this is the file under review)relax.py: Updated to handle magmom
Let me verify the implementation of get_coords in the ABACUS module to ensure it's consistent:
Let me check the complete implementation to verify the magnetic moment handling:
Let me check the test file to ensure the magnetic moment handling is properly tested:
Based on the comprehensive verification, I can now provide the final response:
✓ The magnetic moment handling changes are properly implemented and tested
The changes to get_coords function are well-integrated across the codebase:
- All callers in the ABACUS module (
scf.py,md.py,relax.py) are updated to handle the newmagmomreturn value - The implementation in
get_coordsproperly handles magnetic moments in both Cartesian and direct coordinates - Unit tests verify the magnetic moment parsing and handling with various input formats
- Test data includes spin-related test cases (
STRU.spin)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all get_coords function calls
rg -l "get_coords\(" | while read -r file; do
echo "=== $file ==="
rg -A 2 "get_coords\(" "$file"
done
Length of output: 1467
Script:
#!/bin/bash
# Check the implementation of get_coords in abacus module
echo "=== dpdata/abacus/scf.py ==="
rg -A 20 "^def get_coords" dpdata/abacus/scf.py
Length of output: 1177
Script:
#!/bin/bash
# Get the full implementation of get_coords to verify magmom handling
echo "=== Full get_coords implementation ==="
rg -A 50 "^def get_coords" dpdata/abacus/scf.py
# Also check if there are any tests for this functionality
echo -e "\n=== Tests for magnetic moment handling ==="
rg -l "magmom" tests/
Length of output: 2500
Script:
#!/bin/bash
# Check the test implementation for magnetic moment handling
echo "=== Test implementation ==="
rg -A 10 "magmom" tests/test_abacus_stru_dump.py
# Also check if there are any test data files
echo -e "\n=== Test data files ==="
fd "stru" tests/
Length of output: 1929
dpdata/plugins/abacus.py
Outdated
| data = dpdata.abacus.scf.get_frame(file_name) | ||
| register_mag_data(data) |
There was a problem hiding this comment.
Review data handling flow in from_system
The current implementation has potential issues:
get_frameis called to get data for magnetic registration- Then
get_frame_from_struis called to return the actual frame - This double data fetch is inefficient and could lead to inconsistencies
Consider refactoring to:
def from_system(self, file_name, **kwargs):
- data = dpdata.abacus.scf.get_frame(file_name)
- register_mag_data(data)
- return dpdata.abacus.scf.get_frame_from_stru(file_name)
+ data = dpdata.abacus.scf.get_frame_from_stru(file_name)
+ register_mag_data(data)
+ return data📝 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.
| data = dpdata.abacus.scf.get_frame(file_name) | |
| register_mag_data(data) | |
| data = dpdata.abacus.scf.get_frame_from_stru(file_name) | |
| register_mag_data(data) | |
| return data |
| mysys = dpdata.System("abacus.spin/STRU.spin", fmt="abacus/stru") | ||
| self.assertTrue("spins" in mysys.data) |
There was a problem hiding this comment.
💡 Codebase verification
Path should be relative to test directory
The test file exists at tests/abacus.spin/STRU.spin, but the test is using abacus.spin/STRU.spin. This could cause issues when running tests from different directories. The test file path should be relative to the test directory.
- Update the path in
tests/test_abacus_spin.py:160to use"./abacus.spin/STRU.spin"or use proper test fixtures/helpers to handle test file paths.
🔗 Analysis chain
Verify test file existence and handling.
Let's verify the test file setup and error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify test file existence and contents
if [ -f "abacus.spin/STRU.spin" ]; then
echo "Test file exists"
head -n 10 "abacus.spin/STRU.spin"
else
echo "Warning: Test file not found"
fi
# Check for similar test files in the repository
fd -e spin "STRU"
Length of output: 176
Script:
#!/bin/bash
# Check the test file content in the correct test directory
cat tests/abacus.spin/STRU.spin
# Check if the test is using relative path correctly
rg -A 5 "abacus.spin/STRU.spin" tests/test_abacus_spin.py
# Check if there are other similar test file usages
rg "STRU.spin" tests/
Length of output: 1084
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
dpdata/abacus/scf.py (1)
261-304: LGTM! Well-structured magnetic moment transformation function.The implementation correctly handles the transformation of magnetic moments from spherical to Cartesian coordinates with proper error handling.
Consider these style improvements:
- if not (isinstance(atommag, list) or isinstance(atommag, float)): + if not isinstance(atommag, (list, float)): raise RuntimeError(f"Invalid atommag: {atommag}") - if isinstance(atommag, list): - mag_norm = np.linalg.norm(atommag) - else: - mag_norm = atommag + mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag🧰 Tools
🪛 Ruff
280-280: Multiple
isinstancecalls foratommag, merge into a single callMerge
isinstancecalls foratommag(SIM101)
295-298: Use ternary operator
mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommaginstead ofif-else-blockReplace
if-else-block withmag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag(SIM108)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
dpdata/abacus/md.py(1 hunks)dpdata/abacus/relax.py(1 hunks)dpdata/abacus/scf.py(6 hunks)dpdata/plugins/abacus.py(3 hunks)tests/abacus.scf/stru_test(1 hunks)tests/test_abacus_spin.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- dpdata/abacus/md.py
- dpdata/abacus/relax.py
- dpdata/plugins/abacus.py
- tests/test_abacus_spin.py
🧰 Additional context used
🪛 Ruff
dpdata/abacus/scf.py
280-280: Multiple isinstance calls for atommag, merge into a single call
Merge isinstance calls for atommag
(SIM101)
295-298: Use ternary operator mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag instead of if-else-block
Replace if-else-block with mag_norm = np.linalg.norm(atommag) if isinstance(atommag, list) else atommag
(SIM108)
🔇 Additional comments (4)
tests/abacus.scf/stru_test (2)
25-25: Verify the magnetic moment format and values.
The addition of magnetic moments follows the format: mag x y z where all values are set to 0. While zero magnetic moments might be intentional for this test case, we should verify:
- The format matches the ABACUS documentation
- The test covers both zero and non-zero magnetic moments
Let's check the ABACUS documentation and other test cases:
#!/bin/bash
# Description: Verify magnetic moment format and test coverage
# Search for other test files with magnetic moments
echo "Searching for other files with magnetic moment definitions..."
rg "mag\s+\d+\.\d+\s+\d+\.\d+\s+\d+\.\d+" tests/
# Search for documentation about magnetic moment format
echo "Checking for format documentation..."
fd -t f "README|\.md|\.rst" . | xargs rg -l "magnetic|moment|mag"Also applies to: 29-32
25-25:
Verify the coordinate sign changes.
The coordinates for all atoms have changed from negative to positive values. This could significantly impact the structure's geometry and potentially alter test outcomes.
For example:
- C atom: -5.192682633809 → 5.192682633809
- H atom: -5.416431453540 → 5.416431453540
Let's verify if this is an intentional change by checking other test files and documentation:
Also applies to: 29-32
dpdata/abacus/scf.py (2)
316-316: LGTM! Proper integration of magnetic moment handling.
The magnetic moment transformation and default value handling are correctly implemented.
Also applies to: 352-357
529-532: LGTM! Proper handling of magnetic moment data in frame processing.
The integration of magnetic moment data is consistent with the overall data structure.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #751 +/- ##
==========================================
+ Coverage 85.10% 85.13% +0.03%
==========================================
Files 81 81
Lines 7492 7516 +24
==========================================
+ Hits 6376 6399 +23
- Misses 1116 1117 +1 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
Bug Fixes
Tests