fix(dpmodel): correct type hints for descriptor return types and parameter types#5251
fix(dpmodel): correct type hints for descriptor return types and parameter types#5251njzjz-bot wants to merge 1 commit intodeepmodeling:masterfrom
Conversation
|
Related Documentation Checked 0 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
This PR contains only type hint corrections extracted from PR njzjz#222: - Return type corrections (NoReturn -> None, tuple[Array, Array] -> tuple[Array, Array, Array, Array, Array]) - Parameter type corrections (e_sel: int -> e_sel: int | list[int]) - Parameter name changes to match interface (extended_coord -> coord_ext) Excludes: - # type: ignore comments - assert statements - Variable renames (e_sel -> e_sel_list) - Any logic changes -- OpenClaw
njzjz-bot
force-pushed
the
type-hint-only
branch
from
5409aff to
e7bb15b
Compare
📝 WalkthroughWalkthroughThis PR updates public method signatures across descriptor implementations and atomic models. Return types are expanded from 2-tuples to 5-tuples in descriptor classes (dpa1, dpa2, dpa3, repflows), parameter names are standardized in the DescriptorBlock base class, parameter types are broadened for e_sel selection options, and an internal method's return structure in LinearEnergyAtomicModel is restructured. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
deepmd/dpmodel/descriptor/dpa1.py (1)
488-494:⚠️ Potential issue | 🟡 Minor
DescrptDPA1.callreturn annotation-> Arraynot corrected — gap in type hint coverage.
DescrptBlockSeAtten.callwas updated (Line 967) but the outerDescrptDPA1.callwrapper still annotates-> Array. The actual return at Line 549 is a 5-tuple(grrg, rot_mat, None, None, sw).🔧 Suggested fix
- ) -> Array: + ) -> tuple[Array, Array, Array | None, Array | None, Array]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/dpa1.py` around lines 488 - 494, DescrptDPA1.call currently has the wrong return type annotation (-> Array) while it actually returns a 5-tuple produced by DescrptBlockSeAtten.call (grrg, rot_mat, None, None, sw); update the type hint on the DescrptDPA1.call signature to reflect the actual return type (e.g., -> tuple[Array, Array, None | ???, None | ???, Array] or the appropriate typing.Tuple[...] using the project's Array alias and None types) so callers and linters see the correct shape; change the annotation on the def call(...) of class/function DescrptDPA1.call and ensure any import of typing.Tuple or union syntax is added if needed.deepmd/dpmodel/descriptor/dpa2.py (1)
678-684:⚠️ Potential issue | 🟡 Minor
share_paramsstill declares-> NoReturn— inconsistent with the base class update.
DescriptorBlock.share_paramswas corrected to-> Noneindescriptor.py(this PR), but this override was not updated. The PR explicitly targetsNoReturn → Nonecorrections.🔧 Suggested fix
- ) -> NoReturn: + ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@deepmd/dpmodel/descriptor/dpa2.py` around lines 678 - 684, The override share_params in class/method named share_params (in dpa2.py) still declares -> NoReturn; change its type annotation to -> None to match the base class update (DescriptorBlock.share_params) and ensure the signature exactly matches the base method so the override is consistent; update any related docstring if needed to reflect returning None.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@deepmd/dpmodel/descriptor/repflows.py`:
- Around line 487-488: The parameter type_embedding in the function with
signature ending in ") -> tuple[Array, Array, Array, Array, Array]:" is unused
and triggers ARG002; either consume it (follow the convention used by
DescrptBlockSeAtten by integrating type_embedding into the computation path of
this DescriptorBlock.call implementation) or explicitly suppress the linter and
document the reason: add a brief docstring line explaining it exists for
DescriptorBlock.call interface compatibility and append a local suppression
(e.g., "# noqa: ARG002") to the parameter or function definition so Ruff no
longer flags it.
---
Outside diff comments:
In `@deepmd/dpmodel/descriptor/dpa1.py`:
- Around line 488-494: DescrptDPA1.call currently has the wrong return type
annotation (-> Array) while it actually returns a 5-tuple produced by
DescrptBlockSeAtten.call (grrg, rot_mat, None, None, sw); update the type hint
on the DescrptDPA1.call signature to reflect the actual return type (e.g., ->
tuple[Array, Array, None | ???, None | ???, Array] or the appropriate
typing.Tuple[...] using the project's Array alias and None types) so callers and
linters see the correct shape; change the annotation on the def call(...) of
class/function DescrptDPA1.call and ensure any import of typing.Tuple or union
syntax is added if needed.
In `@deepmd/dpmodel/descriptor/dpa2.py`:
- Around line 678-684: The override share_params in class/method named
share_params (in dpa2.py) still declares -> NoReturn; change its type annotation
to -> None to match the base class update (DescriptorBlock.share_params) and
ensure the signature exactly matches the base method so the override is
consistent; update any related docstring if needed to reflect returning None.
| type_embedding: Array | None = None, | ||
| ) -> tuple[Array, Array, Array, Array, Array]: |
There was a problem hiding this comment.
Unused type_embedding parameter triggers ARG002 — consider suppressing or documenting.
The parameter is added for interface compatibility with DescriptorBlock.call but is never consumed in the method body (confirmed by the static analysis warning on Line 487). This will cause persistent Ruff failures unless suppressed.
🔧 Options to address
Either add a docstring note and suppress:
+ type_embedding: Array | None = None, # noqa: ARG002 – unused, required by DescriptorBlock interfaceOr accept the convention used by DescrptBlockSeAtten, which does use type_embedding.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type_embedding: Array | None = None, | |
| ) -> tuple[Array, Array, Array, Array, Array]: | |
| type_embedding: Array | None = None, # noqa: ARG002 – unused, required by DescriptorBlock interface | |
| ) -> tuple[Array, Array, Array, Array, Array]: |
🧰 Tools
🪛 Ruff (0.15.1)
[warning] 487-487: Unused method argument: type_embedding
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@deepmd/dpmodel/descriptor/repflows.py` around lines 487 - 488, The parameter
type_embedding in the function with signature ending in ") -> tuple[Array,
Array, Array, Array, Array]:" is unused and triggers ARG002; either consume it
(follow the convention used by DescrptBlockSeAtten by integrating type_embedding
into the computation path of this DescriptorBlock.call implementation) or
explicitly suppress the linter and document the reason: add a brief docstring
line explaining it exists for DescriptorBlock.call interface compatibility and
append a local suppression (e.g., "# noqa: ARG002") to the parameter or function
definition so Ruff no longer flags it.
There was a problem hiding this comment.
Pull request overview
This PR corrects type hints across descriptor and atomic model classes in the dpmodel package. The changes ensure type annotations accurately reflect the actual function signatures and return types, improving type checking and code documentation without modifying any logic.
Changes:
- Corrected return type annotations from
tuple[Array, Array]totuple[Array, Array, Array, Array, Array]for descriptorcall()methods that return 5 arrays - Changed
NoReturntoNonefor methods that don't return values and removed unused import - Updated parameter type for
e_selfrominttoint | list[int]to reflect actual usage - Renamed parameters in base class from
extended_coord,extended_atype,extended_atype_embdtocoord_ext,atype_ext,atype_embd_extto match implementations - Fixed incorrect return type in
linear_atomic_model.pyfromtuple[tuple[Array, Array], list[int]]totuple[list[float], list[int]]
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| deepmd/dpmodel/descriptor/repflows.py | Corrected return type to 5-tuple for DescrptBlockRepflows.call(), added missing type_embedding parameter, corrected e_sel parameter type to accept int or list, and fixed RepFlowLayer.call() return type to 3-tuple |
| deepmd/dpmodel/descriptor/dpa3.py | Corrected return type to 5-tuple for call() method |
| deepmd/dpmodel/descriptor/dpa2.py | Corrected return type to 5-tuple for call() method |
| deepmd/dpmodel/descriptor/dpa1.py | Corrected return type to 5-tuple for DescrptBlockSeAtten.call() method |
| deepmd/dpmodel/descriptor/descriptor.py | Changed NoReturn to None for compute_input_stats() and share_params(), removed unused NoReturn import, renamed parameters to match implementations |
| deepmd/dpmodel/atomic_model/linear_atomic_model.py | Corrected return type from tuple[tuple[Array, Array], list[int]] to tuple[list[float], list[int]] for _sort_rcuts_sels() method |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5251 +/- ##
=======================================
Coverage 82.16% 82.16%
=======================================
Files 745 745
Lines 74825 74825
Branches 3615 3615
=======================================
Hits 61482 61482
Misses 12180 12180
Partials 1163 1163 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 participants
This PR contains only type hint corrections extracted from PR njzjz#222:
Excludes:
type: ignore comments
Summary by CodeRabbit