Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: Add consistency test for ZBL between dp and pt #4292

Merged
merged 23 commits into from
Nov 1, 2024

Conversation

anyangml
Copy link
Collaborator

@anyangml anyangml commented Oct 31, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced DPZBLModel, enhancing energy modeling capabilities.
    • Added get_zbl_model function for creating DPZBLModel from input data.
    • New DPZBLLinearEnergyAtomicModel class allows for complex interactions between atomic models.
  • Bug Fixes

    • Corrected typographical errors in multiple test classes to improve code clarity and consistency in method names.
    • Updated model type attributes for DPZBLModel and LinearEnergyModel to reflect accurate classifications.
  • Tests

    • Added comprehensive unit tests for energy models to ensure functionality across various backends.
    • Enhanced existing test classes with corrected method names for improved accuracy.

@anyangml anyangml marked this pull request as draft October 31, 2024 07:02
Copy link
Contributor

coderabbitai bot commented Oct 31, 2024

📝 Walkthrough
📝 Walkthrough

Walkthrough

This pull request introduces a new class DPZBLModel in dp_zbl_model.py, which extends DPEnergyModel_ and includes a method for updating selections. Additionally, a new function get_zbl_model is added to model.py, enabling the construction of DPZBLModel based on input data. The get_model function is modified to incorporate this new model type. A new test suite in test_zbl_ener.py is created to validate the functionality of energy models across different backends, ensuring compatibility and correctness.

Changes

File Path Change Summary
deepmd/dpmodel/model/dp_zbl_model.py - Added class DPZBLModel(DPEnergyModel_).
- Added method update_sel(cls, train_data, type_map, local_jdata).
deepmd/dpmodel/model/model.py - Added function get_zbl_model(data: dict) for creating DPZBLModel.
- Updated get_model(data: dict) to call get_zbl_model(data) if "use_srtab" is present.
source/tests/consistent/model/test_zbl_ener.py - Added class TestEner(CommonTest, ModelTest, unittest.TestCase) for unit testing energy models.
source/tests/consistent/common.py - Updated variable name from addtional_data to additional_data in CommonTest.
source/tests/consistent/fitting/test_dipole.py - Updated method name from addtional_data to additional_data in TestDipole.
source/tests/consistent/fitting/test_dos.py - Updated method name from addtional_data to additional_data in TestDOS.
source/tests/consistent/fitting/test_ener.py - Updated method name from addtional_data to additional_data in TestEner.
source/tests/consistent/fitting/test_polar.py - Updated method name from addtional_data to additional_data in TestPolar.
source/tests/consistent/fitting/test_property.py - Updated method name from addtional_data to additional_data in TestProperty.
source/tests/consistent/test_type_embedding.py - Updated method name from addtional_data to additional_data in TestTypeEmbedding.
source/tests/consistent/model/test_ener.py - Fixed typo in pass_data_to_cls method to reference self.additional_data.

Possibly related PRs

  • test(pt): add common test case for model/atomic model #3767: This PR introduces a new method get_ntypes in the make_model.py file, which is relevant as it may interact with the DPZBLModel class introduced in the main PR, particularly in terms of model type handling.
  • refactor: refactor update_sel and save min_nbor_dist #3829: The changes in this PR involve modifications to the update_sel method across various classes, including the DPModelCommon class, which is relevant since the main PR also introduces an update_sel method in the DPZBLModel class, indicating a direct connection in functionality.
  • Feat (pt): Expose Linear Ener Model  #4194: This PR introduces a new JSON configuration for a linear energy model, which may relate to the DPZBLModel as both involve energy modeling within the same framework.
  • feat(jax): energy model (no grad support) #4226: The introduction of the DeepEval class in this PR enhances the evaluation capabilities of models, which could be relevant to the DPZBLModel as it may be evaluated using similar methods.
  • feat(jax): checkpoint I/O #4236: The modifications to the JAXBackend class and the introduction of serialization and deserialization hooks may relate to the DPZBLModel as it could utilize these backend features for model management.
  • feat(jax): freeze to StableXLO & DeepEval #4256: This PR focuses on integrating the DeepEval functionality into the JAX backend, which is relevant as it may enhance the evaluation of models like DPZBLModel introduced in the main PR.

Suggested labels

Docs

Suggested reviewers

  • njzjz
  • wanghan-iapcm
  • iProzd

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (6)
deepmd/dpmodel/model/dp_zbl_model.py (3)

23-23: Add docstring to explain the intermediate class.

Consider adding a brief docstring to explain the purpose of DPEnergyModel_ and its relationship with DPZBLLinearEnergyAtomicModel. This would help other developers understand the class hierarchy and design pattern.

 DPEnergyModel_ = make_model(DPZBLLinearEnergyAtomicModel)
+"""Base energy model class created from DPZBLLinearEnergyAtomicModel.
+
+The trailing underscore indicates this is an intermediate class used as a base
+for the full DPZBLModel implementation.
+"""

26-34: Add class documentation for DPZBLModel.

The class lacks documentation explaining its purpose, usage, and relationship with the ZBL potential. Consider adding a comprehensive docstring that includes:

  • Purpose of the ZBL model
  • Usage examples
  • Parameters description
  • References to the ZBL potential literature
 @BaseModel.register("zbl")
 class DPZBLModel(DPEnergyModel_):
+    """Deep Potential model with ZBL (Ziegler-Biersack-Littmark) potential support.
+
+    This model combines deep learning with the ZBL potential to better describe
+    short-range atomic interactions. The ZBL potential is particularly important
+    for high-energy atomic collisions and radiation damage simulations.
+
+    References
+    ----------
+    .. [1] J.F. Ziegler, J.P. Biersack, U. Littmark, The Stopping and Range of
+           Ions in Matter, Pergamon, New York (1985)
+    """

36-65: Enhance method documentation while implementation LGTM.

The implementation correctly handles data immutability and delegation to DPModelCommon.update_sel. Consider enhancing the docstring with:

  • More detailed parameter descriptions
  • Return value explanations
  • Usage example
     """Update the selection and perform neighbor statistics.

+    This method updates the neighbor list selection parameters based on the
+    training data and performs statistical analysis of atomic neighborhoods.
+
     Parameters
     ----------
     train_data : DeepmdDataSystem
-        data used to do neighbor statistics
+        Training data system containing atomic configurations used for
+        neighbor statistics calculation
     type_map : list[str], optional
-        The name of each type of atoms
+        Mapping between atom types and their string representations.
+        If None, default type mapping will be used
     local_jdata : dict
-        The local data refer to the current class
+        Local configuration dictionary containing model parameters.
+        Must include a "dpmodel" key with model-specific settings
-    dict
-        The updated local data
-    float
-        The minimum distance between two atoms
+    tuple[dict, Optional[float]]
+        - dict: Updated local configuration with new selection parameters
+        - float: Minimum distance between any two atoms in the dataset,
+                useful for ZBL potential cutoff determination
+
+    Example
+    -------
+    >>> train_data = DeepmdDataSystem(...)
+    >>> type_map = ["O", "H"]
+    >>> local_jdata = {"dpmodel": {"sel": [50, 50]}}
+    >>> updated_data, min_dist = DPZBLModel.update_sel(
+    ...     train_data, type_map, local_jdata
+    ... )
     """
deepmd/dpmodel/model/model.py (1)

67-67: Add docstring and type hints.

The function lacks documentation explaining its purpose, parameters, and return type. Consider adding:

-def get_zbl_model(data: dict):
+def get_zbl_model(data: dict) -> DPZBLModel:
+    """Construct a ZBL (Ziegler-Biersack-Littmark) model from input data.
+
+    Parameters
+    ----------
+    data : dict
+        Dictionary containing model configuration with required keys:
+        - descriptor: dict with DescrptSeA parameters
+        - fitting_net: dict with EnergyFittingNet parameters
+        - use_srtab: str, path to the pair potential table
+        - sw_rmin: float, inner cutoff for switching function
+        - sw_rmax: float, outer cutoff for switching function
+        - type_map: list[str], atom type names
+
+    Returns
+    -------
+    DPZBLModel
+        Combined deep potential and ZBL model
+
+    Raises
+    ------
+    ValueError
+        If fitting type is not 'ener' or required keys are missing
+    """
source/tests/consistent/model/test_zbl_ener.py (2)

147-147: Correct the value format from 00.25 to 0.25.

The value 00.25 in line 147 may cause confusion due to the leading zero. It's better to use 0.25 for clarity.

Apply this diff to update the value:

-            00.25,
+            0.25,

169-169: Update the comment to use proper grammar.

The comment in line 169 should use the past tense "sorted" instead of "sort" for clarity.

Apply this diff to correct the grammar:

-        # TF requires the atype to be sort
+        # TF requires the atype to be sorted
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d165fee and b188648.

📒 Files selected for processing (3)
  • deepmd/dpmodel/model/dp_zbl_model.py (1 hunks)
  • deepmd/dpmodel/model/model.py (4 hunks)
  • source/tests/consistent/model/test_zbl_ener.py (1 hunks)
🔇 Additional comments (7)
deepmd/dpmodel/model/dp_zbl_model.py (2)

1-21: LGTM! Well-organized imports.

The imports are properly organized, with clear separation between typing, atomic model, and utility imports. All necessary dependencies for the ZBL model implementation are present.


1-65: Verify numpy backend integration.

Since this PR aims to expose the ZBL numpy backend, let's verify the integration points.

✅ Verification successful

Let me gather more specific information about the ZBL model implementation.


Based on the gathered information, I can now provide the final response:

Backend integration is properly implemented

The ZBL model implementation follows the framework's multi-backend architecture correctly:

  • The model is properly structured with separate implementations for different backends (PyTorch in pt/model/ and common code in dpmodel/)
  • No direct numpy dependencies are found in the ZBL model files, maintaining backend independence
  • The model inherits from appropriate base classes that handle backend-specific operations
  • Extensive test coverage exists for both PyTorch and common implementations
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the numpy backend integration for ZBL model

# Test 1: Check for numpy-specific implementations
echo "Checking for numpy backend implementations..."
rg -l "numpy" --type py

# Test 2: Look for other ZBL-related files
echo "Checking for other ZBL-related files..."
fd -e py zbl

# Test 3: Check for backend registration patterns
echo "Checking for backend registration patterns..."
rg -A 5 "register.*backend" --type py

Length of output: 15044


Script:

#!/bin/bash
# Check ZBL model implementations and their backend integrations

# Look for ZBL model implementations in both numpy and tensorflow
echo "Checking ZBL model implementations..."
rg -A 10 "class.*ZBL" --type py

# Check for numpy-specific imports or operations in ZBL files
echo "Checking numpy usage in ZBL files..."
rg "import.*numpy|np\." deepmd/dpmodel/model/dp_zbl_model.py deepmd/pt/model/model/dp_zbl_model.py

# Check the base model and linear model implementations
echo "Checking base model implementations..."
rg -A 10 "class DPZBLLinearEnergyAtomicModel" --type py

Length of output: 18517

deepmd/dpmodel/model/model.py (3)

2-7: LGTM: Import changes are well-organized and necessary.

The new imports for DPAtomicModel, PairTabAtomicModel, and DPZBLModel are correctly added and properly organized.

Also applies to: 17-19


17-19: Verify ZBL model registration.

Ensure that the DPZBLModel is properly registered with the BaseModel registry.

#!/bin/bash
# Check if DPZBLModel is registered with BaseModel
ast-grep --pattern 'class DPZBLModel $$$: @register_model("zbl") $$$'

150-151: Verify model type precedence.

The order of model type checks could affect which model is created when multiple conditions are true. Please verify that there are no use cases where both "spin" and "use_srtab" could be present in the input data.

✅ Verification successful

Based on the search results, I can now generate the final response:

No conflict between spin and ZBL model types

The model type checks in get_model function are mutually exclusive:

  • The "spin" check is only performed when model_type == "standard"
  • The "use_srtab" check is independent and comes after the model type check
  • No test cases or configurations show both features being used together
  • The implementations (DPZBLModel and SpinModel) are separate model types with distinct purposes
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for test cases or example configurations that might combine spin and ZBL models
echo "Searching for potential model combinations in tests..."
rg -l "use_srtab.*spin|spin.*use_srtab" tests/

echo "Searching for documentation mentioning both features..."
rg -l "zbl.*spin|spin.*zbl" docs/

Length of output: 165


Script:

#!/bin/bash
# Search for model type checks in the get_model function
echo "Checking model type detection logic:"
rg -A 10 "def get_model" deepmd/dpmodel/model/model.py

# Search for ZBL model implementation
echo -e "\nChecking ZBL model implementation:"
rg -A 5 "class.*ZBL" 

# Search for usage patterns of use_srtab
echo -e "\nChecking use_srtab usage:"
rg "use_srtab"

# Search for spin model implementation
echo -e "\nChecking spin model implementation:"
rg -A 5 "class.*Spin"

Length of output: 27400

source/tests/consistent/model/test_zbl_ener.py (2)

98-112: ⚠️ Potential issue

Ensure RefBackend is defined in the class.

The method get_reference_backend references self.RefBackend, but RefBackend is not defined within the class TestEner or its parent classes in the provided code. This will result in an AttributeError at runtime.

Please verify that RefBackend is properly defined or imported. If it's intended to be an enumeration of backend options, consider defining it within the class or importing it from the appropriate module. For example:

from some_module import RefBackend

211-236: ⚠️ Potential issue

Verify the usage of self.RefBackend in extract_ret method.

In the extract_ret method, self.RefBackend is used to determine the backend, but RefBackend is not defined in the provided code context. This will cause a runtime error.

Ensure that RefBackend is defined or imported correctly. If it's an attribute or enumeration, it should be declared within the class or imported from the relevant module.

Would you like assistance in updating the code to define RefBackend?

deepmd/dpmodel/model/model.py Outdated Show resolved Hide resolved
source/tests/consistent/model/test_zbl_ener.py Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
source/tests/consistent/model/test_zbl_ener.py (4)

39-49: Add docstring to explain test parameters.

The test class is parameterized with different exclude types configurations, but their purpose and expected behavior are not documented. Consider adding a docstring to explain what each parameter combination tests.

Example docstring:

@parameterized(
    (
        [],  # pair_exclude_types
        [[0, 1]],  # atom_exclude_types: exclude interactions between types 0 and 1
    ),
    (
        [],  # pair_exclude_types
        [1],  # atom_exclude_types: exclude all interactions with type 1
    ),
)

83-86: Add type annotations and improve imports.

The model classes are undefined according to static analysis. Consider adding type annotations and importing these types conditionally:

from typing import Optional, Type

# At the top with other imports
if INSTALLED_TF:
    from deepmd.tf.model import EnergyModelTF
else:
    EnergyModelTF = None
# Similar for other models

class TestEner(CommonTest, ModelTest, unittest.TestCase):
    tf_class: Optional[Type] = EnergyModelTF
    dp_class: Optional[Type] = EnergyModelDP
    pt_class: Optional[Type] = EnergyModelPT
🧰 Tools
🪛 Ruff

83-83: Undefined name EnergyModelTF

(F821)


84-84: Undefined name EnergyModelDP

(F821)


85-85: Undefined name EnergyModelPT

(F821)


110-113: Document why JAX backend is skipped.

The skip_jax property always returns True, but it's not clear why JAX backend is permanently disabled. Consider adding a comment explaining the reason or making it configurable if it's temporary.

@property
def skip_jax(self) -> bool:
    # TODO: JAX backend is not yet supported for ZBL energy models
    return True

123-161: Improve test data documentation.

The test data setup could benefit from better documentation:

  1. Add comments explaining the physical meaning of the coordinate data
  2. Document why TF requires sorted atom types
  3. Consider extracting the test data to a fixture or constant

Example improvement:

def setUp(self):
    CommonTest.setUp(self)
    
    # System configuration:
    # - 6 atoms (2 O atoms and 4 H atoms) in a water-like arrangement
    # - Cubic box with 13.0 Å sides
    self.ntypes = 2  # O and H atoms
    ...
    
    # TF backend requires sorted atom types for efficient computation
    # Reorder coordinates to match sorted atom types
    idx_map = np.argsort(self.atype.ravel())
    self.atype = self.atype[:, idx_map]
    self.coords = self.coords[:, idx_map]
source/tests/consistent/fitting/test_dos.py (1)

127-127: LGTM! Consider adding a docstring.

The spelling correction from addtional_data to additional_data improves code quality. Consider adding a docstring to document the purpose and return value of this property method.

    @property
    def additional_data(self) -> dict:
+       """Returns configuration data for the test environment.
+       
+       Returns:
+           dict: A dictionary containing:
+               - ntypes: Number of atom types
+               - dim_descrpt: Dimension of the input descriptor
+               - mixed_types: Boolean flag for mixed types
+       """
        (
            resnet_dt,
            precision,
            mixed_types,
            numb_fparam,
            numb_aparam,
            numb_dos,
        ) = self.param
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b188648 and 4e3ee57.

📒 Files selected for processing (9)
  • source/tests/consistent/common.py (2 hunks)
  • source/tests/consistent/fitting/test_dipole.py (1 hunks)
  • source/tests/consistent/fitting/test_dos.py (1 hunks)
  • source/tests/consistent/fitting/test_ener.py (1 hunks)
  • source/tests/consistent/fitting/test_polar.py (1 hunks)
  • source/tests/consistent/fitting/test_property.py (1 hunks)
  • source/tests/consistent/model/test_ener.py (1 hunks)
  • source/tests/consistent/model/test_zbl_ener.py (1 hunks)
  • source/tests/consistent/test_type_embedding.py (1 hunks)
🧰 Additional context used
🪛 Ruff
source/tests/consistent/model/test_zbl_ener.py

83-83: Undefined name EnergyModelTF

(F821)


84-84: Undefined name EnergyModelDP

(F821)


85-85: Undefined name EnergyModelPT

(F821)


117-117: Undefined name EnergyModelDP

(F821)


119-119: Undefined name EnergyModelPT

(F821)

🪛 GitHub Check: CodeQL
source/tests/consistent/model/test_zbl_ener.py

[notice] 29-29: Unused global variable
The global variable 'DPZBLModelPT' is not used.


[notice] 33-33: Unused global variable
The global variable 'DPZBLModelTF' is not used.

🔇 Additional comments (9)
source/tests/consistent/fitting/test_dipole.py (1)

86-86: LGTM! Consistent typo fix across test files.

The renaming from addtional_data to additional_data improves code clarity while maintaining functionality.

Let's verify the consistent renaming across other mentioned test files:

✅ Verification successful

✓ Typo fix successfully verified across all test files

The property has been correctly renamed from addtional_data to additional_data in all relevant test files:

  • source/tests/consistent/fitting/test_property.py
  • source/tests/consistent/test_type_embedding.py
  • source/tests/consistent/fitting/test_polar.py
  • source/tests/consistent/fitting/test_ener.py
  • source/tests/consistent/fitting/test_dos.py

No instances of the old spelling addtional_data remain in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify the property renaming across test files
# Expect: Only occurrences of the correct spelling 'additional_data'

echo "Checking for any remaining instances of the old spelling..."
rg "addtional_data" "source/tests/consistent/fitting/"

echo "Verifying the new spelling exists in mentioned files..."
rg "additional_data" \
    "source/tests/consistent/fitting/test_ener.py" \
    "source/tests/consistent/fitting/test_dos.py" \
    "source/tests/consistent/fitting/test_polar.py" \
    "source/tests/consistent/fitting/test_property.py" \
    "source/tests/consistent/test_type_embedding.py"

Length of output: 980

source/tests/consistent/fitting/test_polar.py (1)

86-86: LGTM! Typo fix in property name.

The rename from addtional_data to additional_data corrects a typo while maintaining the same functionality.

Let's verify that this property name has been consistently updated across the codebase:

✅ Verification successful

Property name has been consistently updated across the codebase

The verification shows that:

  1. No instances of the misspelled addtional_data remain in the codebase
  2. The correctly spelled additional_data is used consistently across test files and implementation code
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify no instances of the misspelled property remain
# and all references use the correct spelling

# Check for any remaining instances of the misspelled property
echo "Checking for remaining misspellings..."
rg "addtional_data"

# Check usage of the correct property name
echo "Verifying correct property usage..."
rg "additional_data"

Length of output: 1768

source/tests/consistent/test_type_embedding.py (2)

85-85: LGTM! Good catch on fixing the typo.

The spelling correction from addtional_data to additional_data improves code readability.


85-85: Verify consistent method renaming across the codebase.

Let's ensure all references to this method have been updated consistently.

✅ Verification successful

Let me check the actual usage context of additional_data to ensure it's being used consistently.


Method naming is consistent across the codebase

The search results show that there are no instances of the misspelled addtional_data in the codebase, and all occurrences of additional_data are being used correctly in their respective contexts:

  • As a property method in test classes
  • As a class variable in common.py
  • As utility functions for data requirements in the main codebase
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old method name
# and verify that all occurrences use the correct spelling

echo "Checking for any remaining instances of 'addtional_data'..."
rg "addtional_data" -l

echo "Verifying usage of corrected 'additional_data'..."
rg "additional_data" -l

Length of output: 756


Script:

#!/bin/bash
# Check the context of additional_data usage
rg "additional_data" -B 2 -A 2

Length of output: 5502

source/tests/consistent/fitting/test_property.py (1)

107-107: LGTM! Method name spelling correction.

The rename from addtional_data to additional_data fixes a typo and improves code quality.

Let's verify that all occurrences of this method have been renamed consistently:

✅ Verification successful

Method rename has been applied consistently across all test files

The verification confirms:

  • No instances of the misspelled addtional_data remain in the codebase
  • The correctly spelled additional_data method is consistently used across all fitting test files:
    • test_polar.py
    • test_ener.py
    • test_property.py
    • test_dipole.py
    • test_dos.py
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify no instances of the misspelled method name remain
# and all references use the correct spelling

# Test 1: Check for any remaining instances of the misspelled method
echo "Checking for remaining misspellings..."
rg "addtional_data"

# Test 2: Verify the correct spelling is used consistently
echo "Verifying correct usage..."
rg "additional_data" "source/tests/consistent/fitting/"

Length of output: 649

source/tests/consistent/model/test_ener.py (1)

133-133: Spelling correction improves code consistency.

The fix correctly updates the attribute name from "addtional_data" to "additional_data", ensuring consistency with other test classes and preventing potential attribute access errors.

Let's verify this is part of a broader consistency fix:

✅ Verification successful

Spelling consistency verified across test files

The verification confirms:

  • No instances of the misspelled "addtional_data" remain in the codebase
  • The correct spelling "additional_data" is consistently used across multiple test files:
    • As a property in test classes (test_type_embedding.py, test_ener.py, test_polar.py, etc.)
    • In class instantiation (test_zbl_ener.py, test_ener.py)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of the misspelling
rg "addtional_data" 

# Search for the correct spelling to confirm consistent usage
rg "additional_data" -g "test_*.py"

Length of output: 759

source/tests/consistent/fitting/test_ener.py (1)

137-137: LGTM! Method name typo fixed.

The correction of the method name from addtional_data to additional_data improves code quality by fixing a typographical error.

Let's verify that all references to this method have been updated:

✅ Verification successful

Method name correction is complete and consistent

The verification shows that:

  1. No instances of the old misspelled addtional_data remain in the codebase
  2. The corrected additional_data method is consistently used across multiple test files and the core implementation
  3. All references to this method name are properly aligned with the new spelling
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old method name
# and verify that all references use the new name

# Search for any remaining instances of the old method name
echo "Checking for any remaining instances of 'addtional_data'..."
rg "addtional_data"

# Search for references to the new method name to ensure consistency
echo "Verifying references to 'additional_data'..."
rg "additional_data" -A 2

Length of output: 3919

source/tests/consistent/common.py (2)

78-78: LGTM! Fixed typo in variable name.

The change correctly fixes the spelling of the class variable from addtional_data to additional_data.


131-131: LGTM! Updated variable reference.

The change consistently updates the variable reference to use the corrected name additional_data.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (5)
source/tests/consistent/model/test_zbl_ener.py (5)

25-33: Consider documenting the purpose of unused model classes.

The DPZBLModelPT and DPZBLModelTF classes are imported but not used in the code. If these are intended for future use or type hints, please add a comment explaining their purpose.

🧰 Tools
🪛 GitHub Check: CodeQL

[notice] 29-29: Unused global variable
The global variable 'DPZBLModelPT' is not used.


[notice] 33-33: Unused global variable
The global variable 'DPZBLModelTF' is not used.


39-49: Add descriptive names for parameterized test cases.

The test cases could be more self-documenting. Consider using the name parameter in @parameterized to describe what each test case verifies:

 @parameterized(
-    (
-        [],
-        [[0, 1]],
-    ),
-    (
-        [],
-        [1],
-    ),
+    name="test_with_pair_exclude",
+    ([], [[0, 1]]),
+    name="test_with_single_atom_exclude",
+    ([], [1]),
 )

110-113: Document why JAX backend is always skipped.

The skip_jax property always returns True without explanation. Please add a comment explaining why JAX backend tests are skipped or implement the actual skip condition if this is a placeholder.


127-149: Improve readability of test coordinates.

Consider making the coordinate data more readable by using a more structured format or adding comments to explain what these coordinates represent:

-        self.coords = np.array(
-            [
-                12.83, 2.56, 2.18,
-                12.09, 2.87, 2.74,
-                00.25, 3.32, 1.68,
-                3.36, 3.00, 1.81,
-                3.51, 2.51, 2.60,
-                4.27, 3.22, 1.56,
-            ],
+        # Define coordinates for a water molecule system
+        self.coords = np.array([
+            # Oxygen 1
+            [12.83, 2.56, 2.18],
+            # Hydrogen 1
+            [12.09, 2.87, 2.74],
+            # Hydrogen 2
+            [00.25, 3.32, 1.68],
+            # ... add comments for remaining atoms
+        ],

199-224: Document the purpose of SKIP_FLAG in DP backend returns.

In the extract_ret method, the DP backend returns SKIP_FLAG for force and virial. Please add a comment explaining why these values are skipped for the DP backend.

         if backend is self.RefBackend.DP:
+            # DP backend doesn't compute forces and virials in this context
             return (
                 ret["energy_redu"].ravel(),
                 ret["energy"].ravel(),
                 SKIP_FLAG,  # force
                 SKIP_FLAG,  # virial
             )
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between b188648 and 4e3ee57.

📒 Files selected for processing (9)
  • source/tests/consistent/common.py (2 hunks)
  • source/tests/consistent/fitting/test_dipole.py (1 hunks)
  • source/tests/consistent/fitting/test_dos.py (1 hunks)
  • source/tests/consistent/fitting/test_ener.py (1 hunks)
  • source/tests/consistent/fitting/test_polar.py (1 hunks)
  • source/tests/consistent/fitting/test_property.py (1 hunks)
  • source/tests/consistent/model/test_ener.py (1 hunks)
  • source/tests/consistent/model/test_zbl_ener.py (1 hunks)
  • source/tests/consistent/test_type_embedding.py (1 hunks)
🧰 Additional context used
🪛 Ruff
source/tests/consistent/model/test_zbl_ener.py

83-83: Undefined name EnergyModelTF

(F821)


84-84: Undefined name EnergyModelDP

(F821)


85-85: Undefined name EnergyModelPT

(F821)


117-117: Undefined name EnergyModelDP

(F821)


119-119: Undefined name EnergyModelPT

(F821)

🪛 GitHub Check: CodeQL
source/tests/consistent/model/test_zbl_ener.py

[notice] 29-29: Unused global variable
The global variable 'DPZBLModelPT' is not used.


[notice] 33-33: Unused global variable
The global variable 'DPZBLModelTF' is not used.

🔇 Additional comments (10)
source/tests/consistent/fitting/test_dipole.py (1)

86-86: LGTM! Property name typo fix.

The rename from addtional_data to additional_data improves code quality by fixing the typo.

Let's verify that all references to this property have been updated across the codebase:

✅ Verification successful

Property rename is complete and consistent

The verification shows that:

  1. No instances of the misspelled addtional_data exist in the codebase
  2. The correctly spelled additional_data is used consistently across test files and implementation code
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old property name
# Expect: No occurrences of "addtional_data" should be found

# Search for any remaining instances of the misspelled property
rg "addtional_data"

# Search for the correct property name to verify the changes
rg "additional_data"

Length of output: 1600

source/tests/consistent/fitting/test_polar.py (1)

86-86: LGTM! Verify all references to this property.

The typo fix from addtional_data to additional_data is correct.

Let's verify that all references to this property have been updated:

✅ Verification successful

Property name change is safe and consistent

The verification shows that:

  1. No instances of the old misspelled addtional_data exist in the codebase
  2. The new additional_data property is consistently used across test files and properly integrated with the codebase
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old property name
# Test: Search for any remaining instances of 'addtional_data'. Expect: No matches
rg "addtional_data" -g '!*.git*'

# Test: Verify the new property name is used consistently
rg "additional_data" -g '!*.git*'

Length of output: 1626

source/tests/consistent/test_type_embedding.py (1)

85-85: LGTM! Method name spelling correction.

The rename from addtional_data to additional_data fixes a typo while maintaining the method's functionality. This change is part of a broader consistency improvement across multiple test files.

Let's verify that all references to this method have been updated:

✅ Verification successful

Method rename verification successful - no remaining instances of the typo

The search results confirm that:

  1. No instances of the misspelled addtional_data remain in the codebase
  2. The correctly spelled additional_data is consistently used across multiple test files and the core codebase
  3. All references to this method have been properly updated
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the old method name
# Expect: No occurrences of 'addtional_data' should be found

rg "addtional_data" --type python

Length of output: 64


Script:

#!/bin/bash
# Let's try without file type restriction since rg doesn't recognize 'python'
# First, search for the old method name
echo "Searching for old method name 'addtional_data':"
rg "addtional_data"

# Then search for the new method name for context
echo -e "\nSearching for new method name 'additional_data':"
rg "additional_data" -A 2

Length of output: 3909

source/tests/consistent/fitting/test_property.py (2)

107-107: LGTM! Good catch on the typo fix.

The spelling correction from addtional_data to additional_data improves code readability.


107-107: Verify all references to this method are updated.

Let's ensure all references to this method have been updated across the codebase.

✅ Verification successful

Let me check the actual usage of additional_data in these files to ensure they are properly updated.


Method references are properly updated across the codebase

The verification shows that:

  1. No instances of the misspelled addtional_data exist in the codebase
  2. All occurrences of additional_data are properly defined and used:
    • As a property method in test classes (test_property.py, test_polar.py, etc.)
    • As a helper function in training modules
    • As a class variable in common.py
  3. The method is consistently used with the correct spelling across all references
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for any remaining instances of the old method name
# Expect: No occurrences of 'addtional_data' should be found

echo "Searching for any remaining instances of the misspelled method..."
rg "addtional_data" -l

echo "Verifying the corrected method name usage..."
rg "additional_data" -l

# Search for any test classes that might have inherited from TestProperty
echo "Checking for potential inherited classes that might need updates..."
ast-grep --pattern 'class $_(TestProperty)'

Length of output: 937


Script:

#!/bin/bash
# Show the context around additional_data usage in each file
rg "additional_data" -B 2 -A 2

# Also check for any class inheritance patterns specifically
ast-grep --pattern 'class $_ extends TestProperty { $$$ }'
ast-grep --pattern 'class $_($$$, TestProperty$$$) { $$$ }'

Length of output: 5625

source/tests/consistent/model/test_ener.py (1)

133-133: LGTM! Typo fix in attribute name.

The correction from self.addtional_data to self.additional_data fixes a potential AttributeError in the test infrastructure.

Let's verify if similar typos exist elsewhere in the codebase:

✅ Verification successful

No other instances of the typo found in the codebase

The search results show that:

  • The correct spelling additional_data is consistently used across the codebase
  • No instances of the misspelled addtional were found
  • The attribute is properly defined in the parent class (source/tests/consistent/common.py) and used correctly in various test files
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining instances of the typo 'addtional'
rg -i 'addtional' --type py

Length of output: 27


Script:

#!/bin/bash
# Let's try a more comprehensive search for variations of the word
rg -i "ad+t+i*o*nal[_-]?data" --type py

# Also search for the correct spelling to understand the pattern usage
rg -i "additional[_-]?data" --type py

Length of output: 1641

source/tests/consistent/fitting/test_dos.py (1)

127-127: LGTM! Typo fix in property method name.

The rename from addtional_data to additional_data improves code quality by fixing the spelling.

Let's verify that this typo has been fixed consistently across the codebase:

✅ Verification successful

Typo fix verified and consistent across codebase

The search results confirm:

  • No instances of the misspelled addtional_data remain in the codebase
  • The correctly spelled additional_data is used consistently across test files and the main codebase
  • The property method appears to be part of a common test pattern used in multiple test files under source/tests/consistent/fitting/
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the misspelled method name
# and verify the correction is applied consistently

# Test 1: Search for any remaining instances of the misspelled 'addtional_data'
echo "Checking for any remaining misspellings..."
rg "addtional_data"

# Test 2: Verify the corrected spelling exists in other test files mentioned in the summary
echo "Verifying consistent renaming across test files..."
rg "additional_data" "source/tests/consistent/fitting/test_*.py"

Length of output: 372


Script:

#!/bin/bash
# Let's try a more robust search approach

# First, find all test files in the repository
echo "Finding test files..."
fd "test_.*\.py$"

# Then search for any remaining misspellings
echo "Checking for misspellings..."
rg "addtional_data" -t py

# Search for correct spelling
echo "Checking correct spelling usage..."
rg "additional_data" -t py

Length of output: 12000

source/tests/consistent/fitting/test_ener.py (1)

137-137: LGTM! Method name typo fix improves consistency.

The renaming from addtional_data to additional_data is a good fix that improves code quality and maintainability.

Let's verify that this fix has been consistently applied across all related test files:

✅ Verification successful

Method name typo fix has been consistently applied across test files

The verification results show:

  • No remaining instances of the typo addtional_data were found in the test files
  • The correct method name additional_data is consistently used in all related test files:
    • test_dipole.py
    • test_dos.py
    • test_polar.py
    • test_property.py

Note: test_type_embedding.py was not found, but this doesn't affect the verification as the typo fix is consistent across all existing test files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for any remaining instances of the typo and verify consistent naming
# across test files mentioned in the AI summary

# Test 1: Search for any remaining instances of the typo
echo "Checking for any remaining instances of 'addtional_data'..."
rg "addtional_data" "source/tests"

# Test 2: Verify the correct naming in related test files
echo "Verifying correct naming in related test files..."
rg "additional_data" "source/tests/consistent/fitting/test_dipole.py" "source/tests/consistent/fitting/test_dos.py" "source/tests/consistent/fitting/test_polar.py" "source/tests/consistent/fitting/test_property.py" "source/tests/consistent/fitting/test_type_embedding.py"

Length of output: 975

source/tests/consistent/common.py (2)

78-78: LGTM! Fixed typo in variable name.

The variable name has been corrected from addtional_data to additional_data.


131-131: LGTM! Consistent usage of renamed variable.

The usage of additional_data in the pass_data_to_cls method is consistent with the variable rename.

source/tests/consistent/model/test_zbl_ener.py Outdated Show resolved Hide resolved
@anyangml anyangml changed the title feat: expose zbl numpy backend Feat: Add consistency test for ZBL between dp and pt Oct 31, 2024
@anyangml
Copy link
Collaborator Author

This PR should partially resolve #4279

@anyangml anyangml linked an issue Oct 31, 2024 that may be closed by this pull request
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
deepmd/dpmodel/model/model.py (2)

70-71: Add type hints and docstring.

The function lacks type hints and documentation. Consider adding:

-def get_zbl_model(data: dict):
+def get_zbl_model(data: dict) -> DPZBLModel:
+    """Construct a ZBL (Ziegler-Biersack-Littmark) hybrid model.
+
+    Parameters
+    ----------
+    data : dict
+        Dictionary containing model configuration with keys:
+        - descriptor: Descriptor configuration
+        - fitting_net: Fitting network configuration
+        - use_srtab: Path to pair potential table
+        - sw_rmin: Minimum switching radius
+        - sw_rmax: Maximum switching radius
+        - type_map: Atom type mapping
+        - atom_exclude_types: (optional) Atom types to exclude
+        - pair_exclude_types: (optional) Pair types to exclude
+
+    Returns
+    -------
+    DPZBLModel
+        Constructed hybrid model combining deep potential and ZBL pair potential.
+    """

153-154: Consider improving model type determination.

The use of "use_srtab" as a key to determine the model type is not immediately clear. Consider:

  1. Adding a comment explaining the precedence of different model types
  2. Using a more explicit key like "model_type": "zbl" for clearer intent
     if model_type == "standard":
         if "spin" in data:
+            # Spin models take precedence over ZBL models
             return get_spin_model(data)
         elif "use_srtab" in data:
+            # ZBL hybrid models are determined by the presence of pair potential table
             return get_zbl_model(data)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between a95d75e and 4247882.

📒 Files selected for processing (1)
  • deepmd/dpmodel/model/model.py (4 hunks)
🔇 Additional comments (2)
deepmd/dpmodel/model/model.py (2)

2-7: LGTM: Import statements are properly organized.

The new imports for ZBL-related models are correctly organized and follow the project's import style.

Also applies to: 20-22


71-71: 🛠️ Refactor suggestion

Consider using DescrptSeA for consistency.

The function uses BaseDescriptor while get_standard_model uses DescrptSeA. This inconsistency might lead to issues.

Consider aligning with get_standard_model:

-    descriptor = BaseDescriptor(**data["descriptor"])
+    descriptor_type = data["descriptor"].pop("type")
+    if descriptor_type == "se_e2_a":
+        descriptor = DescrptSeA(**data["descriptor"])
+    else:
+        raise ValueError(f"Unknown descriptor type {descriptor_type}")

deepmd/dpmodel/model/model.py Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
deepmd/dpmodel/model/model.py (2)

70-70: Add return type hint and docstring.

The function is missing a return type hint and documentation. Add a docstring explaining the purpose, parameters, and return value.

-def get_zbl_model(data: dict):
+def get_zbl_model(data: dict) -> DPZBLModel:
+    """Create a hybrid ZBL model combining deep potential and pair potential.
+
+    Parameters
+    ----------
+    data : dict
+        Dictionary containing model configuration with keys:
+        - descriptor: dict, descriptor settings
+        - fitting_net: dict, fitting network settings
+        - use_srtab: str, path to pair potential table
+        - sw_rmin: float, minimum switching radius
+        - sw_rmax: float, maximum switching radius
+        - type_map: list[str], atom type mapping
+
+    Returns
+    -------
+    DPZBLModel
+        Hybrid model instance combining deep potential and pair potential
+    """

Line range hint 144-156: Document model type selection precedence.

The order of model type checks (spin -> use_srtab -> standard) implies a precedence hierarchy. Consider documenting this precedence in the function's docstring to make it explicit for users.

 def get_model(data: dict):
     """Get a model from a dictionary.
 
     Parameters
     ----------
     data : dict
         The data to construct the model.
+
+    Notes
+    -----
+    Model type selection follows this precedence:
+    1. If "type" is specified, use the registered model type
+    2. If "spin" is present, create a spin model
+    3. If "use_srtab" is present, create a ZBL model
+    4. Otherwise, create a standard model
     """
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 4247882 and 6a1ff90.

📒 Files selected for processing (1)
  • deepmd/dpmodel/model/model.py (4 hunks)
🔇 Additional comments (2)
deepmd/dpmodel/model/model.py (2)

Line range hint 2-22: LGTM: Import changes are well-organized and necessary.

The new imports are properly organized and support the ZBL model functionality.


154-155: LGTM: Clean integration of ZBL model support.

The modification follows the existing pattern and cleanly integrates the ZBL model support.

deepmd/dpmodel/model/model.py Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
deepmd/dpmodel/atomic_model/linear_atomic_model.py (2)

Line range hint 328-336: Minor grammatical correction in class docstring

Consider rephrasing the class docstring for clarity.

Apply this diff to improve the docstring:

-    """Model linearly combine a list of AtomicModels.
+    """Model that linearly combines a list of AtomicModels.

Line range hint 374-387: Ensure consistent default version in deserialize method

In the deserialize method, the default version is set to 1 (data.pop("@version", 1)), but in the serialize method, "@version": 2 is used. This mismatch could cause version compatibility issues if the @version key is missing in the data. It would be better to set the default version to 2 to match the serialized version.

Apply this diff to fix the default version:

-            check_version_compatibility(data.pop("@version", 1), 2, 2)
+            check_version_compatibility(data.pop("@version", 2), 2, 2)
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6a1ff90 and 435a09e.

📒 Files selected for processing (2)
  • deepmd/dpmodel/atomic_model/linear_atomic_model.py (1 hunks)
  • deepmd/dpmodel/model/dp_zbl_model.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • deepmd/dpmodel/model/dp_zbl_model.py
🔇 Additional comments (1)
deepmd/dpmodel/atomic_model/linear_atomic_model.py (1)

Line range hint 327-460: Implementation of DPZBLLinearEnergyAtomicModel is correct

The new class DPZBLLinearEnergyAtomicModel appropriately extends LinearEnergyAtomicModel and correctly overrides methods to incorporate ZBL interpolation. The methods __init__, serialize, deserialize, and _compute_weight are well-implemented.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (1)
deepmd/pt/model/model/dp_zbl_model.py (1)

Line range hint 146-147: Add None check for dforce in forward method.

The forward_lower method includes an assertion to check if dforce is not None, but this check is missing in the forward method. Consider adding the same assertion in the forward method for consistency.

Add this check in the forward method:

         if do_atomic_virial:
             model_predict["atom_virial"] = model_ret["energy_derv_c"].squeeze(-3)
     else:
+        assert model_ret["dforce"] is not None
         model_predict["force"] = model_ret["dforce"]
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 47d0184 and 6658688.

📒 Files selected for processing (2)
  • deepmd/dpmodel/model/dp_zbl_model.py (1 hunks)
  • deepmd/pt/model/model/dp_zbl_model.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • deepmd/dpmodel/model/dp_zbl_model.py
🔇 Additional comments (4)
deepmd/pt/model/model/dp_zbl_model.py (4)

33-33: LGTM: Model type correctly aligned with registration.

The model_type change from "ener" to "zbl" properly aligns with the @BaseModel.register("zbl") decorator, ensuring consistent model identification.


Line range hint 149-182: LGTM: Well-implemented and documented selection update method.

The update_sel method is properly implemented with:

  • Clear type hints
  • Comprehensive docstring
  • Proper delegation to DPModelCommon

Line range hint 1-182: Verify corresponding test implementation.

Since this PR aims to add consistency tests between dp and pt backends for ZBL, we should verify the test implementation.

Let's check for the test file:

#!/bin/bash
# Look for the test file mentioned in AI summary
fd "test_zbl_ener.py" 

# Check test content for consistency testing between backends
rg -l "zbl.*test" --type py | xargs rg -B 5 -A 5 "dp.*pt"

Line range hint 89-102: Verify force handling consistency with dp backend.

The force assignment when do_grad_c is False needs verification to ensure it matches the dp backend's behavior. This is particularly important since there's no explicit None check for model_ret["dforce"].

Let's verify the corresponding implementation in the dp backend:

✅ Verification successful

Let me analyze the force handling in the dp backend to verify the consistency. I'll generate scripts to search for relevant code.

Let me also check for dforce handling:

And let me check the overall force handling pattern:

This will help verify the force handling consistency between the dp backend and the PyTorch implementation.


Based on the search results, I can now verify the force handling consistency. The code shows that the force handling in dp_zbl_model.py follows the same pattern as other model implementations:

  1. When do_grad_c is True:
model_predict["force"] = model_ret["energy_derv_r"].squeeze(-2)
  1. When do_grad_c is False:
model_predict["force"] = model_ret["dforce"]

This is consistent with the implementations in ener_model.py and dp_linear_model.py, showing that the force handling is standardized across the codebase. The code assigns either the energy derivative or dforce to model_predict["force"] based on the do_grad_c flag, which is the expected behavior.

Force handling is consistent with dp backend implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for similar force handling in dp backend
rg -l "dforce" --type py | xargs rg -B 5 -A 5 "model\[\"force\"\]\s*=.*dforce"

Length of output: 270732


Script:

#!/bin/bash
# Search for force handling in dp backend
ast-grep --pattern 'model_predict["force"] = $$$' 

Length of output: 943


Script:

#!/bin/bash
# Search for dforce handling in dp backend
ast-grep --pattern 'model_predict["dforce"] = $$$'

Length of output: 357


Script:

#!/bin/bash
# Search for force-related assignments in dp backend
rg -B 3 -A 3 'model\["force"\]|model\["dforce"\]' --type py

Length of output: 61

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (4)
deepmd/dpmodel/model/model.py (2)

70-70: Add docstring to document the function's purpose and parameters.

The function lacks documentation explaining its purpose, parameters, and return value.

Add a docstring following the project's style:

 def get_zbl_model(data: dict) -> DPZBLModel:
+    """Construct a DPZBLModel combining deep potential and ZBL pair potential.
+
+    Parameters
+    ----------
+    data : dict
+        Dictionary containing model configuration with keys:
+        - descriptor: dict, descriptor settings
+        - fitting_net: dict, fitting network settings
+        - use_srtab: str, path to the pair potential table
+        - sw_rmin: float, inner cutoff for switching function
+        - sw_rmax: float, outer cutoff for switching function
+        - type_map: list, atom type mapping
+
+    Returns
+    -------
+    DPZBLModel
+        Initialized model combining deep potential with ZBL pair potential.
+    """

71-71: Document the ntypes override.

The override of descriptor's ntypes needs explanation.

Add a comment explaining why this override is necessary:

+    # Override ntypes from type_map length to ensure consistency
     data["descriptor"]["ntypes"] = len(data["type_map"])
deepmd/dpmodel/atomic_model/linear_atomic_model.py (2)

Line range hint 344-366: Consider adding parameter validation.

While the constructor properly initializes the ZBL parameters, it should validate the input parameters to ensure they are within reasonable bounds.

Add parameter validation:

 def __init__(
     self,
     dp_model: DPAtomicModel,
     zbl_model: PairTabAtomicModel,
     sw_rmin: float,
     sw_rmax: float,
     type_map: list[str],
     smin_alpha: Optional[float] = 0.1,
     **kwargs,
 ):
+    if sw_rmin <= 0:
+        raise ValueError("sw_rmin must be positive")
+    if smin_alpha is not None and smin_alpha <= 0:
+        raise ValueError("smin_alpha must be positive")
+    if not isinstance(dp_model, DPAtomicModel):
+        raise TypeError("dp_model must be an instance of DPAtomicModel")
+    if not isinstance(zbl_model, PairTabAtomicModel):
+        raise TypeError("zbl_model must be an instance of PairTabAtomicModel")
     models = [dp_model, zbl_model]
     kwargs["models"] = models
     kwargs["type_map"] = type_map
     super().__init__(**kwargs)

Line range hint 328-478: Ensure comprehensive test coverage for ZBL interpolation.

While the implementation is solid, the complexity of the ZBL interpolation requires thorough testing:

  1. Edge cases for distance values
  2. Numerical stability with various parameter combinations
  3. Consistency between DP and ZBL regions
  4. Smooth transition in the interpolation region

Consider adding the following test scenarios:

  1. Extreme distance values
  2. Various combinations of sw_rmin and sw_rmax
  3. Different smin_alpha values
  4. Consistency checks between DP and ZBL potentials
  5. Smoothness of the transition region
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 6658688 and 0e4208d.

📒 Files selected for processing (6)
  • deepmd/dpmodel/atomic_model/linear_atomic_model.py (2 hunks)
  • deepmd/dpmodel/model/dp_zbl_model.py (1 hunks)
  • deepmd/dpmodel/model/model.py (4 hunks)
  • deepmd/pt/model/model/dp_linear_model.py (1 hunks)
  • source/tests/consistent/fitting/test_dipole.py (1 hunks)
  • source/tests/consistent/fitting/test_polar.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • deepmd/dpmodel/model/dp_zbl_model.py
  • source/tests/consistent/fitting/test_dipole.py
  • source/tests/consistent/fitting/test_polar.py
🔇 Additional comments (7)
deepmd/dpmodel/model/model.py (2)

Line range hint 2-22: LGTM: Import changes are well-organized and complete.

The new imports are properly organized and include all necessary components for the ZBL model implementation.


154-155: LGTM: Clean integration of ZBL model support.

The modification follows the existing pattern and cleanly integrates the ZBL model support.

deepmd/pt/model/model/dp_linear_model.py (1)

33-33: Model type update aligns with framework-wide model classification changes.

The change from "ener" to "linear_ener" provides better specificity in model classification, consistent with similar changes in other model classes (e.g., DPZBLModel's type changing from "ener" to "zbl").

Let's verify the consistency of model type usage across the codebase:

✅ Verification successful

Model type change is consistent with framework-wide model registration

The verification confirms that:

  • The model type "linear_ener" is properly registered via @BaseModel.register("linear_ener") in dp_linear_model.py
  • The model type is consistently used across the framework in:
    • Documentation (doc/model/linear.md, doc/model/pairtab.md)
    • Example configurations (examples/water/linear/input.json, examples/water/d3/input.json)
    • Test files (source/tests/tf/test_train.py)
    • Model registration and initialization code (deepmd/pt/model/model/__init__.py)
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify model type registration and usage
# Test 1: Check model type registration
rg -A 2 '@BaseModel\.register' 

# Test 2: Check model type string references
rg '"linear_ener"|"zbl"'

Length of output: 3975

deepmd/dpmodel/atomic_model/linear_atomic_model.py (4)

37-37: LGTM: Model registration is properly implemented.

The decorator correctly registers the LinearEnergyAtomicModel with the "linear" type identifier.


328-329: LGTM: Class registration and inheritance are correct.

The class is properly registered with the "zbl" type identifier and extends LinearEnergyAtomicModel.


Line range hint 368-397: LGTM: Serialization methods are well-implemented.

The serialization and deserialization methods properly handle version compatibility and all necessary parameters.


Line range hint 399-478: Review the numerical stability of weight computation.

The weight computation implementation has several areas that could benefit from improved numerical stability:

  1. The softmin calculation could overflow/underflow for extreme distance values
  2. Division by zero is handled with numpy's error state, but could be more explicit
  3. The smooth switching function could be simplified

Let's verify the numerical stability:

Consider these improvements:

 def _compute_weight(
     self,
     extended_coord: np.ndarray,
     extended_atype: np.ndarray,
     nlists_: list[np.ndarray],
 ) -> list[np.ndarray]:
     # ... existing code ...
     
     # Prevent overflow in exp calculation
+    scaled_rr = np.clip(pairwise_rr / self.smin_alpha, -500, 500)
     numerator = np.sum(
         np.where(
             nlist_larger != -1,
-            pairwise_rr * np.exp(-pairwise_rr / self.smin_alpha),
+            pairwise_rr * np.exp(-scaled_rr),
             np.zeros_like(nlist_larger),
         ),
         axis=-1,
     )
     denominator = np.sum(
         np.where(
             nlist_larger != -1,
-            np.exp(-pairwise_rr / self.smin_alpha),
+            np.exp(-scaled_rr),
             np.zeros_like(nlist_larger),
         ),
         axis=-1,
     )
     
-    with np.errstate(divide="ignore", invalid="ignore"):
-        sigma = numerator / denominator
+    # More explicit division handling
+    sigma = np.zeros_like(numerator)
+    valid_mask = denominator > 1e-10
+    sigma[valid_mask] = numerator[valid_mask] / denominator[valid_mask]
     
     # Simplify smooth switching function
-    smooth = -6 * u**5 + 15 * u**4 - 10 * u**3 + 1
+    smooth = u * u * u * (u * (u * 6 - 15) + 10)

deepmd/dpmodel/model/model.py Show resolved Hide resolved
@anyangml anyangml marked this pull request as ready for review November 1, 2024 03:30
@anyangml anyangml requested a review from njzjz November 1, 2024 03:30
Copy link

codecov bot commented Nov 1, 2024

Codecov Report

Attention: Patch coverage is 90.47619% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.33%. Comparing base (704db2f) to head (0e4208d).
Report is 2 commits behind head on devel.

Files with missing lines Patch % Lines
deepmd/dpmodel/model/dp_zbl_model.py 82.35% 3 Missing ⚠️
deepmd/dpmodel/model/model.py 95.23% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            devel    #4292   +/-   ##
=======================================
  Coverage   84.33%   84.33%           
=======================================
  Files         555      556    +1     
  Lines       51884    51924   +40     
  Branches     3054     3054           
=======================================
+ Hits        43754    43788   +34     
- Misses       7172     7174    +2     
- Partials      958      962    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@anyangml anyangml enabled auto-merge November 1, 2024 03:59
@anyangml anyangml added this pull request to the merge queue Nov 1, 2024
Merged via the queue into deepmodeling:devel with commit 5c32147 Nov 1, 2024
60 checks passed
@anyangml anyangml deleted the feat/zbl-consistency-test branch November 1, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add consistent tests for the ZBL model
2 participants