support spin for deepmd format #750
Conversation
📝 WalkthroughWalkthroughThe pull request introduces changes across multiple files in the 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 (
|
![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
🧹 Outside diff range and nitpick comments (11)
tests/test_deepmd_spin.py (3)
9-9: Remove extra blank lineConsider removing this extra blank line to maintain consistent spacing.
11-17: Consider usingtempfilefor safer temporary directoriesInstead of using a hardcoded temporary path, consider using Python's
tempfilemodule to create unique temporary directories. This prevents potential conflicts in parallel test runs and ensures proper cleanup.+import tempfile def setUp(self): - self.tmp_save_path = "tmp.deepmd.spin/dump-tmp" + self.tmp_dir = tempfile.mkdtemp(prefix='deepmd_spin_test_') + self.tmp_save_path = os.path.join(self.tmp_dir, "dump-tmp") def tearDown(self): - if os.path.exists(self.tmp_save_path): - shutil.rmtree(self.tmp_save_path) + if os.path.exists(self.tmp_dir): + shutil.rmtree(self.tmp_dir)
18-23: Add docstring and enhance data validationThe helper method would benefit from:
- A docstring explaining its purpose and the expected data shape
- Additional validation of data values (e.g., range checks for spins)
def check_Fe16(self, system): + """Validate spin data for Fe16 system. + + Args: + system: LabeledSystem containing Fe16 data + + Expected shapes: + - spins: (2 frames, 16 atoms, 3 components) + - force_mags: (2 frames, 16 atoms, 3 components) + """ self.assertTrue("spins" in system.data) self.assertTrue("force_mags" in system.data) self.assertEqual(system.data["spins"].shape, (2, 16, 3)) self.assertEqual(system.data["force_mags"].shape, (2, 16, 3)) + # Validate spin values are within physical bounds + self.assertTrue(abs(system.data["spins"]).max() <= 1.0, + "Spin values should be normalized")dpdata/plugins/abacus.py (2)
Line range hint
59-67: Consider aligning singular/plural usage in naming.There's an inconsistency between the data key name "force_mags" (plural) and the deepmd_name "force_mag" (singular). Consider using consistent plurality in both places.
Line range hint
59-67: Add docstring to explain the register_mag_data function.The function lacks documentation explaining its purpose, parameters, and return values. Consider adding a docstring to improve code maintainability.
Example:
def register_mag_data(data): """Register magnetic-related data types for the system. Parameters ---------- data : dict Dictionary containing system data with optional 'spins' and 'force_mags' fields Notes ----- This function registers the following optional data types: - spins: Spin vectors for each atom - force_mags: Magnetic force vectors for each atom """tests/test_abacus_spin.py (3)
87-87: Consider updating reference variable name for consistencyWhile the dictionary key has been updated to "force_mags", the reference variable name
magforces_refcould be renamed toforce_mags_reffor consistency with the new terminology.- magforces_ref = np.array( + force_mags_ref = np.array(Also applies to: 89-89, 99-99
144-144: LGTM with minor naming suggestionThe renaming is consistently applied in the test assertions. As with the test_relax method, consider renaming the
magforces_refvariable toforce_mags_reffor complete terminology consistency.- magforces_ref = np.array( + force_mags_ref = np.array(Also applies to: 146-146, 156-156
Line range hint
1-157: Test coverage is comprehensive and well-structuredThe test suite effectively covers all aspects of the spin functionality:
- Single-point calculations (SCF)
- Geometry optimization (relax)
- Molecular dynamics (MD)
Each test method validates both direct data access and format conversion, ensuring robustness of the implementation.
dpdata/abacus/md.py (1)
222-224: Consider adding docstring to clarify magnetic data structure.Since this PR adds support for spin in deepmd format, it would be helpful to document the structure and units of the magnetic data (
spinsandforce_mags) in the function's docstring. This would make it easier for users to understand how to work with the magnetic data in the deepmd format.dpdata/plugins/deepmd.py (2)
21-41: Add docstring and type hints to improve code clarity.The
register_spin()function should include documentation explaining its purpose, the data types it registers, and their expected formats.Consider adding this docstring:
def register_spin(): + """Register spin-related data types with the System and LabeledSystem classes. + + Registers two data types: + - spins: 3D array (nframes, natoms, 3) representing atomic spins + - force_mags: 3D array (nframes, natoms, 3) representing magnetic forces + + Returns + ------- + None + """
47-47: Consider optimizing multiple register_spin() calls.The
register_spin()function is called multiple times across different format classes. Consider:
- Moving the registration to a single initialization point
- Adding error handling for registration failures
Consider implementing a singleton pattern or moving the registration to the module initialization:
# At module level _SPIN_REGISTERED = False def register_spin(): """Register spin-related data types if not already registered.""" global _SPIN_REGISTERED if _SPIN_REGISTERED: return try: # ... existing registration code ... _SPIN_REGISTERED = True except Exception as e: raise RuntimeError(f"Failed to register spin data types: {e}")Also applies to: 57-57, 69-69, 98-98, 179-179, 235-236
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (18)
dpdata/abacus/md.py(1 hunks)dpdata/abacus/relax.py(1 hunks)dpdata/abacus/scf.py(1 hunks)dpdata/plugins/abacus.py(1 hunks)dpdata/plugins/deepmd.py(6 hunks)tests/test_abacus_spin.py(6 hunks)tests/test_deepmd_spin.py(1 hunks)tests/tmp.deepmd.spin/Fe16-npy/type.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-npy/type_map.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/box.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/coord.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/energy.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/force.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/force_mag.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/spin.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/type.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/type_map.raw(1 hunks)tests/tmp.deepmd.spin/Fe16-raw/virial.raw(1 hunks)
✅ Files skipped from review due to trivial changes (13)
- dpdata/abacus/relax.py
- dpdata/abacus/scf.py
- tests/tmp.deepmd.spin/Fe16-npy/type.raw
- tests/tmp.deepmd.spin/Fe16-npy/type_map.raw
- tests/tmp.deepmd.spin/Fe16-raw/box.raw
- tests/tmp.deepmd.spin/Fe16-raw/coord.raw
- tests/tmp.deepmd.spin/Fe16-raw/energy.raw
- tests/tmp.deepmd.spin/Fe16-raw/force.raw
- tests/tmp.deepmd.spin/Fe16-raw/force_mag.raw
- tests/tmp.deepmd.spin/Fe16-raw/spin.raw
- tests/tmp.deepmd.spin/Fe16-raw/type.raw
- tests/tmp.deepmd.spin/Fe16-raw/type_map.raw
- tests/tmp.deepmd.spin/Fe16-raw/virial.raw
🔇 Additional comments (7)
tests/test_deepmd_spin.py (2)
47-48: LGTM!
The main block follows the standard unittest pattern.
1-48: Verify test data availability
The tests look good and align with PR objectives. However, let's verify the existence of test data files to ensure the tests can run successfully.
dpdata/plugins/abacus.py (1)
59-61: LGTM! Verify the renaming change across the codebase.
The renaming from "mag_forces" to "force_mags" is consistent with the PR objectives and improves terminology clarity.
Let's verify that all occurrences have been updated consistently:
✅ Verification successful
The renaming from "mag_forces" to "force_mags" has been consistently applied across the codebase
The verification shows that:
- No instances of the old term "mag_forces" remain in the codebase
- The new term "force_mags" is consistently used across all relevant files:
- Test files:
tests/test_deepmd_spin.py,tests/test_abacus_spin.py - Core implementation:
dpdata/plugins/deepmd.py,dpdata/plugins/abacus.py - ABACUS format handlers:
dpdata/abacus/{md,scf,relax}.py
- Test files:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining instances of the old terminology
echo "Checking for any remaining 'mag_forces' occurrences:"
rg "mag_forces"
# Search for new terminology to verify consistent usage
echo -e "\nVerifying 'force_mags' usage:"
rg "force_mags"
Length of output: 1458
tests/test_abacus_spin.py (1)
34-34: LGTM: Consistent renaming in test_scf method
The renaming from "mag_forces" to "force_mags" is applied consistently in both direct data validation and format conversion tests.
Also applies to: 50-50
dpdata/abacus/md.py (1)
223-223: LGTM! Consistent renaming of magnetic forces key.
The renaming from "mag_forces" to "force_mags" aligns with the PR objectives and maintains consistency across the codebase.
Let's verify the consistency of this renaming across the codebase:
✅ Verification successful
Consistent renaming confirmed across the codebase
The verification shows that:
- No instances of the old key "mag_forces" remain in the codebase
- The new key "force_mags" is consistently used across:
- Test files (
tests/test_deepmd_spin.py,tests/test_abacus_spin.py) - Plugin modules (
dpdata/plugins/abacus.py,dpdata/plugins/deepmd.py) - ABACUS-related modules (
dpdata/abacus/{md.py,relax.py,scf.py})
- Test files (
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining instances of "mag_forces" and confirm consistent usage of "force_mags"
# Test 1: Search for any remaining instances of the old key name
echo "Checking for remaining instances of 'mag_forces'..."
rg "mag_forces"
# Test 2: Verify the new key name usage
echo "Verifying usage of 'force_mags'..."
rg "force_mags"
Length of output: 1460
dpdata/plugins/deepmd.py (2)
Line range hint 21-236: Implementation looks good with minor suggestions.
The implementation successfully adds spin support to the deepmd format with clean integration into the existing plugin architecture. The changes are backward compatible and align well with the PR objectives.
Consider implementing the suggested improvements for:
- Documentation (adding docstrings)
- Optimization (singleton pattern for registration)
- Test coverage verification
21-41: Verify test coverage for spin data handling.
Please ensure comprehensive test coverage for:
- Registration of spin data types
- Loading/saving of spin data across different formats
- Edge cases (empty systems, systems without spin data)
Let's verify the test coverage:
✅ Verification successful
Let me check the content of the spin-related test files to verify the test coverage:
Test coverage for spin data handling is comprehensive
The test files demonstrate thorough coverage of spin data functionality:
test_deepmd_spin.py: Tests registration, reading, and saving of spin data in both NPY and RAW formatstest_abacus_spin.py: Verifies spin data handling for SCF, relaxation, and MD calculations with precise numerical validationtest_lammps_spin.py: Tests reading/writing spin data from LAMMPS input and dump files, including edge cases like partial/missing spin data
The tests cover all essential aspects:
- Registration of spin data types
- Loading/saving across different formats (DeePMD, ABACUS, LAMMPS)
- Edge cases (zero spins, missing data)
- Data format conversions and numerical precision checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for spin-related test files and coverage
echo "Searching for spin-related test files..."
fd -e py test_.*spin tests/
echo "Checking test coverage in test files..."
rg -l "spin|force_mag" tests/
Length of output: 1959
Script:
#!/bin/bash
# Check content of spin-related test files
echo "=== tests/test_deepmd_spin.py ==="
cat tests/test_deepmd_spin.py
echo -e "\n=== tests/test_abacus_spin.py ==="
cat tests/test_abacus_spin.py
echo -e "\n=== tests/test_lammps_spin.py ==="
cat tests/test_lammps_spin.py
Length of output: 15570
CodSpeed Performance ReportMerging #750 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #750 +/- ##
==========================================
+ Coverage 85.07% 85.10% +0.02%
==========================================
Files 81 81
Lines 7478 7492 +14
==========================================
+ Hits 6362 6376 +14
Misses 1116 1116 ☔ View full report in Codecov by Sentry. |
Hello, developer! I am interested in adding spin to DP. Here, I have a question about force_mags. I don't know how to get this information from OUTCAR. Can you do some comments about this? Thank you. |
Summary by CodeRabbit
New Features
Changes
"mag_forces"to"force_mags"across multiple files for consistency.Bug Fixes