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

breaking: drop support of py3.7. fix: pin scipy constants to version 2018 #774

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ jobs:
runs-on: ubuntu-22.04
strategy:
matrix:
python-version: ["3.7", "3.8", "3.12"]
python-version: ["3.8", "3.12"]

steps:
- uses: actions/checkout@v4
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

## Installation

DP-GEN only supports Python 3.7 and above. You can [setup a conda/pip environment](https://docs.deepmodeling.com/faq/conda.html), and then use one of the following methods to install DP-GEN:
dpdata only supports Python 3.8 and above. You can [setup a conda/pip environment](https://docs.deepmodeling.com/faq/conda.html), and then use one of the following methods to install dpdata:

- Install via pip: `pip install dpdata`
- Install via conda: `conda install -c conda-forge dpdata`
Expand Down
7 changes: 1 addition & 6 deletions docs/make_format.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@

import csv
import os
import sys
from collections import defaultdict
from inspect import Parameter, Signature, cleandoc, signature

if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal
from typing import Literal

from numpydoc.docscrape import Parameter as numpydoc_Parameter
from numpydoc.docscrape_sphinx import SphinxDocString
Expand Down
8 changes: 4 additions & 4 deletions dpdata/stat.py
Original file line number Diff line number Diff line change
Expand Up @@ -116,15 +116,15 @@ class Errors(ErrorsBase):
SYSTEM_TYPE = LabeledSystem

@property
@lru_cache()
@lru_cache
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Remove @lru_cache to prevent potential memory leaks.

Using @lru_cache on methods can lead to memory leaks as it keeps a reference to self, potentially preventing proper garbage collection of class instances. Since these properties compute errors between two systems, the performance benefit of caching might not outweigh the risk of memory leaks.

Apply this diff to fix the issue:

    @property
-    @lru_cache
     def e_errors(self) -> np.ndarray:
         """Energy errors."""

    @property
-    @lru_cache
     def f_errors(self) -> np.ndarray:
         """Force errors."""

    @property
-    @lru_cache
     def e_errors(self) -> np.ndarray:
         """Energy errors."""

    @property
-    @lru_cache
     def f_errors(self) -> np.ndarray:
         """Force errors."""

If caching is necessary for performance reasons, consider:

  1. Using a different caching strategy that doesn't retain references to instances
  2. Implementing manual cache invalidation
  3. Moving the cached computation to a standalone function

Also applies to: 127-127, 156-156, 169-169

🧰 Tools
🪛 Ruff (0.8.2)

119-119: Use of functools.lru_cache or functools.cache on methods can lead to memory leaks

(B019)

def e_errors(self) -> np.ndarray:
"""Energy errors."""
assert isinstance(self.system_1, self.SYSTEM_TYPE)
assert isinstance(self.system_2, self.SYSTEM_TYPE)
return self.system_1["energies"] - self.system_2["energies"]

@property
@lru_cache()
@lru_cache
def f_errors(self) -> np.ndarray:
"""Force errors."""
assert isinstance(self.system_1, self.SYSTEM_TYPE)
Expand Down Expand Up @@ -153,7 +153,7 @@ class MultiErrors(ErrorsBase):
SYSTEM_TYPE = MultiSystems

@property
@lru_cache()
@lru_cache
def e_errors(self) -> np.ndarray:
"""Energy errors."""
assert isinstance(self.system_1, self.SYSTEM_TYPE)
Expand All @@ -166,7 +166,7 @@ def e_errors(self) -> np.ndarray:
return np.concatenate(errors)

@property
@lru_cache()
@lru_cache
def f_errors(self) -> np.ndarray:
"""Force errors."""
assert isinstance(self.system_1, self.SYSTEM_TYPE)
Expand Down
7 changes: 1 addition & 6 deletions dpdata/system.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,16 @@
import hashlib
import numbers
import os
import sys
import warnings
from copy import deepcopy
from typing import (
TYPE_CHECKING,
Any,
Iterable,
Literal,
overload,
)

if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal

import numpy as np

import dpdata
Expand Down
48 changes: 43 additions & 5 deletions dpdata/unit.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,49 @@

from scipy import constants # noqa: TID253

AVOGADRO = constants.Avogadro # Avagadro constant
ELE_CHG = constants.elementary_charge # Elementary Charge, in C
BOHR = constants.value("atomic unit of length") # Bohr, in m
HARTREE = constants.value("atomic unit of energy") # Hartree, in Jole
RYDBERG = constants.Rydberg * constants.h * constants.c # Rydberg, in Jole
physical_constants = {}
# use constants up to 2018
physical_constants.update(constants._codata._physical_constants_2002)
physical_constants.update(constants._codata._physical_constants_2006)
physical_constants.update(constants._codata._physical_constants_2010)
physical_constants.update(constants._codata._physical_constants_2014)
physical_constants.update(constants._codata._physical_constants_2018)
Comment on lines +9 to +13
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid accessing private attributes of scipy.constants

Accessing private attributes like constants._codata._physical_constants_2002 is not recommended and leads to errors, as these attributes are not part of the public API and may not be available in all versions of SciPy. The pipeline failure indicates that the _codata attribute is inaccessible.

To fix this issue and pin the constants to the CODATA 2018 values, you can manually define the required constants. Here's how you can modify the code:

-physical_constants = {}
-# use constants up to 2018
-physical_constants.update(constants._codata._physical_constants_2002)
-physical_constants.update(constants._codata._physical_constants_2006)
-physical_constants.update(constants._codata._physical_constants_2010)
-physical_constants.update(constants._codata._physical_constants_2014)
-physical_constants.update(constants._codata._physical_constants_2018)

-# copied from scipy
-def scipy_constant_value(key: str) -> float:
-    constants._codata._check_obsolete(key)
-    return physical_constants[key][0]

+physical_constants = {
+    "Avogadro constant": 6.02214076e23,  # mol^-1
+    "elementary charge": 1.602176634e-19,  # C
+    "atomic unit of length": 5.29177210903e-11,  # m
+    "atomic unit of energy": 4.3597447222071e-18,  # J
+    "Rydberg constant": 10973731.568160,  # m^-1
+    "Planck constant": 6.62607015e-34,  # J·s
+    "speed of light in vacuum": 299792458,  # m·s^-1
+}
+
+def constant_value(key: str) -> float:
+    return physical_constants[key]

This approach ensures that you're using the CODATA 2018 values without relying on private attributes.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
physical_constants.update(constants._codata._physical_constants_2002)
physical_constants.update(constants._codata._physical_constants_2006)
physical_constants.update(constants._codata._physical_constants_2010)
physical_constants.update(constants._codata._physical_constants_2014)
physical_constants.update(constants._codata._physical_constants_2018)
physical_constants = {
"Avogadro constant": 6.02214076e23, # mol^-1
"elementary charge": 1.602176634e-19, # C
"atomic unit of length": 5.29177210903e-11, # m
"atomic unit of energy": 4.3597447222071e-18, # J
"Rydberg constant": 10973731.568160, # m^-1
"Planck constant": 6.62607015e-34, # J·s
"speed of light in vacuum": 299792458, # m·s^-1
}
def constant_value(key: str) -> float:
return physical_constants[key]
🧰 Tools
🪛 GitHub Actions: Python package

[error] 9-9: Failed to access '_codata' attribute from scipy.constants module. The line 'physical_constants.update(constants._codata._physical_constants_2002)' is trying to access a private/non-existent attribute.



# copied from scipy
def scipy_constant_value(key: str) -> float:
"""Value in physical_constants indexed by key.

Parameters
----------
key : Python string
Key in dictionary `physical_constants`

Returns
-------
value : float
Value in `physical_constants` corresponding to `key`

Examples
--------
>>> from scipy import constants
>>> constants.value('elementary charge')
1.602176634e-19

"""
constants._codata._check_obsolete(key)
return physical_constants[key][0]


AVOGADRO = scipy_constant_value("Avogadro constant") # Avagadro constant
ELE_CHG = scipy_constant_value("elementary charge") # Elementary Charge, in C
BOHR = scipy_constant_value("atomic unit of length") # Bohr, in m
HARTREE = scipy_constant_value("atomic unit of energy") # Hartree, in Jole
RYDBERG = (
scipy_constant_value("Rydberg constant")
* scipy_constant_value("Planck constant")
* scipy_constant_value("speed of light in vacuum")
) # Rydberg, in Jole
wanghan-iapcm marked this conversation as resolved.
Show resolved Hide resolved

# energy conversions
econvs = {
Expand Down
7 changes: 1 addition & 6 deletions dpdata/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@

import io
import os
import sys
from contextlib import contextmanager
from typing import TYPE_CHECKING, Generator, overload
from typing import TYPE_CHECKING, Generator, Literal, overload

if sys.version_info >= (3, 8):
from typing import Literal
else:
from typing_extensions import Literal
import numpy as np

from dpdata.periodic_table import Element
Expand Down
4 changes: 1 addition & 3 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ authors = [
]
license = {file = "LICENSE"}
classifiers = [
"Programming Language :: Python :: 3.7",
"Programming Language :: Python :: 3.8",
"Programming Language :: Python :: 3.9",
"Programming Language :: Python :: 3.10",
Expand All @@ -28,7 +27,7 @@ dependencies = [
'importlib_metadata>=1.4; python_version < "3.8"',
'typing_extensions; python_version < "3.8"',
]
requires-python = ">=3.7"
requires-python = ">=3.8"
readme = "README.md"
keywords = ["lammps", "vasp", "deepmd-kit"]

Expand All @@ -46,7 +45,6 @@ test = [
ase = ['ase']
amber = [
'parmed; python_version >= "3.8"',
'parmed<4; python_version < "3.8"',
]
pymatgen = ['pymatgen']
docs = [
Expand Down
Loading