fix(data): Throw error when data's element is not present in input.json/type_map
#4639
Conversation
📝 WalkthroughWalkthroughThe changes add a validation step in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DeepmdData.__init__
Caller->>DeepmdData.__init__: Initialize with type_map & self.type_map
DeepmdData.__init__->>DeepmdData.__init__: Check if type_map and self.type_map are non-empty
DeepmdData.__init__->>DeepmdData.__init__: Validate each element in self.type_map against provided type_map
alt Missing types found
DeepmdData.__init__->>Caller: Raise ValueError with missing types
else
DeepmdData.__init__->>Caller: Proceed with initialization
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🪧 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 (
|
for more information, see https://pre-commit.ci
|
|
![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)
source/tests/tf/test_deepmd_data.py (2)
455-458: Address unused variable warning.The variable
ddis assigned but never used, triggering a static analysis warning.- def test_init_type_map_error(self) -> None: - with self.assertRaises(ValueError): - dd = DeepmdData(self.data_name, type_map=["foo"]) + def test_init_type_map_error(self) -> None: + with self.assertRaises(ValueError): + DeepmdData(self.data_name, type_map=["foo"])🧰 Tools
🪛 Ruff (0.8.2)
457-457: Local variable
ddis assigned to but never usedRemove assignment to unused variable
dd(F841)
180-183: Address unused variable warning in TestData class.Similar to the other test, the variable
ddis assigned but never used.- def test_init_type_map_error(self) -> None: - with self.assertRaises(ValueError): - dd = DeepmdData(self.data_name, type_map=["bar"]) + def test_init_type_map_error(self) -> None: + with self.assertRaises(ValueError): + DeepmdData(self.data_name, type_map=["bar"])🧰 Tools
🪛 Ruff (0.8.2)
182-182: Local variable
ddis assigned to but never usedRemove assignment to unused variable
dd(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
deepmd/utils/data.py(1 hunks)source/tests/tf/test_deepmd_data.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
source/tests/tf/test_deepmd_data.py
182-182: Local variable dd is assigned to but never used
Remove assignment to unused variable dd
(F841)
457-457: Local variable dd is assigned to but never used
Remove assignment to unused variable dd
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Build wheels for cp310-manylinux_aarch64
- GitHub Check: Test Python (5, 3.12)
- GitHub Check: Test Python (4, 3.12)
- GitHub Check: Test Python (4, 3.9)
- GitHub Check: Test Python (3, 3.12)
- GitHub Check: Test Python (3, 3.9)
- GitHub Check: Test Python (2, 3.12)
- GitHub Check: Test Python (2, 3.9)
- GitHub Check: Test Python (1, 3.12)
- GitHub Check: Test Python (1, 3.9)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Test C++ (false)
- GitHub Check: Test C++ (true)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
🔇 Additional comments (3)
deepmd/utils/data.py (1)
92-96: Well-implemented validation check.This change adds proper validation to ensure all elements in
self.type_mapare present in the providedtype_map. The error message clearly identifies which elements are missing, helping users quickly diagnose the issue.source/tests/tf/test_deepmd_data.py (2)
180-183: Good addition of error handling test case.Adding a test case that verifies the ValueError is raised when an incomplete type map is provided ensures the new validation in DeepmdData is working as expected.
🧰 Tools
🪛 Ruff (0.8.2)
182-182: Local variable
ddis assigned to but never usedRemove assignment to unused variable
dd(F841)
385-454: Well-designed test class for mixed type testing.This new test class properly sets up test data with mixed atom types and validates the behavior of DeepmdData when working with mixed types. The setup method is thorough in creating all necessary test files and data.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
source/tests/tf/test_deepmd_data.py (1)
385-461: Well-structured test class for mixed type scenario.Good addition of the
TestDataMixTypeclass that specifically tests the behavior when handling mixed atom types. The setup method properly initializes test data including coordinates, boxes, and real atom types across multiple sets. The test methods appropriately verify both the successful initialization with a valid type map and error handling with an invalid type map.One suggestion: consider adding a test case that verifies the error message content to ensure it provides clear guidance to users.
You could enhance the error test by verifying the specific error message:
def test_init_type_map_error(self) -> None: - with self.assertRaises(ValueError): + with self.assertRaisesRegex(ValueError, "Some atom types.*not found in.*type_map"): DeepmdData(self.data_name, type_map=["foo"])
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/tests/tf/test_deepmd_data.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- 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-macosx_x86_64
- GitHub Check: Test C++ (false)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Test C++ (true)
- GitHub Check: Build wheels for cp311-manylinux_x86_64
- GitHub Check: Analyze (python)
- GitHub Check: Build C library (2.14, >=2.5.0rc0,<2.15, libdeepmd_c_cu11.tar.gz)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build C library (2.18, libdeepmd_c.tar.gz)
- GitHub Check: Build C++ (clang, clang)
- GitHub Check: Analyze (c-cpp)
- GitHub Check: Build C++ (rocm, rocm)
- GitHub Check: Build C++ (cuda120, cuda)
- GitHub Check: Build C++ (cuda, cuda)
- GitHub Check: Build C++ (cpu, cpu)
🔇 Additional comments (3)
source/tests/tf/test_deepmd_data.py (3)
180-183: Good addition of error test case.This test verifies that a
ValueErroris thrown when an invalid type map (with only one type) is provided, which helps catch configuration errors early.
446-454: Thorough verification of type mapping properties.The test properly verifies all relevant properties related to type mapping:
enforce_type_mapflag is correctly settype_mapcontents match expected valuesmixed_typeis correctly set to True- Type index mappings are correctly established
This ensures that the type mapping system works as intended.
455-458: Appropriate error test matches the existing TestData error test.This test correctly verifies that an error is thrown when an invalid type map is provided, aligning with the similar test in the TestData class.
There is another deepmd-kit/deepmd/tf/fit/ener.py Lines 1007 to 1009 in 70bc6d8 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## devel #4639 +/- ##
==========================================
- Coverage 84.78% 84.64% -0.14%
==========================================
Files 688 688
Lines 66091 66094 +3
Branches 3539 3539
==========================================
- Hits 56032 55944 -88
- Misses 8918 9013 +95
+ Partials 1141 1137 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
See issue #4519 .
When there are elements in the data that are not present in input.json, no error will be reported. For example, when element D is in type_map.raw, but D is not in type_map of input.json, the training process is normal. This error appears when
self.mixed_type=True.Summary by CodeRabbit
New Features
Tests