-
Notifications
You must be signed in to change notification settings - Fork 527
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
Fix: add model_def_script to ZBL #4423
Conversation
📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant ModelFactory
participant DPZBLModel
Caller->>ModelFactory: get_zbl_model()
ModelFactory->>DPZBLModel: Create instance
ModelFactory->>ModelFactory: model = instance
ModelFactory->>ModelFactory: model.model_def_script = JSON(model_params)
ModelFactory->>Caller: return model
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (2)deepmd/pt/model/model/__init__.py (2)Line range hint The addition of Line range hint Let's verify that all model types properly set the ✅ Verification successfulLet me gather more information to specifically check for model_def_script assignments. Model definition script is consistently set across model types The verification shows that model_def_script is properly handled:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if model_def_script is consistently set across all model types
# Search for model instantiations and model_def_script assignments
ast-grep --pattern 'model = $CLASS($$$)
$$$
model.model_def_script = $$$'
# Search for potential missing model_def_script assignments
ast-grep --pattern 'model = $CLASS($$$)' | grep -v 'model_def_script'
Length of output: 11751 Script: #!/bin/bash
# Search for model_def_script assignments in model initialization files
rg -A 2 "model_def_script" deepmd/pt/model/model/
# Search for model initialization functions that might need model_def_script
ast-grep --pattern 'def get_$_model($$$):
$$$
model = $$$'
Length of output: 2137 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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4423 +/- ##
==========================================
+ Coverage 84.59% 84.64% +0.05%
==========================================
Files 614 614
Lines 57009 57137 +128
Branches 3488 3486 -2
==========================================
+ Hits 48224 48366 +142
+ Misses 7658 7645 -13
+ Partials 1127 1126 -1 ☔ View full report in Codecov by Sentry. |
@iProzd is it possible to add some tests in the universal directory? |
When I look at this file, I am wondering if other models (e.g. spin) have the same issue. |
likely |
Pairtab model does not support compression. @njzjz |
This seems to be a different topic. My insight is that an empty method is enough. |
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced model definition handling for improved encapsulation and consistency across different model types. - **Bug Fixes** - Ensured that model definition scripts are correctly set to a JSON string representation for all model instances. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit f343a3b)
<!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced model definition handling for improved encapsulation and consistency across different model types. - **Bug Fixes** - Ensured that model definition scripts are correctly set to a JSON string representation for all model instances. <!-- end of auto-generated comment: release notes by coderabbit.ai --> (cherry picked from commit f343a3b)
Summary by CodeRabbit
New Features
Bug Fixes