Add regression test for ITensor with [BackwardDerivative]#918
Add regression test for ITensor with [BackwardDerivative]#918jhelferty-nv wants to merge 4 commits into
Conversation
Regression test for shader-slang#639. Tests forward and backward passes for functions using ITensor interface parameters with both [Differentiable] and [BackwardDerivative] attributes. Made-with: Cursor
…ion test The [Differentiable] variants test baseline behavior that was never broken and is already covered by other tests. Keep only the [BackwardDerivative] tests that directly guard against the slangpy#639 regression. Made-with: Cursor
📝 WalkthroughWalkthroughA regression test module is added to validate issue Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
slangpy/tests/slangpy_tests/test_itensor_custom_backward.py (1)
59-63: Use.grad_infor semantic clarity in backward pass setup.The pattern in
test_difftensor_load_variants.pyexplicitly usesresult.grad_in.storage.copy_from_numpy()rather thanresult.grad.storage, and also assertsresult.grad_in is not None. While.gradis a valid convenience property that resolves to.grad_inwhen.grad_outis unavailable, using.grad_indirectly is more semantically explicit for seeding the input gradient to the backward pass. For consistency with the codebase pattern and clarity, use:result.grad_in.storage.copy_from_numpy(np.ones(N, dtype=np.float32))The regression test purpose is already documented in the test docstring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@slangpy/tests/slangpy_tests/test_itensor_custom_backward.py` around lines 59 - 63, Replace the backward-pass seeding to use the explicit input-gradient field: ensure the test sets the seeded gradient into result.grad_in rather than the convenience alias result.grad; call result.grad_in.storage.copy_from_numpy(np.ones(N, dtype=np.float32)) and assert result.grad_in is not None before copying so the backward invocation func.bwds(coord=spy.grid((N,)), x=xt, _result=result) receives the semantically correct input gradient.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@slangpy/tests/slangpy_tests/test_itensor_custom_backward.py`:
- Around line 59-63: Replace the backward-pass seeding to use the explicit
input-gradient field: ensure the test sets the seeded gradient into
result.grad_in rather than the convenience alias result.grad; call
result.grad_in.storage.copy_from_numpy(np.ones(N, dtype=np.float32)) and assert
result.grad_in is not None before copying so the backward invocation
func.bwds(coord=spy.grid((N,)), x=xt, _result=result) receives the semantically
correct input gradient.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d523617-177d-45f5-8fad-e501b2098117
📒 Files selected for processing (1)
slangpy/tests/slangpy_tests/test_itensor_custom_backward.py
ccummingsNV
left a comment
There was a problem hiding this comment.
Looks good to me - I'm guessing it has to wait for fixed slang
|
Correct, we don't have a public Slang release with shader-slang/slang#9808 in it yet. |
|
Moving this back to draft because it's taking a while to get a new slang update into slangpy with shader-slang/slang#9808. |
Add regression test for slangpy#639. Tests forward and backward passes
for a function using ITensor interface parameters with
[BackwardDerivative], the pattern from the original issue report. The
underlying compiler crash was fixed by the auto-diff refactor in
shader-slang/slang#9808.
Fixes #639
Made with Cursor
Summary by CodeRabbit