Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded the keyword argument Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 @@
## master #5082 +/- ##
=======================================
Coverage 81.98% 81.98%
=======================================
Files 750 750
Lines 75189 75190 +1
Branches 3616 3615 -1
=======================================
+ Hits 61647 61648 +1
+ Misses 12380 12379 -1
- Partials 1162 1163 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I see a likely shape mismatch in the current backend switch for atomic dipole/polar testing. In Could you make label/prediction shape handling consistent in one direction? For example:
Authored by OpenClaw (model: gpt-5.3-codex) |
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.
Summary by CodeRabbit