fix bug: cannot set property default_mesh of DeepmdDataSystem
#4360
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## r2 #4360 +/- ##
=======================================
Coverage 81.51% 81.51%
=======================================
Files 342 342
Lines 33882 33888 +6
Branches 2881 2878 -3
=======================================
+ Hits 27618 27623 +5
- Misses 5381 5382 +1
Partials 883 883 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DeepmdDataSystem
User->>DeepmdDataSystem: Set default_mesh
DeepmdDataSystem->>DeepmdDataSystem: Initialize _default_mesh
DeepmdDataSystem->>DeepmdDataSystem: Cache default_mesh values
User->>DeepmdDataSystem: Request test_size
DeepmdDataSystem->>DeepmdDataSystem: Calculate test size based on percentage
DeepmdDataSystem->>DeepmdDataSystem: Load test data using _load_test
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: 0
🧹 Outside diff range and nitpick comments (1)
deepmd_utils/utils/data_system.py (1)
246-248: Consider adding cache invalidation and validationThe current implementation has two potential issues:
- The
@lru_cacheon the getter could return stale data if the mesh is modified through the setter- The setter lacks validation of the input value
Consider this improved implementation:
@default_mesh.setter def default_mesh(self, value: List[np.ndarray]): + if not isinstance(value, list) or not all(isinstance(x, np.ndarray) for x in value): + raise ValueError("default_mesh must be a List[np.ndarray]") + if len(value) != self.nsystems: + raise ValueError(f"Expected {self.nsystems} meshes, got {len(value)}") self._default_mesh = value + # Clear the lru_cache to ensure fresh data + self.default_mesh.fget.cache_clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd_utils/utils/data_system.py(2 hunks)
🔇 Additional comments (2)
deepmd_utils/utils/data_system.py (2)
98-98: LGTM: Proper initialization of private attribute
The initialization of _default_mesh follows Python conventions for private attributes and is placed appropriately in the constructor.
237-244: LGTM: Well-implemented property with caching
The implementation correctly uses @lru_cache for performance optimization and implements lazy initialization of mesh values.
|
I am going to fix the Python tests at least, otherwise it's not known that whether the PR breaks other tests. |
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated no suggestions.
Comments skipped due to low confidence (1)
deepmd_utils/utils/data_system.py:4
- The word 'dat' should be corrected to 'data'.
The test dat of system with index `sys_idx` will be returned.
Currently, the multi-task cannot work correctly, and an error is thrown. (See #4358 for more details)
To fix this bug, a minor change in
DeepmdDataSystemis made to make the propertydefault_mesheditable.Summary by CodeRabbit
New Features
Deprecations
get_testmethod as deprecated, shifting to a new approach for accessing test data.