deepmd/tf/utils/parallel_op.py (1)

Line range hint 68-69: Verified: Import change has no negative impact on existing code

The change from typing.Generator to collections.abc.Generator doesn't negatively impact the existing code. The Generator type is correctly used in the type hint for the feed parameter in the generate method.

For improved clarity and consistency with modern Python typing practices, consider updating the return type hint of the generate method:

    def generate(
        self, sess: tf.Session, feed: Generator[Dict[str, Any], None, None]
-   ) -> Generator[Tuple, None, None]:
+   ) -> Generator[Tuple[Any, ...], None, None]:

This change specifies that the generator yields tuples of any length containing any type, which is more precise than the current hint.

Also applies to: 70-71, 72-73, 74-75

.github/workflows/test_python.yml (1)

Line range hint 1-99: Consider the broader impact of dropping Python 3.8 support

The changes in this file consistently update the Python versions used in the CI workflow, which is a good start for dropping Python 3.8 support. However, consider the following points to ensure a smooth transition:

1. Update project documentation, including README, installation instructions, and any version compatibility matrices.
2. Review and update any version-specific code or dependencies throughout the project.
3. Consider adding a note in the project's changelog or release notes about this breaking change.
4. Ensure that all project dependencies are compatible with Python 3.9+.
5. Update any setup files (e.g., setup.py, pyproject.toml) to reflect the new minimum Python version.

To help with this transition, you might want to run the following script to identify files that may need updating:

#!/bin/bash
# Description: Identify files that may need updating due to Python version change

# Search for files mentioning Python versions
rg -l 'python.*3\.[89]' --type-not yml

# Search for setup files
fd -e py -e toml '(setup|pyproject)'

# Search for documentation files
fd -e md -e rst '(README|INSTALL|CHANGELOG)'

This will help ensure that the entire project is consistent with the decision to drop Python 3.8 support.

doc/install/easy-install.md (2)

13-13: Approved: Python version requirement updated.

The Python version requirement has been correctly updated from 3.8 to 3.9, which aligns with the PR objective of dropping Python 3.8 support.

Consider adding a note about this being a breaking change, perhaps in a "What's New" or "Changelog" section, to clearly communicate this update to users. This could help users understand why they might need to update their Python environment.

---

Line range hint 1-128: Consider minor improvements for better user experience

The documentation is well-structured and informative. Consider the following suggestions to further enhance user experience:

1. Add a brief explanation of what DeePMD-kit is at the beginning of the document for new users.
2. Include a "Requirements" section at the top, summarizing all prerequisites (e.g., Python version, GNU C Library version, NVIDIA driver for GPU version).
3. Add links to the official documentation or GitHub repository for each third-party tool mentioned (e.g., conda, docker).
4. Consider adding a "Troubleshooting" section at the end with common installation issues and their solutions.

These additions could make the installation process smoother for users, especially those new to DeePMD-kit or less experienced with software installation.

deepmd/utils/env_mat_stat.py (1)

Line range hint 170-178: Verify: Limited impact of import change and suggestion for type hinting

The import change from typing to collections.abc for Iterator has a limited impact on the codebase. The only usage of Iterator is in the method signature of EnvMatStat.iter(), which remains correct after the change.

As a small improvement, consider updating the type hint for the data parameter in the iter method to use typing.List instead of List. This would make the import more explicit:

from typing import Dict, List as TypingList, Optional

# ...

def iter(self, data: TypingList[Dict[str, np.ndarray]]) -> Iterator[Dict[str, StatItem]]:

This change isn't strictly necessary but could improve code readability and import clarity.

deepmd/tf/utils/neighbor_stat.py (1)

Line range hint 141-144: Approve: Added default value for mixed_type parameter

The addition of a default value (False) for the mixed_type parameter in the NeighborStat constructor is a good improvement. It simplifies the instantiation of NeighborStat objects when mixed type behavior is not required, while maintaining backwards compatibility.

Consider updating the class docstring to reflect this change:

class NeighborStat(BaseNeighborStat):
    """Class for getting training data information.

    It loads data from DeepmdData object, and measures the data info, including neareest nbor distance between atoms, max nbor size of atoms and the output data range of the environment matrix.

    Parameters
    ----------
    ntypes : int
        The num of atom types
    rcut : float
        The cut-off radius
    mixed_type : bool, optional
        Treat all types as a single type. Default is False.
    """

pyproject.toml (3)

31-34: LGTM! Consider updating the Programming Language :: Python :: 3 :: Only classifier.

The changes to drop Python 3.8 support and add support for Python 3.9-3.12 are well implemented. The requires-python field has been correctly updated to ">=3.9".

Consider updating the existing classifier "Programming Language :: Python :: 3 :: Only" to "Programming Language :: Python :: 3.9 :: Only" to more accurately reflect the minimum supported Python version.

Also applies to: 57-57

---

Line range hint 258-259: LGTM! Consider documenting the PyTorch version constraint.

The addition of the PYTORCH_VERSION environment variable and the accompanying comment explaining the version constraint are good practices. This ensures compatibility between PyTorch and the project's CUDA requirements.

Consider adding a note in the project's documentation (e.g., README.md or a requirements file) about the PyTorch version constraint and the reason for it. This will help other developers understand the limitation without having to dig into the build configuration.

---

Add support for additional Python versions in cibuildwheel configuration

The current cibuildwheel setup only includes Python 3.11. To ensure compatibility with the required Python versions (3.9, 3.10, and 3.12), please update the build array in pyproject.toml accordingly.

• Update build = ["cp311-*"] to include the additional versions:

  build = ["cp39-*", "cp310-*", "cp311-*", "cp312-*"]

🔗 Analysis chain

Line range hint 202-204: Verify cibuildwheel configuration compatibility

While there are no visible changes in the [tool.cibuildwheel] section, please ensure that the existing configuration, particularly the build = ["cp311-*"] line, is compatible with the new Python version requirements (3.9+). Consider if you need to add builds for Python 3.9, 3.10, and 3.12 as well.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify if cibuildwheel configuration is compatible with new Python requirements

# Check if there are any references to Python 3.8 in the cibuildwheel configuration
grep -n "cp38" pyproject.toml

# List all Python version references in the cibuildwheel configuration
grep -n "cp3" pyproject.toml

Length of output: 245

doc/install/install-from-source.md (1)

24-24: LGTM: Python version requirement updated correctly.

The change from Python 3.8 to 3.9 as the minimum required version is consistent with the PR objective of dropping Python 3.8 support.

Consider updating other parts of the documentation that might reference Python 3.8, such as example commands or compatibility notes, to ensure consistency throughout the document.

deepmd/tf/infer/deep_eval.py (1)

Line range hint 1-1230: Overall: Consistent modernization, but address potential issues

The changes in this file consistently update the caching mechanism from lru_cache to cache, aligning with the PR's objective of dropping Python 3.8 support. This modernization simplifies the code and leverages newer Python features.

However, there are several points to consider:

1. Memory Management: The static analysis tool flagged potential memory leak issues with caching methods. Ensure that all cached properties don't depend on mutable state and that the number of unique instances is bounded.

2. Session Handling: Pay special attention to the caching of the TensorFlow session. Implement proper cleanup procedures to prevent resource leaks.

3. Code Maintenance: The updates to DeepEvalOld raise questions about code maintenance strategies. Consider deprecating or removing unused code to reduce technical debt.

4. Documentation: Add comments explaining the caching strategy, cleanup procedures, and future plans for deprecated classes.

5. Testing: Ensure that these changes are covered by comprehensive tests, especially focusing on memory usage and session management.

By addressing these points, you'll not only modernize the code but also improve its overall quality and maintainability.

deepmd/dpmodel/utils/neighbor_stat.py (2)

Line range hint 53-54: Typo in docstring: 'neareest' should be 'nearest'

In the docstring of the call method, the word "neareest" is misspelled. Please correct it to "nearest".

Apply this diff to fix the typo:

-        """Calculate the neareest neighbor distance between atoms, maximum nbor size of
+        """Calculate the nearest neighbor distance between atoms, maximum neighbor size of

Additionally, consider expanding "nbor" to "neighbor" for clarity.

---

Line range hint 127-129: Inconsistent parameter naming: mixed_type vs mixed_types

There is an inconsistency in the parameter naming between the NeighborStat and NeighborStatOP classes:

• In NeighborStat.__init__, the parameter is named mixed_type (singular).
• In NeighborStatOP.__init__, the parameter is named mixed_types (plural).

For consistency and to avoid confusion, please standardize the parameter name across both classes.

Apply this diff to standardize the parameter name to mixed_types:

In NeighborStat class:

 class NeighborStat(BaseNeighborStat):
     def __init__(
         self,
         ntypes: int,
         rcut: float,
-        mixed_type: bool = False,
+        mixed_types: bool = False,
     ) -> None:
-        super().__init__(ntypes, rcut, mixed_type)
-        self.op = NeighborStatOP(ntypes, rcut, mixed_type)
+        super().__init__(ntypes, rcut, mixed_types)
+        self.op = NeighborStatOP(ntypes, rcut, mixed_types)

Also, update the docstrings to reflect the parameter name change.

Also applies to: 39-42

deepmd/pt/utils/neighbor_stat.py (3)

Line range hint 94-95: Potential issue with masking diff tensor due to dimensionality mismatch

The masking operation on the diff tensor may not correctly exclude self-interactions. The mask created with torch.eye(nloc, nall, dtype=torch.bool, device=diff.device) has shape [nloc, nall], which may not align properly with diff of shape [nframes, nloc, nall, 3]. This can lead to incorrect masking or runtime errors.

Consider adjusting the mask to match the dimensions of diff by unsqueezing it along the frame dimension:

-mask = torch.eye(nloc, nall, dtype=torch.bool, device=diff.device)
+mask = torch.eye(nloc, nall, dtype=torch.bool, device=diff.device).unsqueeze(0)
 diff[mask] = torch.inf

Alternatively, you might use advanced indexing or broadcasting to apply the mask correctly across all frames.

---

Line range hint 104-106: Optimize neighbor count calculation by vectorizing over atom types

The loop over range(self.ntypes) to compute nnei can be a performance bottleneck for systems with many atom types. Vectorizing this operation can enhance performance by leveraging PyTorch's efficient tensor computations.

Refactor the code as follows to eliminate the Python loop:

 for ii in range(self.ntypes):
     nnei[:, :, ii] = torch.sum(
         mask & extend_atype.eq(ii)[:, None, :], dim=-1
     )
+else:
+    # Vectorized computation over atom types
+    type_mask = extend_atype[:, None, :, None] == torch.arange(self.ntypes, device=extend_atype.device)
+    nnei = torch.sum(mask[..., None] & type_mask, dim=-2)

This approach utilizes broadcasting and removes the explicit loop, improving scalability and performance.

---

Line range hint 134-139: Inconsistent parameter naming: mixed_type vs. mixed_types

The parameter mixed_type in NeighborStat is inconsistently named compared to mixed_types in NeighborStatOP. This mismatch can lead to confusion and potential errors.

To ensure consistency, rename mixed_type to mixed_types in NeighborStat:

 class NeighborStat(BaseNeighborStat):
     """Neighbor statistics using pure NumPy.

     def __init__(
         self,
         ntypes: int,
         rcut: float,
-        mixed_type: bool = False,
+        mixed_types: bool = False,
     ) -> None:
-        super().__init__(ntypes, rcut, mixed_type)
-        op = NeighborStatOP(ntypes, rcut, mixed_type)
+        super().__init__(ntypes, rcut, mixed_types)
+        op = NeighborStatOP(ntypes, rcut, mixed_types)
         self.op = torch.jit.script(op)
         self.auto_batch_size = AutoBatchSize()

Ensure all references to mixed_type within NeighborStat are updated accordingly.

deepmd/pt/utils/env_mat_stat.py (3)

Line range hint 22-24: Typo in Class Docstring

There's a typo in the class docstring of EnvMatStatSe:

"Environmental matrix statistics for the se_a/se_r environemntal matrix."

The word "environemntal" should be "environmental".

---

Line range hint 44-50: Typo in Exception Message

In the __init__ method's error handling, there's a typo in the exception message:

"last_dim should be 1 for raial-only or 4 for full descriptor."

It should be "radial-only".

---

Line range hint 56-66: Enhance Parameter Description in Docstring

In the iter method's docstring, the description for the data parameter is minimal:

data : List[Dict[str, Union[torch.Tensor, List[Tuple[int, int]]]]]

The data.

Consider providing a more detailed description of the expected format and contents of data to improve understandability for users of the method.

deepmd/utils/compat.py (4)

Line range hint 124-153: Ensure 'training' key exists in jdata before accessing

In the convert_input_v1_v2 function, accessing jdata["training"] without checking if the 'training' key exists may lead to a KeyError if 'training' is missing in jdata.

Consider adding a check to ensure 'training' exists:

 def convert_input_v1_v2(
     jdata: Dict[str, Any], warning: bool = True, dump: Optional[Union[str, Path]] = None
 ) -> Dict[str, Any]:
+    if "training" not in jdata:
+        raise KeyError("The 'training' key is missing in jdata.")
     tr_cfg = jdata["training"]

---

Line range hint 153-184: Handle missing 'training' key safely in deprecate_numb_test

Using jdata.get("training", {}) may inadvertently create a new empty dictionary if 'training' does not exist, and calling pop("numb_test") on it will raise a KeyError.

Modify the code to check for the existence of 'training' and 'numb_test' keys:

 def deprecate_numb_test(
     jdata: Dict[str, Any], warning: bool = True, dump: Optional[Union[str, Path]] = None
 ) -> Dict[str, Any]:
-    try:
-        jdata.get("training", {}).pop("numb_test")
-    except KeyError:
-        pass
+    training_cfg = jdata.get("training")
+    if training_cfg and "numb_test" in training_cfg:
+        training_cfg.pop("numb_test")
+        if warning:
+            warnings.warn(
+                "The argument training->numb_test has been deprecated since v2.0.0. "
+                "Use training->validation_data->batch_size instead."
+            )

---

Line range hint 185-198: Prevent KeyError when checking 'systems' in jdata["training"]

In the is_deepmd_v1_input function, accessing jdata["training"] without verifying its existence can cause a KeyError if 'training' is not a key in jdata.

Adjust the function to safely check for 'training':

 def is_deepmd_v1_input(jdata):
-    return "systems" in jdata["training"].keys()
+    return "training" in jdata and "systems" in jdata["training"]

---

Line range hint 185-198: Simplify conditional logic in update_deepmd_input

Refactoring the conditionals can enhance readability and maintainability.

You can restructure the update_deepmd_input function as follows:

 def update_deepmd_input(
     jdata: Dict[str, Any], warning: bool = True, dump: Optional[Union[str, Path]] = None
 ) -> Dict[str, Any]:
-    def is_deepmd_v0_input(jdata):
-        return "model" not in jdata.keys()
-
-    def is_deepmd_v1_input(jdata):
-        return "training" in jdata and "systems" in jdata["training"]
-
-    if is_deepmd_v0_input(jdata):
+    if "model" not in jdata:
         jdata = convert_input_v0_v1(jdata, warning, None)
-        jdata = convert_input_v1_v2(jdata, False, None)
-        jdata = deprecate_numb_test(jdata, False, dump)
-    elif is_deepmd_v1_input(jdata):
-        jdata = convert_input_v1_v2(jdata, warning, None)
-        jdata = deprecate_numb_test(jdata, False, dump)
+        warning = False
+    if "training" in jdata and "systems" in jdata["training"]:
+        jdata = convert_input_v1_v2(jdata, warning, None)
+        warning = False
     jdata = deprecate_numb_test(jdata, warning, dump)
     return jdata

Configuration used: CodeRabbit UI
Review profile: CHILL