Skip to content

Conversation

@njzjz
Copy link
Member

@njzjz njzjz commented Dec 15, 2025

case_embd was supported but the JAX backend was not touched.

Summary by CodeRabbit

  • Bug Fixes

    • Ensured the case_embd parameter is consistently converted and handled during fitting, improving compatibility across array backends and preventing mis-coercion.
  • Tests

    • Adjusted test setup to reset the default computation graph before enabling eager execution, stabilizing related test runs.

✏️ Tip: You can customize this high-level summary in your review settings.

`case_embd` was supported but the JAX backend was not touched.
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 15, 2025

📝 Walkthrough

Walkthrough

Added "case_embd" to fitting attribute conversion logic for JAX and strict Array API paths, and updated a TensorFlow test module to reset the default graph before enabling eager execution.

Changes

Cohort / File(s) Change Summary
JAX fitting update
deepmd/jax/fitting/fitting.py
Added "case_embd" to the set of parameter names routed through to_jax_array and wrapped/converted via the existing general-fitting setter logic.
Array-API strict fitting tests
source/tests/array_api_strict/fitting/fitting.py
Added "case_embd" to the set of names passed to to_array_api_strict_array within the general fitting attribute setter in tests.
TensorFlow test setup
source/tests/pt/test_tabulate.py
Inserted tf.reset_default_graph() at the start of setUpModule() before enabling eager execution.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Check that case_embd is a valid fitting parameter for the affected modules.
  • Verify conversions behave correctly for None and non-None values across supported Flax/array-API versions.
  • Confirm the TF reset call is safe in the test environment and doesn't interfere with other tests.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly relates to the main change: adding case_embd support to the JAX backend's setattr logic.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 31f8647 and d4277dd.

📒 Files selected for processing (1)
  • source/tests/pt/test_tabulate.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • source/tests/pt/test_tabulate.py

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.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes a missing attribute handler for case_embd in the JAX backend. The case_embd attribute was already supported in PyTorch and Paddle backends but was not being properly handled in the JAX backend's setattr_for_general_fitting function.

  • Added case_embd to the set of attributes that are converted to JAX arrays in the fitting module

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov
Copy link

codecov bot commented Dec 15, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.14%. Comparing base (26013cb) to head (d4277dd).
⚠️ Report is 4 commits behind head on devel.

Additional details and impacted files
@@           Coverage Diff           @@
##            devel    #5104   +/-   ##
=======================================
  Coverage   82.14%   82.14%           
=======================================
  Files         709      709           
  Lines       72458    72468   +10     
  Branches     3615     3615           
=======================================
+ Hits        59520    59530   +10     
+ Misses      11776    11775    -1     
- Partials     1162     1163    +1     

☔ View full report in Codecov by Sentry.
📢 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.

@iProzd iProzd added this pull request to the merge queue Dec 17, 2025
github-merge-queue bot pushed a commit that referenced this pull request Dec 17, 2025
`case_embd` was supported but the JAX backend was not touched.

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

## Release Notes

* **Bug Fixes**
* Improved parameter handling in the fitting module to properly support
the `case_embd` parameter, ensuring it receives consistent treatment and
array conversion as other fitting parameters.

<sub>✏️ Tip: You can customize this high-level summary in your review
settings.</sub>

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 17, 2025
@njzjz njzjz added this pull request to the merge queue Dec 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2025
@njzjz njzjz added this pull request to the merge queue Dec 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2025
@njzjz njzjz added this pull request to the merge queue Dec 18, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Dec 18, 2025
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@ustc.edu.cn>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@ustc.edu.cn>
@njzjz njzjz enabled auto-merge December 18, 2025 20:47
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@ustc.edu.cn>
Signed-off-by: Jinzhe Zeng <jinzhe.zeng@ustc.edu.cn>
@njzjz njzjz added this pull request to the merge queue Dec 19, 2025
Merged via the queue into deepmodeling:devel with commit 188dae3 Dec 19, 2025
58 checks passed
@njzjz njzjz deleted the jax-case_embd branch December 19, 2025 23:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants