refactor: refactor update_sel and save min_nbor_dist #3829
Conversation
Fix deepmodeling#3525. Fix deepmodeling#3544. Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
WalkthroughWalkthroughThe changes across multiple files enhance the Changes
Assessment against linked issues
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (4)
Files skipped from review due to trivial changes (1)
Files skipped from review as they are similar to previous changes (1)
Additional comments not posted (2)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #3829 +/- ##
==========================================
+ Coverage 82.53% 82.62% +0.08%
==========================================
Files 515 515
Lines 49492 49563 +71
Branches 2985 2987 +2
==========================================
+ Hits 40849 40949 +100
+ Misses 7732 7703 -29
Partials 911 911 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
Good idea. The e2e and unit tests are not in conflict. |
|
We haven't had fixtures for descriptors, though. |
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range and nitpick comments (7)
deepmd/utils/update_sel.py (2)
86-115: Review the handling of emptytype_mapinget_nbor_stat. Consider adding a comment explaining why an emptytype_mapis set toNone.
[!TIP]
Codebase VerificationThe current unit tests for
update_one_selcover theautoandauto:1.5cases but do not test the integerselcase or the warning mechanism for insufficientselvalues. Please add test cases to cover these scenarios:
Integer
selinput.Warning mechanism when
selvalues are insufficient.source/tests/tf/test_train.py
source/tests/pt/test_update_sel.py
source/tests/common/dpmodel/test_update_sel.py
Analysis chain
Line range hint
25-59: Ensure thorough testing of theupdate_one_selmethod, especially the new logic handling differentseltypes and the warning mechanism.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the unit tests cover all branches in `update_one_sel`. # Test: Search for unit tests covering `update_one_sel`. rg --type py "test_update_one_sel"Length of output: 296
Script:
#!/bin/bash # Extract the content of the test_update_one_sel functions from the identified test files. # Extract from source/tests/tf/test_train.py rg --type py -A 20 "def test_update_one_sel" source/tests/tf/test_train.py # Extract from source/tests/pt/test_update_sel.py rg --type py -A 20 "def test_update_one_sel" source/tests/pt/test_update_sel.py # Extract from source/tests/common/dpmodel/test_update_sel.py rg --type py -A 20 "def test_update_one_sel" source/tests/common/dpmodel/test_update_sel.pyLength of output: 3198
source/tests/common/dpmodel/test_update_sel.py (1)
42-71: The testtest_update_sel_hybridprovides a comprehensive check of theupdate_selfunctionality for hybrid descriptors. Consider adding more comments to explain the setup and expected outcomes for clarity.doc/development/create-a-model-pt.md (1)
Line range hint
1-5: Consider adding a comma after "model" and correcting "trainner" to "trainer" for grammatical accuracy.- If you'd like to create a new model that isn't covered by the existing DeePMD-kit library, but reuse DeePMD-kit's other efficient modules such as data processing, trainner, etc, you may want to read this section. + If you'd like to create a new model that isn't covered by the existing DeePMD-kit library, but reuse DeePMD-kit's other efficient modules such as data processing, trainer, etc., you may want to read this section.source/tests/pt/test_update_sel.py (1)
16-20: Consider adding a docstring to theupdate_selfunction to explain its purpose and parameters.deepmd/tf/model/pairtab.py (1)
Line range hint
275-308: Consider adding error handling for cases wheremin_nbor_distisNoneto prevent potential runtime errors inPairTabModel.- if min_nbor_dist is not None: + if min_nbor_dist is not None: + return local_jdata_cpy, min_nbor_dist + else: + # Handle the case where min_nbor_dist is None + log.error("min_nbor_dist is None, which may indicate an issue with neighbor statistics calculation.") + raise ValueError("Failed to calculate min_nbor_dist.")source/tests/tf/test_train.py (1)
41-51: Refactor duplicated code intest_update_one_sel.Consider extracting the repeated logic into a helper function to avoid code duplication and improve maintainability.
|
@coderabbitai resolve |
Fix deepmodeling#3525. Fix deepmodeling#3544. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced `update_sel` method to accept additional parameters and return more detailed data, improving model selection and neighbor statistics. - **Bug Fixes** - Improved handling and processing of training data to enhance model accuracy. - **Refactor** - Updated method signatures and logic for consistency and better performance. - **Chores** - Removed unused `hook` method to streamline codebase. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Jinzhe Zeng <jinzhe.zeng@rutgers.edu> Co-authored-by: Han Wang <92130845+wanghan-iapcm@users.noreply.github.com>
Fix #3525. Fix #3544.
Summary by CodeRabbit
update_selmethod to accept additional parameters and return more detailed data, improving model selection and neighbor statistics.hookmethod to streamline codebase.