fix(pt): get correct sel_type in pt model
#5097
Conversation
ChiahsinChu
force-pushed
the
devel-pt-sel_type
branch
from
17ca9af to
0b0b1a4
Compare
📝 WalkthroughWalkthroughReinitialize PyTorch fitting exclusion state during DPAtomicModel init and after type-map updates; inject a computed Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2024-10-16T21:49:57.401ZApplied to files:
🧬 Code graph analysis (4)source/tests/consistent/model/test_dipole.py (4)
deepmd/tf/model/model.py (3)
source/tests/universal/dpmodel/atomc_model/test_atomic_model.py (3)
deepmd/dpmodel/atomic_model/dp_atomic_model.py (4)
⏰ 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). (28)
🔇 Additional comments (6)
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 |
ChiahsinChu
force-pushed
the
devel-pt-sel_type
branch
2 times, most recently
from
17ca9af to
be6c622
Compare
![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
🧹 Nitpick comments (2)
deepmd/pt/model/atomic_model/dp_atomic_model.py (1)
52-68: Reinitializing fitting exclusions in__init__is appropriateWiring
self.atom_exclude_typesintoself.fitting_net.reinit_exclude(...)at construction time is the right place to fix the PTget_sel_typebehavior and is consistent with the existingreinit_excludeAPI on the fitting side. This should correctly initialize the internal exclusion mask for both training and inference and is safe whenatom_exclude_typesis empty.You may also want to consider re-calling
reinit_exclude(self.atom_exclude_types)afterchange_type_map()ifntypeschanges but the same excluded indices are expected to remain meaningful.deepmd/tf/model/model.py (1)
1002-1015: Atom-exclusion →sel_typemapping in TF deserialize looks correct; consider normalizing typeMapping
atom_exclude_typestosel_typevia the complement overtype_mapmatches the existing pattern indeepmd.tf.fit.dipole.deserializeand should give TF parity with PT for selected types.Two small robustness / consistency tweaks to consider:
- Normalize
sel_typeto a plain Python list before passing it intoFitting.deserialize, to match other call sites and avoid any downstream assumptions about the type:- sel_type = np.setdiff1d( - full_type_list, atom_exclude_types, assume_unique=True - ) - fitting_dict["sel_type"] = sel_type + sel_type = np.setdiff1d( + full_type_list, atom_exclude_types, assume_unique=True + ) + fitting_dict["sel_type"] = sel_type.tolist()
- If you are not relying on the small speed gain, you could also drop
assume_unique=True(as in this code it is not performance‑critical) to be maximally safe against any accidental duplicates inatom_exclude_types.Please re-run the TF backend model deserialization tests (including any TF↔PT conversion tests) to confirm that
Fitting.deserializeis happy withsel_typewhen provided as a Python list rather than a NumPy array.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deepmd/pt/model/atomic_model/dp_atomic_model.py(1 hunks)deepmd/tf/model/model.py(1 hunks)source/tests/pt/model/test_get_model.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
deepmd/tf/model/model.py (3)
source/tests/consistent/model/test_ener.py (2)
data(82-112)data(307-337)source/api_cc/src/common.cc (1)
sel_type(80-80)deepmd/tf/fit/dipole.py (1)
deserialize(470-495)
deepmd/pt/model/atomic_model/dp_atomic_model.py (3)
deepmd/pd/model/task/fitting.py (1)
reinit_exclude(360-365)deepmd/pt/model/task/fitting.py (1)
reinit_exclude(550-555)deepmd/dpmodel/fitting/general_fitting.py (1)
reinit_exclude(389-394)
source/tests/pt/model/test_get_model.py (1)
deepmd/pt/model/atomic_model/dp_atomic_model.py (1)
get_sel_type(387-394)
⏰ 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). (27)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Test Python (5, 3.10)
- GitHub Check: Test Python (2, 3.13)
- GitHub Check: Test Python (1, 3.13)
- GitHub Check: Test Python (3, 3.13)
- GitHub Check: Test Python (4, 3.13)
- GitHub Check: Test Python (5, 3.13)
- GitHub Check: Test Python (6, 3.13)
- GitHub Check: Test Python (6, 3.10)
- GitHub Check: Test Python (4, 3.10)
- GitHub Check: Test Python (2, 3.10)
- GitHub Check: Test Python (1, 3.10)
- GitHub Check: Test Python (3, 3.10)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test C++ (true, false, false, true)
- GitHub Check: Test C++ (false, true, true, false)
- GitHub Check: Test C++ (true, true, true, false)
- GitHub Check: Test C++ (false, false, false, true)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/tf/model/model.py (1)
1006-1015: TF deserialization: sel_type derivation from atom_exclude_types looks correct; consider minor robustness tweaksThis block correctly enables PT→TF conversion with
atom_exclude_typesby computingsel_typeas the complement overtype_mapand feeding it intoFitting.deserialize, which aligns with the intended behavior.Two small, non-blocking robustness points:
np.setdiff1d(..., assume_unique=True)assumesatom_exclude_typesis unique. If a user ever passes duplicates, behavior can be surprising; droppingassume_unique=Truekeeps semantics correct at negligible cost.- When
atom_exclude_typesis non-empty, this path requiresdata["type_map"]to exist. If any legacy models could haveatom_exclude_typeswithouttype_map, it may be worth guarding with a clearer error (or falling back) instead of a bareKeyError.If these situations are impossible in your supported formats, the current implementation is fine as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
deepmd/pt/model/atomic_model/dp_atomic_model.py(2 hunks)deepmd/tf/model/model.py(1 hunks)source/tests/pt/model/test_get_model.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- source/tests/pt/model/test_get_model.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-16T21:49:57.401Z
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4226
File: deepmd/dpmodel/atomic_model/base_atomic_model.py:202-202
Timestamp: 2024-10-16T21:49:57.401Z
Learning: When reviewing PRs, avoid making refactor suggestions that are not directly related to the PR's changes. For example, in `deepmd/dpmodel/atomic_model/base_atomic_model.py`, do not suggest simplifying `for kk in ret_dict.keys()` to `for kk in ret_dict` unless it's relevant to the PR.
Applied to files:
deepmd/pt/model/atomic_model/dp_atomic_model.py
🧬 Code graph analysis (2)
deepmd/tf/model/model.py (3)
deepmd/tf/fit/fitting.py (2)
Fitting(32-266)deserialize(86-108)deepmd/tf/fit/dipole.py (1)
deserialize(470-495)deepmd/tf/fit/polar.py (1)
deserialize(691-714)
deepmd/pt/model/atomic_model/dp_atomic_model.py (1)
deepmd/pt/model/task/fitting.py (1)
reinit_exclude(550-555)
⏰ 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). (16)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (python)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Test C++ (false, false, false, true)
- GitHub Check: Test C++ (true, false, false, true)
- GitHub Check: Test C++ (true, true, true, false)
- GitHub Check: Test C++ (false, true, true, false)
🔇 Additional comments (2)
deepmd/pt/model/atomic_model/dp_atomic_model.py (2)
67-67: Hooking atom_exclude_types into fitting_net on initializationCalling
self.fitting_net.reinit_exclude(self.atom_exclude_types)right after assigningself.fitting_netensures the fitting’s internal exclusion mask (and thusget_sel_type()) is consistent with the atomic model’satom_exclude_typesfrom the moment the model is constructed. This directly addresses the reported PT-trainedget_sel_typediscrepancy and looks correct given the existingreinit_excludeAPI.
155-156: Refreshing fitting exclusion after type_map changesReinvoking
self.fitting_net.reinit_exclude(self.atom_exclude_types)immediately afterchange_type_map()is important to rebuild the exclusion mask with the updated number and ordering of types, soget_sel_type()remains consistent after type remapping. This is a clean, localized fix with no obvious side effects.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #5097 +/- ##
=======================================
Coverage 82.14% 82.14%
=======================================
Files 709 709
Lines 72458 72467 +9
Branches 3615 3615
=======================================
+ Hits 59520 59531 +11
+ Misses 11776 11774 -2
Partials 1162 1162 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
source/tests/consistent/model/test_polar.py (1)
202-208: LGTM! Test correctly verifies cross-backend consistency.The test appropriately validates that TF and PT backends compute the same
sel_typeafter deserialization withatom_exclude_types. This directly addresses the PR objective of ensuring PyTorch models respect atom exclusions.Consider adding an explicit assertion for the expected value to make the test more robust:
def test_atom_exclude_types(self): _ret, data = self.get_reference_ret_serialization(self.RefBackend.PT) data["atom_exclude_types"] = [1] self.reset_unique_id() tf_obj = self.tf_class.deserialize(data, suffix=self.unique_id) pt_obj = self.pt_class.deserialize(data) + # With type_map ["O", "H"] and excluding type 1, sel_type should be [0] + self.assertEqual(pt_obj.get_sel_type(), [0]) self.assertEqual(tf_obj.get_sel_type(), pt_obj.get_sel_type())source/tests/consistent/model/test_dipole.py (1)
208-214: LGTM! Test correctly verifies cross-backend consistency.The test appropriately validates that TF and PT backends compute the same
sel_typeafter deserialization withatom_exclude_types. This ensures the dipole model correctly respects atom exclusions.Consider adding an explicit assertion for the expected value to make the test more robust:
def test_atom_exclude_types(self): _ret, data = self.get_reference_ret_serialization(self.RefBackend.PT) data["atom_exclude_types"] = [1] self.reset_unique_id() tf_obj = self.tf_class.deserialize(data, suffix=self.unique_id) pt_obj = self.pt_class.deserialize(data) + # With type_map ["O", "H"] and excluding type 1, sel_type should be [0] + self.assertEqual(pt_obj.get_sel_type(), [0]) self.assertEqual(tf_obj.get_sel_type(), pt_obj.get_sel_type())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
source/tests/consistent/model/test_dipole.py(1 hunks)source/tests/consistent/model/test_polar.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
source/tests/consistent/model/test_polar.py (1)
deepmd/tf/fit/polar.py (3)
deserialize(691-714)get_sel_type(207-209)get_sel_type(776-778)
⏰ 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). (28)
- GitHub Check: Test C++ (true, true, true, false)
- GitHub Check: Test C++ (false, false, false, true)
- GitHub Check: Test C++ (false, true, true, false)
- GitHub Check: Test C++ (true, false, false, true)
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Test Python (1, 3.13)
- GitHub Check: Test Python (1, 3.10)
- GitHub Check: Test Python (4, 3.13)
- GitHub Check: Test Python (3, 3.13)
- GitHub Check: Test Python (4, 3.10)
- GitHub Check: Test Python (5, 3.10)
- GitHub Check: Test Python (6, 3.10)
- GitHub Check: Test Python (5, 3.13)
- GitHub Check: Test Python (2, 3.13)
- GitHub Check: Test Python (3, 3.10)
- GitHub Check: Test Python (2, 3.10)
- GitHub Check: Test Python (6, 3.13)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (python)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (rocm, rocm)
ChiahsinChu
force-pushed
the
devel-pt-sel_type
branch
from
77b9503 to
b693ae0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
deepmd/pt/model/atomic_model/dp_atomic_model.py (1)
156-157: Inconsistency: Missinghasattrcheck for consistency.Line 67 defensively checks
hasattr(self.fitting_net, "reinit_exclude")before calling the method, but line 157 calls it directly without the check. All fitting types in the inheritance chain (Fitting, GeneralFitting, InvarFitting, and their subclasses) implementreinit_exclude, so the code is safe. However, add the same defensive check here for consistency with the__init__pattern:if hasattr(self.fitting_net, "reinit_exclude"): self.fitting_net.reinit_exclude(self.atom_exclude_types)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
deepmd/pt/model/atomic_model/dp_atomic_model.py(2 hunks)deepmd/tf/model/model.py(1 hunks)source/tests/consistent/model/test_dipole.py(1 hunks)source/tests/consistent/model/test_polar.py(1 hunks)source/tests/pt/model/test_get_model.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- deepmd/tf/model/model.py
- source/tests/pt/model/test_get_model.py
- source/tests/consistent/model/test_polar.py
- source/tests/consistent/model/test_dipole.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-16T21:49:57.401Z
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4226
File: deepmd/dpmodel/atomic_model/base_atomic_model.py:202-202
Timestamp: 2024-10-16T21:49:57.401Z
Learning: When reviewing PRs, avoid making refactor suggestions that are not directly related to the PR's changes. For example, in `deepmd/dpmodel/atomic_model/base_atomic_model.py`, do not suggest simplifying `for kk in ret_dict.keys()` to `for kk in ret_dict` unless it's relevant to the PR.
Applied to files:
deepmd/pt/model/atomic_model/dp_atomic_model.py
🧬 Code graph analysis (1)
deepmd/pt/model/atomic_model/dp_atomic_model.py (3)
deepmd/pd/model/task/fitting.py (1)
reinit_exclude(360-365)deepmd/pt/model/task/fitting.py (1)
reinit_exclude(550-555)deepmd/dpmodel/fitting/general_fitting.py (1)
reinit_exclude(389-394)
⏰ 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). (28)
- GitHub Check: Test Python (4, 3.13)
- GitHub Check: Test Python (5, 3.10)
- GitHub Check: Test Python (6, 3.13)
- GitHub Check: Test Python (6, 3.10)
- GitHub Check: Test Python (5, 3.13)
- GitHub Check: Test Python (1, 3.13)
- GitHub Check: Test Python (2, 3.10)
- GitHub Check: Test Python (4, 3.10)
- GitHub Check: Test Python (3, 3.13)
- GitHub Check: Test Python (2, 3.13)
- GitHub Check: Test Python (1, 3.10)
- GitHub Check: Test Python (3, 3.10)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Build wheels for cp311-macosx_x86_64
- 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: Test C++ (true, false, false, true)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (python)
- GitHub Check: Test C++ (true, true, true, false)
- GitHub Check: Test C++ (false, false, false, true)
- GitHub Check: Test C++ (false, true, true, false)
🔇 Additional comments (1)
deepmd/pt/model/atomic_model/dp_atomic_model.py (1)
67-68: Core fix correctly initializes exclusion state.The conditional call to
reinit_excludeensures the fitting network's exclusion mask reflectsatom_exclude_typesfrom initialization, fixing theget_sel_typediscrepancy described in issue #5096.
ChiahsinChu
force-pushed
the
devel-pt-sel_type
branch
from
2e4e11c to
2b15f10
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
source/tests/universal/dpmodel/atomc_model/test_atomic_model.py (1)
77-81: Utility function duplicates logic from production code.The function correctly computes the complement of
atom_exclude_types, but this logic is duplicated indeepmd/tf/model/model.py(lines 1010-1012). Consider extracting this to a shared utility module to reduce duplication and maintenance burden.Would you like me to generate a refactoring that extracts this to a shared utility module, such as
deepmd/utils/atom_exclude.py?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
deepmd/dpmodel/atomic_model/dp_atomic_model.py(2 hunks)deepmd/pt/model/atomic_model/dp_atomic_model.py(2 hunks)deepmd/tf/model/model.py(1 hunks)source/tests/consistent/model/test_dipole.py(1 hunks)source/tests/consistent/model/test_polar.py(1 hunks)source/tests/pt/model/test_get_model.py(1 hunks)source/tests/universal/dpmodel/atomc_model/test_atomic_model.py(17 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- source/tests/pt/model/test_get_model.py
- deepmd/pt/model/atomic_model/dp_atomic_model.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-16T21:49:57.401Z
Learnt from: njzjz
Repo: deepmodeling/deepmd-kit PR: 4226
File: deepmd/dpmodel/atomic_model/base_atomic_model.py:202-202
Timestamp: 2024-10-16T21:49:57.401Z
Learning: When reviewing PRs, avoid making refactor suggestions that are not directly related to the PR's changes. For example, in `deepmd/dpmodel/atomic_model/base_atomic_model.py`, do not suggest simplifying `for kk in ret_dict.keys()` to `for kk in ret_dict` unless it's relevant to the PR.
Applied to files:
deepmd/dpmodel/atomic_model/dp_atomic_model.py
🧬 Code graph analysis (5)
source/tests/consistent/model/test_dipole.py (4)
source/tests/consistent/model/test_polar.py (3)
test_atom_exclude_types(202-212)skip_tf(92-93)data(46-69)deepmd/tf/fit/dipole.py (2)
deserialize(470-495)get_sel_type(160-162)deepmd/tf/fit/polar.py (3)
deserialize(691-714)get_sel_type(207-209)get_sel_type(776-778)deepmd/tf/model/tensor.py (1)
get_sel_type(81-82)
deepmd/dpmodel/atomic_model/dp_atomic_model.py (7)
deepmd/dpmodel/fitting/general_fitting.py (2)
reinit_exclude(389-394)change_type_map(327-347)deepmd/dpmodel/descriptor/se_t.py (1)
reinit_exclude(310-315)deepmd/pt/model/model/make_model.py (1)
change_type_map(503-514)deepmd/pt/model/atomic_model/base_atomic_model.py (1)
change_type_map(303-334)deepmd/dpmodel/atomic_model/base_atomic_model.py (1)
change_type_map(134-149)deepmd/dpmodel/atomic_model/linear_atomic_model.py (1)
change_type_map(115-130)deepmd/dpmodel/descriptor/make_base_descriptor.py (1)
change_type_map(131-137)
deepmd/tf/model/model.py (3)
source/tests/consistent/fitting/test_dipole.py (1)
data(68-82)deepmd/tf/fit/fitting.py (2)
Fitting(32-266)deserialize(86-108)deepmd/tf/fit/dipole.py (1)
deserialize(470-495)
source/tests/consistent/model/test_polar.py (2)
deepmd/tf/fit/polar.py (2)
get_sel_type(207-209)get_sel_type(776-778)deepmd/tf/model/tensor.py (1)
get_sel_type(81-82)
source/tests/universal/dpmodel/atomc_model/test_atomic_model.py (2)
deepmd/dpmodel/atomic_model/dp_atomic_model.py (1)
get_sel_type(243-250)deepmd/dpmodel/atomic_model/linear_atomic_model.py (1)
get_sel_type(353-363)
⏰ 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). (28)
- GitHub Check: Build wheels for cp311-macosx_x86_64
- GitHub Check: Build wheels for cp311-macosx_arm64
- GitHub Check: Build wheels for cp311-win_amd64
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (5, 3.10)
- GitHub Check: Test Python (5, 3.13)
- GitHub Check: Test Python (4, 3.13)
- GitHub Check: Test Python (2, 3.10)
- GitHub Check: Test Python (6, 3.13)
- GitHub Check: Test Python (1, 3.13)
- GitHub Check: Test Python (3, 3.13)
- GitHub Check: Test Python (4, 3.10)
- GitHub Check: Test Python (3, 3.10)
- GitHub Check: Test Python (6, 3.10)
- GitHub Check: Test Python (1, 3.10)
- GitHub Check: Test Python (2, 3.13)
- GitHub Check: Build C++ (cpu, cpu)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Test C++ (false, true, true, false)
- GitHub Check: Test C++ (false, false, false, true)
- GitHub Check: Test C++ (true, false, false, true)
- GitHub Check: Test C++ (true, true, true, false)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Analyze (python)
🔇 Additional comments (6)
deepmd/dpmodel/atomic_model/dp_atomic_model.py (2)
54-55: LGTM! Correctly initializes fitting exclusion state.The conditional call to
self.fitting.reinit_exclude(self.atom_exclude_types)ensures that the fitting's exclusion settings are properly initialized duringDPAtomicModelconstruction, which is essential forget_sel_type()to return the correct selected atom types.
196-196: Bug fix: Use correct attribute name.Changed from
self.fitting_net.change_type_map(type_map=type_map)toself.fitting.change_type_map(type_map=type_map). This corrects the attribute name to match the actual assignment at line 53 (self.fitting = fitting).source/tests/consistent/model/test_polar.py (1)
202-212: LGTM! Cross-backend consistency test for atom_exclude_types.The test correctly validates that
atom_exclude_typesis handled consistently between TF and PT backends by comparingget_sel_type()results after deserialization. The skip checks appropriately handle unavailable backends.source/tests/consistent/model/test_dipole.py (1)
208-218: LGTM! Cross-backend consistency test for atom_exclude_types.The test validates that
atom_exclude_typesis handled consistently between TF and PT backends for dipole models. The implementation properly skips when backends are unavailable, addressing the past review comment.source/tests/universal/dpmodel/atomc_model/test_atomic_model.py (1)
97-97: LGTM! Comprehensive test coverage for atom_exclude_types.The parameterization includes both empty (
[]) and non-empty ([0])atom_exclude_typesvalues, and the test methodtest_sel_type_from_atom_exclude_typesvalidates that the utility function produces the expectedsel_type. This pattern is consistently applied across all test classes.Also applies to: 110-110, 142-143, 150-156
deepmd/tf/model/model.py (1)
1006-1015: Fix: Code pattern does NOT match the reference implementation in dipole.py and assumes uniqueness without validation.The implementation uses
np.setdiff1d(..., assume_unique=True)which differs from the reference pattern indeepmd/tf/fit/dipole.py(lines 469-494), which uses a list comprehension:[ii for ii in range(data["ntypes"]) if ii not in exclude_types].The
assume_unique=Trueparameter has undefined behavior ifatom_exclude_typescontains duplicates, per NumPy documentation. No validation exists in the codebase to ensure uniqueness. Consider either:
- Adding validation to check for duplicates in
atom_exclude_types, or- Using the safer list comprehension approach from dipole.py that doesn't require the uniqueness assumption
⛔ Skipped due to learnings
Learnt from: njzjz Repo: deepmodeling/deepmd-kit PR: 4226 File: deepmd/dpmodel/atomic_model/base_atomic_model.py:202-202 Timestamp: 2024-10-16T21:49:57.401Z Learning: When reviewing PRs, avoid making refactor suggestions that are not directly related to the PR's changes. For example, in `deepmd/dpmodel/atomic_model/base_atomic_model.py`, do not suggest simplifying `for kk in ret_dict.keys()` to `for kk in ret_dict` unless it's relevant to the PR.
1. Allow backend-convert from pt to tf with `atom_exclude_types` 2. Fix bug in `get_sel_type` method of pt model (by calling `fitting.reinit_exclude` in atomic model init). Fix #5096 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fitting now reinitializes exclusion/selection settings during model init and after type-map changes, and deserialization injects selection info when atom exclusions exist, ensuring correct selection behavior. * **Tests** * Added unit and cross-backend tests to verify atom-exclude → selection computation and parity between backends; expanded test coverage for exclusion scenarios. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ChiahsinChu
force-pushed
the
devel-pt-sel_type
branch
from
2b15f10 to
d9f1b6d
Compare
3 participants
atom_exclude_typesget_sel_typemethod of pt model (by callingfitting.reinit_excludein atomic model init).Fix #5096
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.