Fix: always output weighted average error #4401
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TestFunction
participant DeepGlobalPolar
participant DeepWFC
participant OtherModels
User->>TestFunction: Initiate test
TestFunction->>DeepGlobalPolar: Process system
DeepGlobalPolar-->>TestFunction: Return results
TestFunction->>DeepWFC: Process system
DeepWFC-->>TestFunction: Return results
TestFunction->>OtherModels: Process system
OtherModels-->>TestFunction: Return results
TestFunction->>TestFunction: Calculate weighted averages
TestFunction->>User: Log weighted averages
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd/entrypoints/test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
deepmd/entrypoints/test.py
195-198: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (1)
deepmd/entrypoints/test.py (1)
185-196: LGTM! The error logging structure is consistent.
The changes ensure consistent logging of weighted averages across different model types.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #4401 +/- ##
==========================================
+ Coverage 84.59% 84.64% +0.05%
==========================================
Files 614 614
Lines 57009 57007 -2
Branches 3486 3487 +1
==========================================
+ Hits 48224 48254 +30
+ Misses 7660 7627 -33
- Partials 1125 1126 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Signed-off-by: Chun Cai <amoycaic@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
deepmd/entrypoints/test.py (1)
195-198: Combine similar conditions for better maintainability.The conditions for
DeepPolarandDeepGlobalPolarcan be combined since they use the same print function.Apply this diff to combine the conditions:
- elif isinstance(dp, DeepPolar): - print_polar_sys_avg(avg_err) - elif isinstance(dp, DeepGlobalPolar): - print_polar_sys_avg(avg_err) + elif isinstance(dp, (DeepPolar, DeepGlobalPolar)): + print_polar_sys_avg(avg_err)🧰 Tools
🪛 Ruff (0.7.0)
195-198: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd/entrypoints/test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
deepmd/entrypoints/test.py
195-198: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (1)
deepmd/entrypoints/test.py (1)
199-200:
Verify DeepWFC import for runtime usage.
DeepWFC is imported conditionally with TYPE_CHECKING, but it's used in runtime code. This could lead to runtime errors.
Let's verify the import setup:
Consider moving the import outside the TYPE_CHECKING block if the class is needed at runtime:
-if TYPE_CHECKING:
- from deepmd.infer.deep_wfc import (
- DeepWFC,
- )
+from deepmd.infer.deep_wfc import (
+ DeepWFC,
+)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
deepmd/entrypoints/test.py (1)
Line range hint
1018-1052: Enhance docstring for test_wfc functionThe docstring is missing descriptions for some parameters and the return value structure.
Add the following parameter descriptions to the docstring:
def test_wfc( dp: "DeepWFC", data: DeepmdData, numb_test: int, detail_file: Optional[str], ) -> tuple[list[np.ndarray], list[int]]: - """Test energy type model. + """Test wave function coefficient (WFC) model. Parameters ---------- dp : DeepPot instance of deep potential data : DeepmdData data container object numb_test : int munber of tests to do detail_file : Optional[str] - file where test details will be output + file where test details will be output. If None, no details are saved. Returns ------- tuple[list[np.ndarray], list[int]] - arrays with results and their shapes + Dictionary containing RMSE metrics and their corresponding data sizes: + - 'rmse': Tuple of (RMSE value, size of WFC array) """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd/entrypoints/test.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
deepmd/entrypoints/test.py
195-198: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (1)
deepmd/entrypoints/test.py (1)
31-33: LGTM: Clean import addition for DeepWFC support
The import is properly placed with other model imports and is necessary for the new functionality.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
deepmd/entrypoints/test.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.7.0)
deepmd/entrypoints/test.py
195-198: Combine if branches using logical or operator
Combine if branches
(SIM114)
🔇 Additional comments (2)
deepmd/entrypoints/test.py (2)
37-39: LGTM!
The import of DeepWFC is correctly placed and necessary for the new model support.
199-200: LGTM!
The addition of the DeepWFC model type check and corresponding error printing is correctly implemented.
This PR makes all dp test result output the "weighted average of errors" part. It would be better for post-processing tools to handle the output data.
Summary by CodeRabbit
New Features
DeepWFCmodel, enhancing the functionality of the testing framework.Bug Fixes
DeepGlobalPolarmodel, ensuring consistent logging of average errors for all models.Refactor
DeepGlobalPolarmodel.