Skip to content

test(pt): document se_e3_tebd gr output#5536

Open
njzjz-bot wants to merge 1 commit into
deepmodeling:masterfrom
njzjz-bot:fix-5463-se-e3-tebd-gr-none
Open

test(pt): document se_e3_tebd gr output#5536
njzjz-bot wants to merge 1 commit into
deepmodeling:masterfrom
njzjz-bot:fix-5463-se-e3-tebd-gr-none

Conversation

@njzjz-bot

@njzjz-bot njzjz-bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Problem

  • The pt_expt se_e3_tebd consistency test had a TODO asking why gr is None.
  • The descriptor docs implied a non-None equivariant gr, even though se_e3_tebd contracts the angular information into an invariant descriptor.

Change

  • Assert that both backends return gr is None in the test.
  • Replace the TODO with a code comment explaining the invariant three-body contraction.
  • Update the dpmodel return-value docs to state that gr is intentionally None.

Notes

  • Fixes investigate why gr is None #5463.
  • Verification: python3 -m py_compile source/tests/pt_expt/descriptor/test_se_t_tebd.py deepmd/dpmodel/descriptor/se_t_tebd.py; git diff --check.
  • Full pytest was not run locally because this environment does not have pytest installed.

Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)

Summary by CodeRabbit

  • Documentation

    • Clarified descriptor output documentation: single-particle equivariant output returns None, and three-body angular information is contracted into rotationally invariant descriptor format rather than exposed separately.
  • Tests

    • Updated tests with explicit assertions for None outputs and improved comparison logic by removing conditional branches.

The se_e3_tebd descriptor contracts three-body angular information into a rotationally invariant descriptor, so it does not expose a separate equivariant gr representation. Make the test assert this behavior explicitly and update the return-value docs.

Authored by OpenClaw (model: custom-chat-jinzhezeng-group/gpt-5.5)
@dosubot dosubot Bot added the Docs label Jun 14, 2026
@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The DescrptSeTTebd.call method docstring is updated to state that this descriptor returns None for the equivariant single-particle output. The corresponding test removes the conditional gr comparison block and instead explicitly asserts that gr1 and gr2 are None.

Changes

Clarify and enforce gr=None for se_t_tebd descriptor

Layer / File(s) Summary
Docstring and test assertion for gr=None
deepmd/dpmodel/descriptor/se_t_tebd.py, source/tests/pt_expt/descriptor/test_se_t_tebd.py
DescrptSeTTebd.call docstring now explicitly states the equivariant single-particle output is None; the test replaces the prior conditional gr comparison (including the TODO comment) with direct assertIsNone checks for gr1 and gr2, followed by the rd1/rd2 equality check.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'test(pt): document se_e3_tebd gr output' accurately describes the main change of documenting and testing the gr output behavior of the se_e3_tebd descriptor.
Linked Issues check ✅ Passed The pull request successfully addresses issue #5463 by documenting why gr is None through updated test assertions and docstring clarifications in the descriptor implementation.
Out of Scope Changes check ✅ Passed All changes are directly scoped to addressing issue #5463: updating test assertions for gr output and clarifying docstrings in the se_t_tebd descriptor file.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
deepmd/dpmodel/descriptor/se_t_tebd.py (1)

363-363: ⚠️ Potential issue | 🟠 Major

Fix return type annotation to match actual 5-tuple return.

The return type annotation declares -> tuple[Array, Array] (2-tuple) at line 363, but the function returns a 5-tuple (grrg, rot_mat, None, None, sw) at line 420. The docstring correctly documents all 5 return values. Update the annotation to tuple[Array, Array, None, None, Array] or use an appropriate type alias to match the implementation.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@deepmd/dpmodel/descriptor/se_t_tebd.py` at line 363, The return type
annotation for the function declares a 2-tuple type `tuple[Array, Array]` but
the actual return statement at line 420 returns a 5-tuple containing `(grrg,
rot_mat, None, None, sw)`. Update the return type annotation to accurately
reflect the 5-tuple being returned, changing it to `tuple[Array, Array, None,
None, Array]` to match both the implementation and what is documented in the
docstring.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@deepmd/dpmodel/descriptor/se_t_tebd.py`:
- Line 363: The return type annotation for the function declares a 2-tuple type
`tuple[Array, Array]` but the actual return statement at line 420 returns a
5-tuple containing `(grrg, rot_mat, None, None, sw)`. Update the return type
annotation to accurately reflect the 5-tuple being returned, changing it to
`tuple[Array, Array, None, None, Array]` to match both the implementation and
what is documented in the docstring.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d680dfba-0862-429e-a859-2de54d5a1f78

📥 Commits

Reviewing files that changed from the base of the PR and between 87d8557 and 8abfe96.

📒 Files selected for processing (2)
  • deepmd/dpmodel/descriptor/se_t_tebd.py
  • source/tests/pt_expt/descriptor/test_se_t_tebd.py

@codecov

codecov Bot commented Jun 14, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.18%. Comparing base (016141f) to head (8abfe96).
⚠️ Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5536      +/-   ##
==========================================
+ Coverage   81.34%   82.18%   +0.84%     
==========================================
  Files         868      890      +22     
  Lines       96358   101357    +4999     
  Branches     4233     4243      +10     
==========================================
+ Hits        78383    83304    +4921     
- Misses      16675    16751      +76     
- Partials     1300     1302       +2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@njzjz njzjz requested a review from wanghan-iapcm June 15, 2026 04:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

investigate why gr is None

1 participant