Issue #10071: Changes from Claude by szihs · Pull Request #10080 · shader-slang/slang

szihs · 2026-02-19T12:07:32Z

This PR addresses issue #10071

The derivative of fmod(x, y) = x - y * floor(x/y) w.r.t. y is -floor(x/y), not 0. Updated both forward and backward derivative implementations in diff.meta.slang to include the correct y gradient. Forward: d(fmod) = x.d - floor(x.p/y.p) * y.d (was just x.d) Backward: y.d = -floor(x.p/y.p) * dOut (was 0) Fixes #10071 Co-authored-by: Harsh Aggarwal (NVIDIA) <szihs@users.noreply.github.com>

coderabbitai · 2026-02-19T12:09:08Z

No actionable comments were generated in the recent review. 🎉

📝 Walkthrough

Walkthrough

This PR updates the fmod function's backward differentiation implementation to properly handle derivatives from both operands, with both a forward-reverse mode and in-place variant. Accompanying test suite validates derivative correctness across multiple scenarios.

Changes

Cohort / File(s)	Summary
Core Implementation `source/slang/diff.meta.slang`	Updated __d_fmod backward pass variants to compute and propagate derivatives from both x and y operands using floor division logic; added explanatory comments detailing derivative behavior.
Test Coverage `tests/autodiff/autodiff-fmod.slang`, `tests/autodiff/autodiff-fmod.slang.expected.txt`	Added new test file exercising backward and forward-mode differentiation for fmod with multiple test cases and corresponding expected output validation.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A modest fmod now knows both ways,
Its derivatives dance through backward's maze,
With floor and floor we trace the path,
Where x and y both feel the math—
The hoppy code hops true today! 🐇✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Title check	❓ Inconclusive	The PR title is vague and generic, referring only to 'Changes from Claude' without describing what the changes actually accomplish.	Use a more descriptive title that summarizes the actual change, such as 'Fix fmod gradient computation in automatic differentiation' or 'Issue `#10071`: Fix incorrect fmod derivative w.r.t. y'.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description check	✅ Passed	The description is minimal but related to the changeset—it mentions addressing issue `#10071`, which the commits reference, though it lacks implementation details.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch claude/issue-10071-20260219-1037

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

Copilot

Pull request overview

This PR aims to fix autodiff derivatives for fmod(x, y) so the gradient w.r.t. y is correctly propagated, and adds a regression test under tests/autodiff/.

Changes:

Update diff.meta.slang forward/backward derivative implementations for fmod.
Add a new autodiff regression test for fmod and its expected output.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File	Description
`source/slang/diff.meta.slang`	Adjusts `fmod` forward/backward derivative propagation logic.
`tests/autodiff/autodiff-fmod.slang`	Adds a compute test covering forward and backward autodiff for `fmod`.
`tests/autodiff/autodiff-fmod.slang.expected.txt`	Adds golden output for the new `fmod` autodiff test.

Copilot · 2026-02-19T12:12:29Z

source/slang/diff.meta.slang

+// fmod(x, y) = x - y * floor(x / y)
+// d/dx fmod(x, y) = 1
+// d/dy fmod(x, y) = -floor(x / y)


The added comment describes fmod(x, y) using floor(x/y), but Slang’s fmod matches C/HLSL semantics: fmod(x,y) = x - y * trunc(x/y) (toward zero). This matters for negative inputs, where floor gives a different quotient and would make the documented derivative incorrect.

Copilot · 2026-02-19T12:12:30Z

source/slang/diff.meta.slang

+    T.Differential dx = x.d;
+    T.Differential dy = __mul_p_d(-floor(x.p / y.p), y.d);
+    return DifferentialPair<T>(fmod(x.p, y.p), T.dadd(dx, dy));


__d_fmod uses floor(x.p / y.p) to compute the y contribution. For fmod (float remainder), the correct quotient is trunc(x/y) toward zero; using floor gives wrong gradients for negative values (e.g., x=-7, y=4). Please switch to trunc(x.p / y.p) (or an equivalent helper consistent with the actual fmod implementation).

Copilot · 2026-02-19T12:12:30Z

source/slang/diff.meta.slang

    x = diffPair(x.p, dOut);
-    y = diffPair(y.p);
+    y = diffPair(y.p, __mul_p_d(-floor(x.p / y.p), dOut));


The backward derivative also uses floor(x.p / y.p) for the propagated derivative to y. To match fmod remainder semantics (truncation toward zero), this should use trunc(x.p / y.p); otherwise gradients w.r.t. y will be incorrect for negative inputs.

Copilot · 2026-02-19T12:12:30Z

tests/autodiff/autodiff-fmod.slang

+    // fmod(5.5, 2.0) = 1.5
+    // d/dx fmod(x,y) = 1
+    // d/dy fmod(x,y) = -floor(x/y) = -floor(5.5/2.0) = -floor(2.75) = -2
+    {


The test comments use -floor(x/y) for the y-derivative, but Slang fmod uses truncation toward zero (trunc(x/y)). The current inputs are all positive so floor==trunc; adding a negative-input case (e.g., x=-7, y=4) would catch the difference.

Copilot · 2026-02-19T12:12:30Z

tests/autodiff/autodiff-fmod.slang

+RWStructuredBuffer<float> outputBuffer;
+
+typedef DifferentialPair<float> dpfloat;
+typedef float.Differential dfloat;


typedef float.Differential dfloat; is declared but not used in this test. Removing it would avoid dead code and keep the test focused on the fmod derivative behavior.

Suggested change

typedef float.Differential dfloat;

Copilot AI review requested due to automatic review settings February 19, 2026 12:07

szihs requested a review from a team as a code owner February 19, 2026 12:07

Copilot started reviewing on behalf of szihs February 19, 2026 12:08 View session

Copilot AI reviewed Feb 19, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Issue #10071: Changes from Claude#10080

Issue #10071: Changes from Claude#10080
szihs wants to merge 1 commit intomasterfrom
claude/issue-10071-20260219-1037

szihs commented Feb 19, 2026

Uh oh!

coderabbitai bot commented Feb 19, 2026 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 inconclusive)

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Copilot AI Feb 19, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Comments

Conversation

szihs commented Feb 19, 2026

Uh oh!

coderabbitai bot commented Feb 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

❌ Failed checks (1 inconclusive)

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai bot commented Feb 19, 2026 •

edited

Loading