fix(tabulate): add c1 extrapolation outside tables#5601
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds extrapolation-aware tabulate locators and polynomial helpers, rewires CPU and GPU SE-a/r/t/t-tebd kernels to use them, and adds new GTest coverage for out-of-range and boundary behavior. ChangesTabulate linear extrapolation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@source/lib/src/tabulate.cc`:
- Around line 42-45: `locate_xx()` and the table-building logic in `tabulate.cc`
assume `upper`, `max`, and the stride values form an exact grid, but the current
`int(...)` truncation and `np.arange(...)` usage can break that invariant. Add
validation or rounding so the grid counts are only accepted when `(max - upper)
/ stride1` (and the related span calculations) are integral, and make the tail
extrapolation branch use the same segment as the `xx < max` path in
`locate_xx()` so the continuation at `max` remains C1.
In `@source/lib/tests/test_tabulate_extrapolate.cc`:
- Around line 232-268: Add GPU parity coverage for the extrapolation behavior in
the tabulation tests: the current `TabulateExtrapolate` cases only verify the
`*_cpu` paths, so a regression in the GPU implementation could slip through.
Extend the existing tests or add matching GPU assertions using the same helpers
and symbols (`se_a_value`, `se_r_value`, `se_t_value`, `se_t_tebd_value`, and
their gradient counterparts) so the tail behavior is validated against the GPU
entrypoints as well.
- Around line 248-259: The SeT extrapolation test coverage is incomplete in
`SeTUsesLinearLookupTails` and `se_t_grad` validation. Extend the existing
`TEST(TabulateExtrapolate, SeTUsesLinearLookupTails)` to include the exact lower
boundary (`kLower`/`kMin`) and add an assertion that the observable derivative
contract matches `central_diff(se_t_value, xx)` by comparing it against the sum
of `dy_dem_x` and `dy_dem`. Use the existing helpers `locate_se_t`,
`expected_table_value`, `expected_table_grad`, `se_t_value`, and `se_t_grad` so
the test exercises the split derivative behavior consistently across the
boundary and tail cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2a96cc4-d660-4022-8d1b-54f2b03c1f83
📒 Files selected for processing (3)
source/lib/src/gpu/tabulate.cusource/lib/src/tabulate.ccsource/lib/tests/test_tabulate_extrapolate.cc
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
source/lib/tests/test_tabulate_extrapolate.cc (1)
472-528: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAdd direct
*_grad_gradregression coverage.These cases only hit the forward and
*_grad_*APIs. The linked bug and this PR also change the*_grad_grad_*kernels, but nothing here invokes them, so a second-derivative regression can still pass as long asgradremains correct. Please add CPU/GPU wrappers for these_a,se_r,se_t, andse_t_tebdgrad-grad entrypoints and assert the out-of-range interpolation curvature is masked there too.🤖 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 `@source/lib/tests/test_tabulate_extrapolate.cc` around lines 472 - 528, The current tests in TabulateExtrapolate only exercise the forward and grad paths, so they miss regressions in the grad-grad kernels. Add CPU and GPU test coverage for the se_a, se_r, se_t, and se_t_tebd *_grad_grad_* entrypoints, using the same out-of-range boundary/tail cases already covered here. Reuse the existing helpers such as expect_linear_tail, expect_boundary, locate_se_a_or_r, and locate_se_t to verify the second-derivative behavior is masked correctly for both boundary and extrapolated inputs.
🤖 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 `@source/lib/tests/test_tabulate_extrapolate.cc`:
- Around line 472-528: The current tests in TabulateExtrapolate only exercise
the forward and grad paths, so they miss regressions in the grad-grad kernels.
Add CPU and GPU test coverage for the se_a, se_r, se_t, and se_t_tebd
*_grad_grad_* entrypoints, using the same out-of-range boundary/tail cases
already covered here. Reuse the existing helpers such as expect_linear_tail,
expect_boundary, locate_se_a_or_r, and locate_se_t to verify the
second-derivative behavior is masked correctly for both boundary and
extrapolated inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a06d52d0-af9f-48bc-9d4c-cb17a65bccf6
📒 Files selected for processing (3)
source/lib/src/gpu/tabulate.cusource/lib/src/tabulate.ccsource/lib/tests/test_tabulate_extrapolate.cc
🚧 Files skipped from review as they are similar to previous changes (2)
- source/lib/src/gpu/tabulate.cu
- source/lib/src/tabulate.cc
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5601 +/- ##
==========================================
+ Coverage 82.35% 82.41% +0.06%
==========================================
Files 896 903 +7
Lines 100952 101844 +892
Branches 4059 4071 +12
==========================================
+ Hits 83138 83936 +798
- Misses 16349 16439 +90
- Partials 1465 1469 +4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@source/lib/tests/test_tabulate_extrapolate.cc`:
- Around line 663-676: The current tail test only covers the CPU grad_grad path,
so add matching GPU grad_grad tail assertions in TabulateExtrapolate using the
existing se_a/se_r/se_t helpers and the *_grad_grad_gpu entrypoints. Extend
GradGradKernelsMatchDyFiniteDifferenceInTails to exercise the GPU
second-derivative kernels at the same xx tail values, mirroring the existing CPU
finite-difference checks so regressions in the GPU tabulate path are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45b8d38e-3dde-45ce-b8fb-7427fcfb51a9
📒 Files selected for processing (1)
source/lib/tests/test_tabulate_extrapolate.cc
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
source/lib/tests/test_tabulate_extrapolate.cc (2)
290-300: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a tail check for
se_agrad-grad withtwo_embed.Line 299 always passes
two_embed = nullptr, so the grad-grad attention branch withdz_dy_dtwois still untested even though the forward/grad two-embed tail path is now covered.🤖 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 `@source/lib/tests/test_tabulate_extrapolate.cc` around lines 290 - 300, The se_a grad-grad helper in test_tabulate_extrapolate.cc only exercises the non-two_embed path because se_a_grad_grad_dy always passes nullptr for two_embed, leaving the dz_dy_dtwo branch untested. Add a dedicated test case using deepmd::tabulate_fusion_se_a_grad_grad_cpu in se_a_grad_grad_dy (or a sibling helper) that supplies a real two_embed buffer and asserts the tail behavior for the attention grad-grad path, so the two_embed branch is covered alongside the existing em/em_x checks.
663-676: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftMirror these grad-grad tail checks on GPU.
This new regression test only exercises CPU grad-grad entrypoints, while the PR also rewires GPU grad-grad kernels. Add CUDA/ROCm variants so CPU/GPU parity covers the same tail derivative contract.
🤖 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 `@source/lib/tests/test_tabulate_extrapolate.cc` around lines 663 - 676, The grad-grad tail regression currently covers only the CPU entrypoints in GradGradKernelsMatchDyFiniteDifferenceInTails, so extend it to exercise the corresponding GPU kernels as well. Reuse the same tail points and finite-difference helper, but add CUDA/ROCm-specific checks for the matching grad-grad symbols so CPU and GPU parity is validated against the same derivative contract.
🤖 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.
Nitpick comments:
In `@source/lib/tests/test_tabulate_extrapolate.cc`:
- Around line 290-300: The se_a grad-grad helper in test_tabulate_extrapolate.cc
only exercises the non-two_embed path because se_a_grad_grad_dy always passes
nullptr for two_embed, leaving the dz_dy_dtwo branch untested. Add a dedicated
test case using deepmd::tabulate_fusion_se_a_grad_grad_cpu in se_a_grad_grad_dy
(or a sibling helper) that supplies a real two_embed buffer and asserts the tail
behavior for the attention grad-grad path, so the two_embed branch is covered
alongside the existing em/em_x checks.
- Around line 663-676: The grad-grad tail regression currently covers only the
CPU entrypoints in GradGradKernelsMatchDyFiniteDifferenceInTails, so extend it
to exercise the corresponding GPU kernels as well. Reuse the same tail points
and finite-difference helper, but add CUDA/ROCm-specific checks for the matching
grad-grad symbols so CPU and GPU parity is validated against the same derivative
contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 45b8d38e-3dde-45ce-b8fb-7427fcfb51a9
📒 Files selected for processing (1)
source/lib/tests/test_tabulate_extrapolate.cc
wanghan-iapcm
approved these changes
Jun 29, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
SchrodingersCattt
pushed a commit
to SchrodingersCattt/deepmd-kit
that referenced
this pull request
Jun 30, 2026
Fixes deepmodeling#5593. This implements the C1 linear extrapolation suggested in deepmodeling#5593 (comment): - keep the original out-of-range coordinate and evaluate the endpoint value/slope at the proper boundary offset - linearly extend table values outside the covered range while keeping gradients at the constant boundary slope - apply the same behavior to se_a, se_r, se_t, and se_t_tebd on CPU and GPU paths - add focused lib tests for lower/high tails, boundary continuity, finite-difference force consistency, and zero tail curvature Validation: - ruff check . - ruff format . - cmake --build source/build --target runUnitTests_lib -j2 - source/build/lib/tests/runUnitTests_lib --gtest_filter='TestTabulateSeA.tabulate_fusion_se_a_cpu:TestTabulateSeA.tabulate_fusion_se_a_grad_cpu:TestTabulateSeR.tabulate_fusion_se_r_cpu:TestTabulateSeR.tabulate_fusion_se_r_grad_cpu:TestTabulateSeT.tabulate_fusion_se_t_cpu:TestTabulateSeT.tabulate_fusion_se_t_grad_cpu:TestTabulateSeTTebd.tabulate_fusion_se_t_tebd_cpu:TestTabulateSeTTebd.tabulate_fusion_se_t_tebd_grad_cpu:TabulateExtrapolate.*' Note: I used the existing source/build directory reconfigured as TensorFlow-only locally because the previous cache had ENABLE_PYTORCH=ON with a CUDA PyTorch install but no CUDA toolkit available. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Tabulated SE polynomial evaluations now use an extrapolation-aware form for out-of-range inputs, improving smoothness and continuity at both lower and upper table boundaries across SE-a, SE-t, SE-t-tebd, and SE-r. * **Bug Fixes** * Corrected boundary/tail handling so value and gradient computations remain consistent near min/max, including off-grid max behavior; gradients now incorporate extrapolated results where needed. * **Tests** * Added CPU and GPU test coverage validating value/gradient/grad-grad consistency via polynomial reference comparisons and finite-difference checks for all supported SE variants. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5593.
This implements the C1 linear extrapolation suggested in #5593 (comment):
Validation:
Note: I used the existing source/build directory reconfigured as TensorFlow-only locally because the previous cache had ENABLE_PYTORCH=ON with a CUDA PyTorch install but no CUDA toolkit available.
Summary by CodeRabbit
New Features
Bug Fixes
Tests