feat(tf/pt): add/refact lammps & C++ support for spin model #4321
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
* add spin nlist for tf * add UT nlist for spin * fix UT nlist for spin * add lammps spin UT * add model for deviation * fix lammps spin UT * support lammps spin MPI UT * add lammps UT for PT
Signed-off-by: Duo <50307526+iProzd@users.noreply.github.com>
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
source/api_c/tests/test_deepspin_a_hpp_tf.cc(1 hunks)source/api_cc/src/DeepPotTF.cc(1 hunks)source/api_cc/src/DeepSpinTF.cc(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/api_c/tests/test_deepspin_a_hpp_tf.cc
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
source/api_c/tests/test_deepspin_a_hpp_tf.cc (3)
40-42: Usefile_namevariable instead of hardcoding the file pathIn line 41, you're hardcoding the input file path
"../../tests/infer/deepspin_nlist.pbtxt"even though you have defined the variablefile_namein line 40. Using the variable enhances maintainability and reduces the risk of inconsistencies.Apply this diff to use the
file_namevariable:deepmd::hpp::convert_pbtxt_to_pb( - "../../tests/infer/deepspin_nlist.pbtxt", + file_name, "deepspin_nlist_hpp.pb");
49-61: Clean up commented-out code to improve readabilityThere are several blocks of commented-out code that may clutter the test and reduce readability. If these lines are no longer needed, consider removing them to keep the codebase clean.
For example, you can remove these lines:
EXPECT_EQ(natoms * 9, expected_v.size()); -// EXPECT_EQ(natoms * 9, expected_v.size()); ... expected_tot_e = 0.; -// expected_tot_v.resize(9); -// std::fill(expected_tot_v.begin(), expected_tot_v.end(), 0.); for (unsigned int ii = 0; ii < natoms; ++ii) { expected_tot_e += expected_e[ii]; } -// for (unsigned int ii = 0; ii < natoms; ++ii) { -// for (int dd = 0; dd < 9; ++dd) { -// expected_tot_v[dd] += expected_v[ii * 9 + dd]; -// } -// }
178-200: Clean up commented-out code to improve readabilitySimilar to earlier, there are commented-out sections in this part of the code. Removing unnecessary comments helps maintain a clear and concise codebase.
Consider removing these lines:
EXPECT_EQ(natoms * 9, expected_v.size()); -// EXPECT_EQ(natoms * 9, expected_v.size()); ... expected_tot_e = 0.; -// expected_tot_v.resize(9); -// std::fill(expected_tot_v.begin(), expected_tot_v.end(), 0.); for (unsigned int ii = 0; ii < natoms; ++ii) { expected_tot_e += expected_e[ii]; } -// for (unsigned int ii = 0; ii < natoms; ++ii) { -// for (int dd = 0; dd < 9; ++dd) { -// expected_tot_v[dd] += expected_v[ii * 9 + dd]; -// } -// }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
source/api_c/tests/test_deepspin_a_hpp_tf.cc(1 hunks)
🔇 Additional comments (1)
source/api_c/tests/test_deepspin_a_hpp_tf.cc (1)
152-204: Refactor to eliminate code duplication between test classes
The TestInferDeepSpinTFAHPP and TestInferDeepSpinTFANoPbcHPP classes share a significant amount of duplicated code. Refactoring by creating a common base class or using parameterized tests would enhance maintainability and reduce redundancy.
This issue was previously noted, and addressing it would improve the code structure.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
source/api_c/tests/test_deepspin_a_hpp_tf.cc (2)
35-35: Clean up commented virial-related code.There are multiple commented sections related to virial calculations throughout the test class. If this functionality is planned for future implementation, consider:
- Adding a TODO comment explaining the plan
- Moving the commented code to a separate branch or issue
- If the functionality is no longer needed, remove the commented code entirely
Also applies to: 77-78, 89-90, 98-100, 112-113, 124-126, 135-137, 141-143
241-265: Document the purpose of cpu_lmp_nlist test.The test appears to validate neighbor list functionality with non-periodic boundary conditions, but its purpose and setup could be better documented. Consider adding comments explaining:
- Why this test is specific to the non-periodic case
- The significance of the neighbor list data structure
- The expected behavior differences compared to periodic boundary conditions
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
source/api_c/tests/test_deepspin_a_hpp_tf.cc(1 hunks)
🔇 Additional comments (2)
source/api_c/tests/test_deepspin_a_hpp_tf.cc (2)
1-11: LGTM!
The file structure and includes are well-organized and follow best practices.
1-280: LGTM with suggestions!
The test implementation is thorough and covers both periodic and non-periodic boundary conditions with appropriate assertions. Consider addressing the commented code cleanup and documentation improvements mentioned above.
The previous review's suggestion about reducing code duplication between test classes remains valid.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (33)
source/api_cc/tests/test_deepspin_model_devi.cc (2)
19-23: Consider moving test data to fixture files.The hardcoded test data could be moved to fixture files to improve maintainability and allow for easier updates of test cases.
Consider creating a test fixture file and loading it in SetUp:
// test_fixtures/spin_model_devi_data.h struct SpinModelDeviTestData { static const std::vector<VALUETYPE> coord; static const std::vector<VALUETYPE> spin; // ... other data };
59-66: Enhance test failure messages for better debugging.The equality checks would benefit from more descriptive failure messages to help identify issues quickly.
Consider using EXPECT_EQ with custom messages:
- EXPECT_EQ(dp0.cutoff(), dp_md.cutoff()); + EXPECT_EQ(dp0.cutoff(), dp_md.cutoff()) + << "Cutoff mismatch between dp0 and dp_md";source/api_c/include/c_api.h (1)
386-432: Documentation needs improvement for array dimensions.The
spinparameter's documentation should more clearly specify the array dimensions. While it mentions "nframes x natoms x 3", it would be helpful to clarify:
- The total size of the array (nframes * natoms * 3)
- That each spin vector is 3D (x, y, z components)
- The expected range or normalization of spin values
source/api_c/src/c_api.cc (2)
134-186: Add documentation for the new spin model classes.While the implementation is correct, adding documentation for the new classes would improve maintainability. Consider adding:
- Class purpose and responsibilities
- Parameter descriptions
- Usage examples
+/** + * @brief Deep learning model for spin configurations + * + * Inherits from DP_DeepBaseModel to provide spin-specific functionality + */ DP_DeepSpin::DP_DeepSpin() {} +/** + * @brief Model deviation calculator for spin configurations + * + * Inherits from DP_DeepBaseModelDevi to provide spin-specific deviation calculations + */ DP_DeepSpinModelDevi::DP_DeepSpinModelDevi() {}
Line range hint
48-2180: Consider enhancing error handling and documentation.The spin model implementation is well-structured and follows good practices. To further improve the codebase:
- Implement consistent input validation across all compute functions
- Document the spin model functionality comprehensively
- Consider adding runtime checks for spin vector normalization
- Add unit tests specifically for error cases
source/api_c/include/deepmd.hpp (5)
970-1002: Consider adding bounds checking for parameter validationThe
validate_fparam_aparammethod could benefit from additional validation:
- Check for negative values in nframes and nloc
- Validate that fparam and aparam vectors are not empty when required
template <typename VALUETYPE> void validate_fparam_aparam(const int &nframes, const int &nloc, const std::vector<VALUETYPE> &fparam, const std::vector<VALUETYPE> &aparam) const { + if (nframes <= 0 || nloc <= 0) { + throw deepmd::hpp::deepmd_exception( + "nframes and nloc must be positive"); + } if (fparam.size() != dfparam && fparam.size() != static_cast<size_t>(nframes) * dfparam) { throw deepmd::hpp::deepmd_exception(
2600-2602: Commented code should be removed or justifiedThere are commented-out sections in the virial calculations. If these are intentionally disabled, add a comment explaining why. If they're no longer needed, remove them.
- // for (int j = 0; j < 9; j++) { - // virial[i][j] = virial_flat[i * 9 + j]; - // } + // Virial calculations temporarily disabled for spin models + // TODO: Implement proper virial tensor for magnetic systemsAlso applies to: 2709-2711
2766-2766: Fix typo in commentThe word "continuous" is misspelled as "continous".
- // memory will be continous for std::vector but not std::vector<std::vector> + // memory will be continuous for std::vector but not std::vector<std::vector>
2503-2511: Consider adding error recovery mechanismThe initialization of DeepSpinModelDevi could benefit from a more robust error handling mechanism:
- Add state validation after DP_NewDeepSpinModelDeviWithParam
- Consider implementing a rollback mechanism if subsequent initializations fail
dp = DP_NewDeepSpinModelDeviWithParam( cstrings.data(), cstrings.size(), gpu_rank, c_file_contents.data(), c_file_contents.size(), size_file_contents.data()); + if (!dp) { + throw deepmd::hpp::deepmd_exception("Failed to create DeepSpinModelDevi"); + } DP_CHECK_OK(DP_DeepSpinModelDeviCheckOK, dp); numb_models = models.size(); dfparam = DP_DeepSpinModelDeviGetDimFParam(dp); daparam = DP_DeepSpinModelDeviGetDimAParam(dp); aparam_nall = DP_DeepSpinModelDeviIsAParamNAll(dp); dpbase = (DP_DeepBaseModelDevi *)dp;
1556-1566: Consider optimizing memory allocation patternThe current implementation allocates vectors individually. Consider using a single allocation for better memory locality:
- std::vector<VALUETYPE> fparam_, aparam_; - validate_fparam_aparam(nframes, natoms, fparam, aparam); - tile_fparam_aparam(fparam_, nframes, dfparam, fparam); - tile_fparam_aparam(aparam_, nframes, natoms * daparam, aparam); + // Pre-calculate sizes + const size_t fparam_size = nframes * dfparam; + const size_t aparam_size = nframes * natoms * daparam; + // Single allocation + std::vector<VALUETYPE> params(fparam_size + aparam_size); + auto fparam_ = params.data(); + auto aparam_ = fparam_ + fparam_size;source/api_c/tests/test_deepspin_a_hpp_tf.cc (3)
39-44: Consider adding error handling for file operationsWhen performing file operations like converting and initializing the model file, it's good practice to check for potential errors that may occur during these operations. This ensures that any issues are caught early and handled appropriately.
63-63: Handle potential errors in file removalThe
removefunction returns a value indicating success or failure. Checking this return value can help detect situations where the file may not be deleted properly.
49-60: Clean up commented-out codeThere are several sections of code that are commented out. If these code segments are no longer needed, consider removing them to improve readability and maintainability.
source/api_cc/include/DeepSpin.h (13)
19-20: Use= defaultfor default constructor and destructorThe default constructor and destructor can be declared using
= default;for better clarity and to signal the compiler to generate default implementations.Apply this diff:
- DeepSpinBackend() {}; - virtual ~DeepSpinBackend() {}; + DeepSpinBackend() = default; + virtual ~DeepSpinBackend() = default;
29-30: Pass primitive types by value instead of const referencePassing primitive types like
intbyconst int&may have unnecessary overhead compared to passing by value. Consider changingconst int& gpu_ranktoint gpu_rank.Apply this diff:
- DeepSpinBackend(const std::string& model, - const int& gpu_rank = 0, - const std::string& file_content = ""); + DeepSpinBackend(const std::string& model, + int gpu_rank = 0, + const std::string& file_content = "");
39-40: Pass primitive types by value instead of const referenceSimilarly, in the
initmethod, consider passinggpu_rankby value instead ofconst int&.Apply this diff:
- virtual void init(const std::string& model, - const int& gpu_rank = 0, - const std::string& file_content = "") = 0; + virtual void init(const std::string& model, + int gpu_rank = 0, + const std::string& file_content = "") = 0;
183-184: Pass primitive types by value instead of const referenceIn the
DeepSpinconstructor, consider changingconst int& gpu_ranktoint gpu_rank.Apply this diff:
- DeepSpin(const std::string& model, - const int& gpu_rank = 0, - const std::string& file_content = ""); + DeepSpin(const std::string& model, + int gpu_rank = 0, + const std::string& file_content = "");
193-194: Pass primitive types by value instead of const referenceIn the
initmethod ofDeepSpin, consider passinggpu_rankby value.Apply this diff:
- void init(const std::string& model, - const int& gpu_rank = 0, - const std::string& file_content = ""); + void init(const std::string& model, + int gpu_rank = 0, + const std::string& file_content = "");
436-437: Pass primitive types by value instead of const referenceIn the
DeepSpinModelDeviconstructor, consider passinggpu_rankby value.Apply this diff:
- DeepSpinModelDevi(const std::vector<std::string>& models, - const int& gpu_rank = 0, - const std::vector<std::string>& file_contents = std::vector<std::string>()); + DeepSpinModelDevi(const std::vector<std::string>& models, + int gpu_rank = 0, + const std::vector<std::string>& file_contents = std::vector<std::string>());
447-448: Pass primitive types by value instead of const referenceIn the
initmethod ofDeepSpinModelDevi, consider passinggpu_rankby value.Apply this diff:
- void init(const std::vector<std::string>& models, - const int& gpu_rank = 0, - const std::vector<std::string>& file_contents = std::vector<std::string>()); + void init(const std::vector<std::string>& models, + int gpu_rank = 0, + const std::vector<std::string>& file_contents = std::vector<std::string>());
440-440: Fix typo in commentThe word "contrcutor" should be corrected to "constructor".
Apply this diff:
- * @brief Initialize the DP model deviation contrcutor. + * @brief Initialize the DP model deviation constructor.
470-471: Remove redundant text in commentThe comment repeats "Then all frames and atoms are provided with the same aparam." This redundancy can be removed for clarity.
Apply this diff:
- *same aparam. Then all frames are assumed to be provided with the - *same aparam. + *same aparam. Then all frames are assumed to be provided with the same aparam.
508-509: Remove redundant text in commentSimilar redundancy in the comment regarding
aparam. The text "dim_aparam." seems out of place.Apply this diff:
- *same aparam. dim_aparam. Then all frames and atoms are provided with the - *same aparam. + *same aparam. Then all frames and atoms are provided with the same aparam.
550-551: Remove redundant text in commentThe comment repeats and includes "dim_aparam." which may be a typo.
Apply this diff:
- *same aparam. dim_aparam. Then all frames and atoms are provided with the - *same aparam. + *same aparam. Then all frames and atoms are provided with the same aparam.
593-594: Remove redundant text in commentRedundant and possibly incorrect text in the comment can be corrected for clarity.
Apply this diff:
- *same aparam. dim_aparam. Then all frames and atoms are provided with the - *same aparam. + *same aparam. Then all frames and atoms are provided with the same aparam.
421-421: Consider renamingDeepSpinModelDevifor clarityThe class name
DeepSpinModelDevimight be unclear to readers. IfDevistands for "Deviation" or another term, consider using the full word for better readability.source/api_cc/src/DeepSpin.cc (7)
33-38: Consider handling multiple initializations more robustlyCurrently, the
initmethod checks if the object is already initialized and, if so, prints a warning tostd::cerrand returns without re-initializing. This behavior might lead to unexpected results if re-initialization is required. Consider throwing an exception to prevent misuse or allowing re-initialization by resetting the internal state appropriately.
40-46: Avoid relying solely on file extensions to determine the backendRelying on file extensions (e.g.,
.pthfor PyTorch,.pbfor TensorFlow) to determine the backend can be error-prone if users provide files with incorrect or missing extensions. Consider accepting the backend type explicitly as a parameter or implementing a more robust method to detect the backend from the file content.
71-86: Ensure consistent naming conventions for function parametersThe parameter names in the
computemethods use trailing underscores inconsistently (e.g.,dforce_,fparam_,aparam_). For improved readability and maintainability, consider standardizing the naming convention across all parameters, such as using a consistent prefix or suffix.
471-476: Handle multiple initializations ofDeepSpinModelDevimore robustlySimilar to the
DeepSpinclass, theinitmethod ofDeepSpinModelDeviprints a warning and returns if called multiple times. To prevent potential misuse, consider throwing an exception or allowing re-initialization by properly resetting the object's state.
478-480: Provide a more informative exception message when no models are specifiedThe exception message "no model is specified" may lack sufficient context. Consider enhancing it to help users understand the error, such as: "DeepSpinModelDevi initialization failed: No models were provided. Please specify at least one model."
483-488: Use appropriate data types for loop indicesThe loop over
numb_modelsusesunsigned intfor the indexii. It's recommended to usestd::size_tfor indexing, as it is the standard type for sizes and indices, ensuring portability and correctness across different platforms.
512-516: Consider parallelizing computations over multiple modelsIn the
computemethods ofDeepSpinModelDevi, computations over multiple models are performed sequentially. If thread safety and resource management allow, consider parallelizing these loops using multithreading techniques (e.g., OpenMP, std::thread) to improve performance, especially when handling a large number of models.Also applies to: 567-570, 626-629, 692-695
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
source/api_c/include/c_api.h(20 hunks)source/api_c/include/deepmd.hpp(22 hunks)source/api_c/src/c_api.cc(15 hunks)source/api_c/tests/test_deepspin_a_hpp_tf.cc(1 hunks)source/api_c/tests/test_deepspin_model_devi_hpp.cc(1 hunks)source/api_cc/include/DeepSpin.h(1 hunks)source/api_cc/src/DeepSpin.cc(1 hunks)source/api_cc/tests/test_deepspin_model_devi.cc(1 hunks)
🔇 Additional comments (23)
source/api_c/tests/test_deepspin_model_devi_hpp.cc (3)
1-11: LGTM! Headers and includes are well-organized.
The file includes all necessary headers and follows proper licensing conventions.
43-44: LGTM! Test suite setup is correct.
Properly uses Google Test's typed test suite functionality.
45-162: 🛠️ Refactor suggestion
Test cases need improvements in robustness and clarity.
- Several commented-out assertions suggest incomplete testing of virial-related functionality.
- The magic number
EPSILONis used without clear definition. - Test case names could be more descriptive.
- Consider potential memory leaks in compute calls.
Consider these improvements:
- Either implement the virial tests or add TODO comments explaining why they're commented out
- Define EPSILON as a constant with a clear purpose
- Rename test cases to better reflect what they're testing
- Add RAII or try-catch blocks for compute calls
+// Tolerance for floating-point comparisons
+constexpr double EPSILON = 1e-10;
-TYPED_TEST(TestInferDeepSpinModeDevi, cpu_build_nlist)
+TYPED_TEST(TestInferDeepSpinModeDevi, ComputeEnergyForcesAndMagneticForces)
// TODO: Virial tests are currently disabled pending implementation of...
// EXPECT_EQ(vdir.size(), vmd.size());Let's verify if virial computations are implemented in the codebase:
source/api_cc/tests/test_deepspin_model_devi.cc (2)
93-93: Document or remove commented-out virial tests.
There are multiple commented-out virial test assertions. If these tests are temporarily disabled, please document why. If they're no longer needed, consider removing them.
Let's check if virial calculations are implemented elsewhere:
#!/bin/bash
# Search for virial-related tests and implementations
rg -l "virial|vir" --type cpp --type hppAlso applies to: 97-97, 107-109, 138-138, 140-140, 144-144, 146-146, 156-158, 162-164
39-41: Consider using different model files for effective model deviation testing.
Using the same model file twice (deeppot_dpa_spin.pth) for model deviation testing may not effectively test the deviation calculation logic, as both models would produce identical results.
Let's verify if there are other model files available:
source/api_c/include/c_api.h (4)
15-15: LGTM: API version bump is appropriate.
The increment from version 23 to 24 correctly reflects the addition of significant new functionality related to spin models.
97-115: LGTM: Well-structured base model implementation.
The base model structures and methods provide a solid foundation with:
- Clear separation between model and model deviation types
- Consistent memory management with creation/deletion functions
- Comprehensive accessor methods for model properties
Also applies to: 1388-1481
171-206: LGTM: Comprehensive spin model implementation.
The spin model implementation is well-structured with:
- Consistent API patterns following the base model design
- Complete set of computation functions for both double and float precision
- Proper memory management functions
- Comprehensive utility methods for model properties
Also applies to: 386-432, 1629-1755
Line range hint 1518-1627: Add missing API version tags for consistency.
Several methods are missing the "@SInCE API version 24" tag that should be added for consistency with other new functions. For example:
DP_DeepPotGetCutoffDP_DeepPotGetNumbTypes- Other methods in this block
This aligns with njzjz's previous comment about adding version tags to new functions.
source/api_c/src/c_api.cc (5)
48-66: Well-structured base class implementation!
The introduction of base classes DP_DeepBaseModel and DP_DeepBaseModelDevi follows good object-oriented design principles by:
- Extracting common functionality and parameters
- Properly initializing members in constructors
- Following consistent naming conventions
318-408: Add input validation for spin computations.
The past review comments about input validation are still valid. Consider adding safety checks:
#!/bin/bash
# Description: Check if input validation is consistently applied across similar functions
# Test: Search for input validation patterns in compute functions
rg -A 2 "if \(!coord|!spin|!atype\)" --type cpp509-609: Add neighbor list validation for spin computations.
The past review comments about neighbor list validation are still valid.
1989-2180: Well-implemented utility methods!
The base model utility methods are properly implemented with:
- Consistent patterns across all methods
- Proper string handling
- Clear parameter access functions
862-876: Address commented-out virial computations.
The virial computations are commented out without explanation. Either:
- Remove the commented code if it's no longer needed
- Add a TODO comment explaining why it's commented out and when it will be implemented
- Implement the virial computations if they are required
source/api_c/include/deepmd.hpp (2)
898-969: LGTM: Well-designed base class implementation
The DeepBaseModel class provides a solid foundation with:
- Clean virtual destructor for proper inheritance
- Consistent error checking with assertions
- Clear getter methods for model properties
1452-1804: Verify thread safety in DeepSpin implementation
The DeepSpin class handles spin model computations but should ensure thread safety:
- The dp pointer is modified in multiple methods
- Multiple compute() methods access shared resources
source/api_c/tests/test_deepspin_a_hpp_tf.cc (7)
68-101: Test case 'cpu_build_nlist' is well-implemented
The test case correctly initializes variables, invokes the dp.compute method, and asserts the expected results, ensuring the functionality is thoroughly tested.
103-144: Test case 'cpu_build_nlist_atomic' is comprehensive
This test effectively checks the atomic energy calculations and force outputs, properly validating the behavior of the dp.compute method with atomic parameters.
146-149: Print summary function is correctly invoked
The print_summary method is called without issues, ensuring that the summary functionality of the DeepSpin class is operational.
151-204: Duplicate code detected between test classes
The TestInferDeepSpinTFAHPP and TestInferDeepSpinTFANoPbcHPP classes share significant portions of code. Refer to previous review comments for suggestions on refactoring to reduce duplication.
207-239: Test case 'cpu_build_nlist' without PBC is well-structured
The test accurately assesses the functionality when periodic boundary conditions are not applied, ensuring robustness in different simulation scenarios.
241-280: Test case 'cpu_lmp_nlist' effectively utilizes neighbor lists
The test correctly constructs neighbor lists and verifies the dp.compute method's behavior with explicit neighbor information.
282-324: Test case 'cpu_lmp_nlist_atomic' thoroughly checks atomic calculations
This test ensures that atomic energies and forces are correctly computed when using neighbor lists, providing confidence in the method's accuracy.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
source/api_c/tests/test_deepspin_model_devi_hpp.cc (3)
61-66: Document or implement commented assertions.Several assertions are commented out. If these are for future functionality, they should be documented.
Consider either:
- Removing the commented assertions if they're not applicable
- Adding TODO comments explaining when these will be implemented
- Implementing them if the functionality exists
- // EXPECT_EQ(dp0.dim_fparam(), dp_md.dim_fparam()); - // EXPECT_EQ(dp0.dim_aparam(), dp_md.dim_aparam()); + // TODO(iProzd): Implement these assertions when fparam/aparam support is added + // EXPECT_EQ(dp0.dim_fparam(), dp_md.dim_fparam()); + // EXPECT_EQ(dp0.dim_aparam(), dp_md.dim_aparam());
69-111: Reduce code duplication in test cases.The
cpu_build_nlistandcpu_build_nlist_atomictests have significant code duplication in setup and verification.Consider extracting common setup and verification logic into helper methods:
protected: // Helper to set up test vectors void setupTestVectors(int nmodel, std::vector<double>& edir, std::vector<double>& emd, std::vector<std::vector<VALUETYPE>>& fdir, std::vector<std::vector<VALUETYPE>>& fmagdir, std::vector<std::vector<VALUETYPE>>& vdir, std::vector<std::vector<VALUETYPE>>& fmd, std::vector<std::vector<VALUETYPE>>& fmmagd, std::vector<std::vector<VALUETYPE>>& vmd) { edir.resize(nmodel); fdir.resize(nmodel); fmagdir.resize(nmodel); vdir.resize(nmodel); } // Helper to verify results void verifyResults(const std::vector<double>& edir, const std::vector<double>& emd, const std::vector<std::vector<VALUETYPE>>& fdir, const std::vector<std::vector<VALUETYPE>>& fmd, const std::vector<std::vector<VALUETYPE>>& fmagdir, const std::vector<std::vector<VALUETYPE>>& fmmagd, int nmodel) { EXPECT_EQ(edir.size(), emd.size()); EXPECT_EQ(fdir.size(), fmd.size()); EXPECT_EQ(fmagdir.size(), fmmagd.size()); for (int kk = 0; kk < nmodel; ++kk) { EXPECT_LT(fabs(edir[kk] - emd[kk]), EPSILON); for (int ii = 0; ii < fdir[0].size(); ++ii) { EXPECT_LT(fabs(fdir[kk][ii] - fmd[kk][ii]), EPSILON); } for (int ii = 0; ii < fmagdir[0].size(); ++ii) { EXPECT_LT(fabs(fmagdir[kk][ii] - fmmagd[kk][ii]), EPSILON); } } }Also applies to: 113-166
100-106: Define or document the EPSILON constant.The EPSILON constant is used for floating-point comparisons but its value and purpose are not documented.
Consider adding a clear definition:
+// Maximum allowed difference between expected and actual floating-point values +constexpr double EPSILON = 1e-10;
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
source/api_c/tests/test_deepspin_model_devi_hpp.cc (1)
44-44: Remove empty TearDown method.The empty
TearDownmethod can be removed as it doesn't serve any purpose.- void TearDown() override {};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
source/api_c/tests/test_deepspin_model_devi_hpp.cc(1 hunks)
🔇 Additional comments (4)
source/api_c/tests/test_deepspin_model_devi_hpp.cc (4)
69-111:
Enable commented assertions and improve test coverage.
-
Most assertions are commented out, which significantly reduces test coverage.
-
The test name suggests it's testing neighbor list building, but it focuses on compute results.
-
The test uses
natomswhich was previously uninitialized. -
Enable the commented assertions after fixing the initialization of
natoms. -
Consider renaming the test to better reflect its purpose (e.g.,
compute_consistency). -
Add explicit tests for neighbor list building.
#!/bin/bash
# Description: Check if DeepSpinModelDevi::compute is implemented
# Test: Search for the compute method implementation
ast-grep --pattern $'class DeepSpinModelDevi {
$$$
compute($$$) {
$$$
}
$$$
}'113-168: Verify status of atomic-level computations.
The entire atomic-level test case is commented out. Please verify if atomic-level computations are implemented in DeepSpinModelDevi and either:
- Enable the test if the functionality is implemented, or
- Document why the test is disabled if the functionality is not yet ready.
#!/bin/bash
# Description: Check if atomic-level compute methods are implemented
# Test: Search for atomic compute methods in DeepSpinModelDevi
ast-grep --pattern $'class DeepSpinModelDevi {
$$$
compute($$$, $$$, $$$, $$$, $$$, $$$, $$$) {
$$$
}
$$$
}'15-21:
Initialize natoms and document test data structure.
The test data structure needs documentation and natoms remains uninitialized. This could lead to undefined behavior.
Apply this diff:
protected:
+ // 4 atoms with 3D coordinates (x,y,z) in a periodic box
std::vector<VALUETYPE> coord = {12.83, 2.56, 2.18, 12.09, 2.87, 2.74,
3.51, 2.51, 2.60, 4.27, 3.22, 1.56};
+ // 4 atoms with 3D spin vectors, first two atoms have z-component = 1.2737
std::vector<VALUETYPE> spin = {0., 0., 1.2737, 0., 0., 1.2737,
0., 0., 0., 0., 0., 0.};
std::vector<int> atype = {0, 0, 1, 1};
std::vector<VALUETYPE> box = {13., 0., 0., 0., 13., 0., 0., 0., 13.};
- int natoms;
+ const int natoms = 4; // Number of atoms in the test systemLikely invalid or redundant comment.
61-62: Verify if commented assertions can be enabled.
Several attribute checks are commented out. Please verify if these assertions for dim_fparam and dim_aparam can be enabled or document why they are disabled.
Also applies to: 65-66
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (9)
source/lmp/tests/test_lammps_spin_pt.py (4)
98-103: Add error handling for file operations.The setup and teardown functions should handle potential file operation errors.
def setup_module(): - write_lmp_data_spin(box, coord, spin, type_NiO, data_file) + try: + write_lmp_data_spin(box, coord, spin, type_NiO, data_file) + except Exception as e: + raise RuntimeError(f"Failed to write test data file: {e}") from e def teardown_module(): - os.remove(data_file) + try: + if data_file.exists(): + os.remove(data_file) + except Exception as e: + print(f"Warning: Failed to remove test data file: {e}")
106-127: Simplify unit validation in _lammps helper.The function has redundant unit validation checks that can be simplified.
def _lammps(data_file, units="metal") -> PyLammps: + if units != "metal": + raise ValueError("units for spin should be metal") lammps = PyLammps() lammps.units(units) lammps.boundary("p p p") lammps.atom_style("spin") - if units == "metal": - lammps.neighbor("2.0 bin") - else: - raise ValueError("units for spin should be metal") + lammps.neighbor("2.0 bin") lammps.neigh_modify("every 10 delay 0 check no") lammps.read_data(data_file.resolve()) - if units == "metal": - lammps.mass("1 58") - lammps.mass("2 16") - else: - raise ValueError("units for spin should be metal") - if units == "metal": - lammps.timestep(0.0005) - else: - raise ValueError("units for spin should be metal") + lammps.mass("1 58") + lammps.mass("2 16") + lammps.timestep(0.0005) lammps.fix("1 all nve") return lammps
149-187: Clean up commented code in test_pair_deepmd_virial.The function contains commented out code that should either be removed or implemented.
Consider either:
- Removing the commented code if it's no longer needed
- Implementing the virial tests if they are required
- Adding a TODO comment explaining why these tests are commented out
189-285: Refactor duplicate model deviation calculations.The model deviation calculation logic is duplicated across test functions. Consider extracting it into a helper function.
+def calculate_model_deviation(expected_f, expected_f2, expected_fm, expected_fm2, relative=None): + norm = np.linalg.norm(np.mean([expected_f, expected_f2], axis=0), axis=1) + norm_spin = np.linalg.norm(np.mean([expected_fm, expected_fm2], axis=0), axis=1) + expected_md_f = np.linalg.norm(np.std([expected_f, expected_f2], axis=0), axis=1) + expected_md_fm = np.linalg.norm(np.std([expected_fm, expected_fm2], axis=0), axis=1) + if relative is not None: + expected_md_f /= norm + relative + expected_md_fm /= norm_spin + relative + return expected_md_f, expected_md_fm def test_pair_deepmd_model_devi(lammps): # ... existing setup code ... - expected_md_f = np.linalg.norm(np.std([expected_f, expected_f2], axis=0), axis=1) - expected_md_fm = np.linalg.norm(np.std([expected_fm, expected_fm2], axis=0), axis=1) + expected_md_f, expected_md_fm = calculate_model_deviation( + expected_f, expected_f2, expected_fm, expected_fm2 + ) # ... rest of the test ... def test_pair_deepmd_model_devi_atomic_relative(lammps): # ... existing setup code ... - norm = np.linalg.norm(np.mean([expected_f, expected_f2], axis=0), axis=1) - norm_spin = np.linalg.norm(np.mean([expected_fm, expected_fm2], axis=0), axis=1) - expected_md_f = np.linalg.norm(np.std([expected_f, expected_f2], axis=0), axis=1) - expected_md_f /= norm + relative - expected_md_fm = np.linalg.norm(np.std([expected_fm, expected_fm2], axis=0), axis=1) - expected_md_fm /= norm_spin + relative + expected_md_f, expected_md_fm = calculate_model_deviation( + expected_f, expected_f2, expected_fm, expected_fm2, relative + ) # ... rest of the test ...source/lmp/tests/test_lammps_spin.py (4)
1-92: LGTM! Consider adding docstring documentation.The imports, constants, and test data setup are well-organized. The expected values and test configurations are precisely defined.
Consider adding a module-level docstring explaining the purpose of these tests and the physical system being tested (appears to be a NiO system with 2 Ni atoms with spins and 2 O atoms without spins).
102-108: Add comprehensive error handling and cleanup.The setup/teardown functions should handle errors and clean up all generated files.
Consider enhancing the functions:
def setup_module(): - write_lmp_data_spin(box, coord, spin, type_NiO, data_file) + try: + write_lmp_data_spin(box, coord, spin, type_NiO, data_file) + except Exception as e: + pytest.fail(f"Failed to write LAMMPS data: {e}") def teardown_module(): - os.remove(data_file) + for file in [data_file, pb_file, pb_file2]: + try: + if file.exists(): + file.unlink() + except Exception as e: + print(f"Warning: Failed to remove {file}: {e}", file=sys.stderr)
153-191: Clean up or restore commented code.The test contains a significant block of commented code related to pressure and virial calculations. Either:
- Remove the commented code if it's no longer needed
- Restore and fix the code if these tests are still required
193-214: Add documentation for model deviation calculations.The model deviation tests contain complex calculations without explanation. Consider adding docstrings explaining:
- The purpose of model deviation testing
- The meaning of the calculated metrics
- The significance of the relative parameter
Also applies to: 216-241
source/api_c/include/deepmd.hpp (1)
Line range hint
3187-3196: Enhance error handling in read_file_to_string.Consider adding more specific error handling:
- Check if file exists before reading
- Handle file size limits
- Add specific exception types for different error cases
Example enhancement:
void inline read_file_to_string(std::string model, std::string &file_content) { + if (!std::filesystem::exists(model)) { + throw deepmd::hpp::deepmd_exception("Model file not found: " + model); + } int size; const char *c_file_content = DP_ReadFileToChar2(model.c_str(), &size); if (size < 0) { // negative size indicates error std::string error_message = std::string(c_file_content, -size); DP_DeleteChar(c_file_content); throw deepmd::hpp::deepmd_exception(error_message); } + if (size > MAX_FILE_SIZE) { + DP_DeleteChar(c_file_content); + throw deepmd::hpp::deepmd_exception("File size exceeds maximum allowed"); + } file_content = std::string(c_file_content, size); DP_DeleteChar(c_file_content); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
source/api_c/include/deepmd.hpp(22 hunks)source/lmp/tests/test_lammps_spin.py(1 hunks)source/lmp/tests/test_lammps_spin_pt.py(1 hunks)
🔇 Additional comments (6)
source/lmp/tests/test_lammps_spin_pt.py (2)
1-91: LGTM! Well-structured imports and test constants.
The imports are properly organized, and the test constants are well-defined with clear naming conventions.
240-250: LGTM! Well-structured MPI test configuration.
The MPI test is properly configured with appropriate skip conditions and parametrization.
source/lmp/tests/test_lammps_spin.py (2)
134-139: LGTM! Well-implemented fixture.
The fixture properly manages LAMMPS instance lifecycle with appropriate cleanup.
244-289: LGTM! Well-implemented MPI test.
The MPI test is well-structured with:
- Proper environment checks
- Parameterized balance testing
- Appropriate temporary file handling
- Comprehensive assertions
source/api_c/include/deepmd.hpp (2)
895-1003: LGTM! Well-designed base class implementation.
The DeepBaseModel class provides a solid foundation with:
- Clean interface for derived classes
- Proper virtual destructor
- Strong parameter validation
- Good documentation
1527-1567:
Add spin vector dimension validation.
The compute method should validate spin vector dimensions as noted in the past review. This is still a valid concern that needs to be addressed.
Apply this diff to add validation:
unsigned int natoms = atype.size();
unsigned int nframes = natoms > 0 ? coord.size() / natoms / 3 : 1;
assert(nframes * natoms * 3 == coord.size());
+ assert(nframes * natoms * 3 == spin.size() && "Spin vector must have same dimensions as coordinates");
if (!box.empty()) {
assert(box.size() == nframes * 9);
}![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"pressure{jj} equal c_pressure[{ii+1}]") | ||
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"virial{jj} atom c_virial[{ii+1}]") |
Check notice
Code scanning / CodeQL
Commented-out code
| # for ii in range(6): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # assert np.array( | ||
| # lammps.variables[f"pressure{jj}"].value | ||
| # ) / constants.nktv2p == pytest.approx( | ||
| # -expected_v[idx_map, jj].sum(axis=0) / vol |
Check notice
Code scanning / CodeQL
Commented-out code
| # for ii in range(9): | ||
| # assert np.array( | ||
| # lammps.variables[f"virial{ii}"].value | ||
| # ) / constants.nktv2p == pytest.approx(expected_v[idx_map, ii]) |
Check notice
Code scanning / CodeQL
Commented-out code
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"pressure{jj} equal c_pressure[{ii+1}]") | ||
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"virial{jj} atom c_virial[{ii+1}]") |
Check notice
Code scanning / CodeQL
Commented-out code
| # for ii in range(6): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # assert np.array( | ||
| # lammps.variables[f"pressure{jj}"].value | ||
| # ) / constants.nktv2p == pytest.approx( | ||
| # -expected_v[idx_map, jj].sum(axis=0) / vol |
Check notice
Code scanning / CodeQL
Commented-out code
| # for ii in range(9): | ||
| # assert np.array( | ||
| # lammps.variables[f"virial{ii}"].value | ||
| # ) / constants.nktv2p == pytest.approx(expected_v[idx_map, ii]) |
Check notice
Code scanning / CodeQL
Commented-out code
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"pressure{jj} equal c_pressure[{ii+1}]") | ||
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"virial{jj} atom c_virial[{ii+1}]") |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| # for ii in range(6): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # assert np.array( | ||
| # lammps.variables[f"pressure{jj}"].value | ||
| # ) / constants.nktv2p == pytest.approx( | ||
| # -expected_v[idx_map, jj].sum(axis=0) / vol |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| # for ii in range(9): | ||
| # assert np.array( | ||
| # lammps.variables[f"virial{ii}"].value | ||
| # ) / constants.nktv2p == pytest.approx(expected_v[idx_map, ii]) |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"pressure{jj} equal c_pressure[{ii+1}]") | ||
| # for ii in range(9): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # lammps.variable(f"virial{jj} atom c_virial[{ii+1}]") |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| # for ii in range(6): | ||
| # jj = [0, 4, 8, 3, 6, 7, 1, 2, 5][ii] | ||
| # assert np.array( | ||
| # lammps.variables[f"pressure{jj}"].value | ||
| # ) / constants.nktv2p == pytest.approx( | ||
| # -expected_v[idx_map, jj].sum(axis=0) / vol |
Check notice
Code scanning / CodeQL
Commented-out code Note test
| # for ii in range(9): | ||
| # assert np.array( | ||
| # lammps.variables[f"virial{ii}"].value | ||
| # ) / constants.nktv2p == pytest.approx(expected_v[idx_map, ii]) |
Check notice
Code scanning / CodeQL
Commented-out code Note test
The conversations in #4216 are not readable. I opened this clean one for reviewers.
Summary by CodeRabbit
Release Notes
New Features
PairDeepSpinclass for integrating spin models into LAMMPS simulations.DeepSpinandDeepSpinModelDeviclasses with new methods for energy and force calculations.DeepPotTFclass to compute atomic energies and virials.Bug Fixes
Tests
DeepSpinandDeepSpinModelDeviclasses to validate functionality and ensure accuracy in computations.Documentation