Fix: dptest label mismatch #5082
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughTwo files modified: a formatting update adding a blank line in the test module, and a logic refinement in the data utility that adds an explicit subset check for all-atom particle table handling in the data loading function. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
🧹 Nitpick comments (2)
deepmd/entrypoints/test.py (2)
1137-1137: Consider removing commented code rather than keeping it.While commenting out the
type_selargument appears intentional (likely to fix the label mismatch), keeping commented code reduces readability. If this argument is no longer needed, remove the comment entirely.Apply this diff:
- # type_sel=dp.get_sel_type(),
1278-1278: Consider removing commented code rather than keeping it.Similar to the change in
test_polar, this commented line should be removed entirely if thetype_selargument is no longer needed.Apply this diff:
- # type_sel=dp.get_sel_type(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
deepmd/dpmodel/infer/deep_eval.py(1 hunks)deepmd/entrypoints/test.py(2 hunks)deepmd/jax/infer/deep_eval.py(1 hunks)deepmd/pt/infer/deep_eval.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4144
File: source/api_cc/tests/test_deeppot_dpa_pt.cc:166-246
Timestamp: 2024-09-19T04:25:12.408Z
Learning: Refactoring between test classes `TestInferDeepPotDpaPt` and `TestInferDeepPotDpaPtNopbc` is addressed in PR #3905.
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4144
File: source/api_cc/tests/test_deeppot_dpa_pt.cc:166-246
Timestamp: 2024-10-08T15:32:11.479Z
Learning: Refactoring between test classes `TestInferDeepPotDpaPt` and `TestInferDeepPotDpaPtNopbc` is addressed in PR #3905.
🧬 Code graph analysis (2)
deepmd/jax/infer/deep_eval.py (4)
deepmd/jax/jax2tf/tfmodel.py (1)
model_output_type(238-240)deepmd/jax/jax2tf/serialization.py (1)
model_output_type(285-286)deepmd/jax/model/hlo.py (1)
model_output_type(234-236)deepmd/dpmodel/model/make_model.py (1)
model_output_type(184-193)
deepmd/dpmodel/infer/deep_eval.py (4)
deepmd/dpmodel/model/base_model.py (1)
model_output_type(99-100)deepmd/dpmodel/model/make_model.py (1)
model_output_type(184-193)deepmd/dpmodel/model/spin_model.py (1)
model_output_type(271-273)deepmd/pt/model/model/make_model.py (1)
model_output_type(90-101)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: CodeQL analysis (python)
- GitHub Check: Agent
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C library (2.14, >=2.5.0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Analyze (python)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
🔇 Additional comments (3)
deepmd/pt/infer/deep_eval.py (1)
251-251: LGTM! Broadening polar model recognition to include "polarizability".The change correctly extends the model type detection to recognize models outputting "polarizability" as polar models alongside the existing "polar" check. This is consistent with the PR objective and aligns with similar changes across all three backend implementations.
deepmd/jax/infer/deep_eval.py (1)
161-161: LGTM! Consistent with other backends.The change correctly extends polar model recognition to include "polarizability", maintaining consistency with the pt and dpmodel backend implementations.
deepmd/dpmodel/infer/deep_eval.py (1)
140-140: LGTM! Consistent implementation across all backends.The change correctly extends the polar model recognition to include "polarizability", completing the uniform implementation across dpmodel, jax, and pt backends.
There was a problem hiding this comment.
Pull request overview
This PR fixes a label mismatch issue in dptest for polarizability models. The core issue was that the model output definition uses "polarizability" as the key, but the model type detection code was only checking for "polar" in the model output type, causing polarizability models to not be recognized correctly.
- Added
"polarizability" in model_output_typecheck to properly detect polar models across PT, JAX, and dpmodel backends - Removed
type_selparameter from polar and dipole test functions as it's not needed for these model types (data is global, not per-type)
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| deepmd/pt/infer/deep_eval.py | Added check for "polarizability" in model_output_type to correctly identify polar models |
| deepmd/jax/infer/deep_eval.py | Added check for "polarizability" in model_output_type to correctly identify polar models |
| deepmd/dpmodel/infer/deep_eval.py | Added check for "polarizability" in model_output_type to correctly identify polar models |
| deepmd/entrypoints/test.py | Commented out type_sel parameter in test_polar and test_dipole functions as it's not applicable to these global properties |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Plan: Pro
📒 Files selected for processing (2)
deepmd/entrypoints/test.py(2 hunks)deepmd/utils/data.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4144
File: source/api_cc/tests/test_deeppot_dpa_pt.cc:166-246
Timestamp: 2024-09-19T04:25:12.408Z
Learning: Refactoring between test classes `TestInferDeepPotDpaPt` and `TestInferDeepPotDpaPtNopbc` is addressed in PR #3905.
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4144
File: source/api_cc/tests/test_deeppot_dpa_pt.cc:166-246
Timestamp: 2024-10-08T15:32:11.479Z
Learning: Refactoring between test classes `TestInferDeepPotDpaPt` and `TestInferDeepPotDpaPtNopbc` is addressed in PR #3905.
🧬 Code graph analysis (1)
deepmd/entrypoints/test.py (2)
deepmd/utils/data.py (1)
add(137-190)deepmd/utils/data_system.py (1)
add(344-397)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (6, 3.9)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (6, 3.12)
- GitHub Check: Test Python (5, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C library (2.14, >=2.5.0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
deepmd/utils/data.py (1)
640-672: Subset-onlytype_selhandling in_load_datalooks correctGating the atomic+
type_selbranch onnatoms_sel != natomsconfines the reshape/selection logic to true subsets and avoids unnecessarily reprocessing the “all types selected” case. This matches the semantics implied bytype_seland makes the behavior less surprising; the rest of the branch and error handling remain consistent.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## devel #5082 +/- ##
==========================================
- Coverage 84.28% 84.28% -0.01%
==========================================
Files 709 709
Lines 70561 70562 +1
Branches 3618 3618
==========================================
- Hits 59472 59470 -2
- Misses 9923 9924 +1
- Partials 1166 1168 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
We have already had an option in this function, output_natoms_for_type_sel. You could set it to true for your case.
2 participants
Currently, when running dptest on atomic polar. The order of atoms for
coordandlabelare misaligned.This is a result of type_sel = None for coord, but not None for label.
I tried to set it to None for label as well, but it broke TF compatibility.
So the easiest hack I can think of is to add a check while loading data.