breaking: drop support of py3.7. fix: pin scipy constants to version 2018 #774
Conversation
CodSpeed Performance ReportMerging #774 will not alter performanceComparing Summary
|
📝 WalkthroughWalkthroughThe changes in Changes
Sequence DiagramsequenceDiagram
participant Code as Client Code
participant Unit as unit.py
participant Constants as physical_constants
Code->>Unit: Request constant value
Unit->>Unit: Call scipy_constant_value()
Unit->>Constants: Retrieve value by key
Constants-->>Unit: Return constant value
Unit-->>Code: Return constant value
The sequence diagram illustrates how constant values are now retrieved through the Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
dpdata/unit.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Python package
dpdata/unit.py
[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.
| 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) |
There was a problem hiding this comment.
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.
| 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.
for more information, see https://pre-commit.ci
wanghan-iapcm
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #774 +/- ##
==========================================
- Coverage 85.16% 85.12% -0.04%
==========================================
Files 81 81
Lines 7528 7528
==========================================
- Hits 6411 6408 -3
- Misses 1117 1120 +3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/test.yml(1 hunks)README.md(1 hunks)dpdata/stat.py(3 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
dpdata/stat.py
119-119: Use of functools.lru_cache or functools.cache on methods can lead to memory leaks
(B019)
127-127: Use of functools.lru_cache or functools.cache on methods can lead to memory leaks
(B019)
156-156: Use of functools.lru_cache or functools.cache on methods can lead to memory leaks
(B019)
169-169: Use of functools.lru_cache or functools.cache on methods can lead to memory leaks
(B019)
🔇 Additional comments (4)
.github/workflows/test.yml (1)
12-12: LGTM! Python version update is consistent.The removal of Python 3.7 from the test matrix aligns with the project's decision to support only Python 3.8 and above.
README.md (1)
11-11: LGTM! Documentation update is accurate.The README correctly reflects the new minimum Python version requirement of 3.8.
pyproject.toml (1)
30-30: LGTM! Project configuration is properly updated.The Python version requirement in pyproject.toml is consistent with the changes in other files.
dpdata/stat.py (1)
119-119: Verify the relevance of lru_cache changes.These changes appear unrelated to the PR's objective of pinning scipy constants to version 2018. Please clarify if these changes are intentional and necessary for this PR.
Also applies to: 127-127, 156-156, 169-169
🧰 Tools
🪛 Ruff (0.8.2)
119-119: Use of
functools.lru_cacheorfunctools.cacheon methods can lead to memory leaks(B019)
| @@ -116,15 +116,15 @@ class Errors(ErrorsBase): | |||
| SYSTEM_TYPE = LabeledSystem | |||
|
|
|||
| @property | |||
| @lru_cache() | |||
| @lru_cache | |||
There was a problem hiding this comment.
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:
- Using a different caching strategy that doesn't retain references to instances
- Implementing manual cache invalidation
- 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)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
dpdata/system.py (1)
14-15: Consider adding docstrings to overloads.While the type overloads enhance type safety and IDE support, consider adding docstrings to explain the purpose and expected behavior of each overload.
Example:
@overload def __getitem__(self, key: int | slice | list | np.ndarray) -> System: + """Get a subsystem by frame index. + + Parameters + ---------- + key : int | slice | list | np.ndarray + Frame index or indices to select + + Returns + ------- + System + A new system containing only the selected frames + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/make_format.py(1 hunks)dpdata/system.py(1 hunks)dpdata/utils.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- docs/make_format.py
🔇 Additional comments (2)
dpdata/utils.py (1)
6-6: LGTM! Import change aligns with Python 3.8+ requirement.The direct import of
Literalfromtypingis correct as it's natively available in Python 3.8+, which aligns with the PR objective to drop Python 3.7 support.dpdata/system.py (1)
13-15: LGTM! Import changes improve code organization.The import changes are well-organized and align with the Python 3.8+ requirement.
drop support of py3.7
fix issue #773
Summary by CodeRabbit
New Features
Bug Fixes
Refactor