Skip to content

Restrict the sum forward rule to AbstractArray - #867

Merged
devmotion merged 3 commits into
JuliaDiff:mainfrom
gezerui888:fix/sum-generator-frule-768
Nov 9, 2026
Merged

devmotion merged 3 commits into
JuliaDiff:mainfrom
gezerui888:fix/sum-generator-frule-768

Conversation

@gezerui888

@gezerui888 gezerui888 commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #768.

The sum forward rule passes dims to inputs whose Base.sum methods reject that keyword. I restrict the rule to AbstractArray so non-array, non-tuple inputs use backend differentiation. This includes Generators, Numbers and NamedTuples. The tuple rule remains unchanged.

The regressions check that frule returns nothing for a Generator, a Number and a NamedTuple. The Generator and NamedTuple cases use structural tangents. All three fail with the original keyword error before this change.

Validation:

  • On Julia 1.10.13, the reported Diffractor derivative fails before the change and returns (100.0,) afterward. Seven array, tuple, scalar and dimension controls pass in both runs.
  • On Julia 1.13.1, Pkg.test(; test_args=["--jobs=2"]) passes: 43,364 passed and 73 marked broken, including the JLArrays tests.
  • On Julia 1.10.13, the merged mapreduce and Random tests pass: 1,951 passed and 22 marked broken.
  • Changed lines pass formatting checks, and git diff --check passes.
  • The patch version is 1.74.13; VersionVigilante accepts the increment from current main.

The public Diffractor workflow still reaches a separate Tangent.data field error on Julia 1.13.1. The successful derivative result above uses Julia 1.10, which Diffractor supports.

cc @oxinabox

@gezerui888

Copy link
Copy Markdown
Contributor Author

I bumped the patch version to 1.74.12. VersionVigilante now passes. The source and regression test are unchanged.

@codecov

codecov Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.65%. Comparing base (30cf1f6) to head (80a46ea).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #867   +/-   ##
=======================================
  Coverage   96.65%   96.65%           
=======================================
  Files          28       28           
  Lines        3615     3615           
=======================================
  Hits         3494     3494           
  Misses        121      121           

☔ 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.

@devmotion devmotion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! Narrowing the rule fixes #768.

Open question: do we need this frule at all? An alternative is to remove the sum(x; dims) frule entirely and let forward-mode AD differentiate through Base's sum. That would rule out this class of bug (rules claiming calls they can't handle, see also #868). Nothing inside ChainRules calls this frule, so it only matters to forward-mode AD systems that use ChainRules frules (mainly Diffractor). What we don't know:

  • whether AD through Base's mapreduce is noticeably slower than the rule for arrays (not benchmarked)
  • whether it works at all on GPU arrays, where Base's sum goes to GPUArrays' mapreduce kernels (not tested; the @gpu test_frule tests only show that the rule itself works on JLArrays)

If someone has data on either, that would decide between narrowing (this PR) and removing.

No working behaviour is lost by narrowing. On Julia 1.10 and 1.13, the only Base sum method that accepts dims is sum(a::AbstractArray; dims, kw...). For any other input, the old untyped rule's sum(x; dims=:) goes through sum(a; kw...) to mapreduce and mapfoldl, and throws. So that method could only ever have worked for AbstractArrays, and the frule signatures now match the existing rrules.

The fix covers more than Generators. On main, every input that is neither an AbstractArray nor a Tuple hits the same error. I checked locally:

frule((NoTangent(), 1.0), sum, 2.0)
# main: MethodError: no method matching mapfoldl(::typeof(identity), ::typeof(Base.add_sum), ::Float64; dims::Colon)
# this PR: nothing

nt = (a=1.0, b=2.0)
frule((NoTangent(), Tangent{typeof(nt)}(a=1.0, b=2.0)), sum, nt)
# main: MethodError ... ::@NamedTuple{a::Float64, b::Float64}; dims::Colon
# this PR: nothing

Non-blocking suggestions:

  • Mention in the description that this fixes sum frules for any non-array, non-tuple input (Numbers, NamedTuples, Sets, Dicts, custom iterators), not only Generators.
  • Extend the regression testset to a couple of those inputs (e.g. the Number and NamedTuple cases above). It could also use a realistic tangent such as Tangent{typeof(xs)}(f=NoTangent(), iter=NoTangent()) instead of ZeroTangent(), since that is closer to what an AD system passes in.

The test checks what this repo can check. It throws the #768 error before the change and returns nothing after. Testing the full derivative through the AD fallback would need a forward-mode AD as a test dependency, which we don't want here.

The broader mismatch between the sum rules and Base's signatures (e.g. no rule accepts init) is a separate problem, tracked in #868. It doesn't need to be fixed in this PR.

@gezerui888

Copy link
Copy Markdown
Contributor Author

I added the Number and NamedTuple cases and a structural Generator tangent. All three regressions fail with the original rule and pass with the narrowed rule. I also expanded the description to cover non-array, non-tuple inputs.

I merged current main and bumped the version to 1.74.13. The full Julia 1.13.1 suite passes with 43,364 tests and 73 marked broken; the focused Julia 1.10.13 run passes with 1,951 tests and 22 marked broken. I kept the array rule and left init support to #868.

@devmotion devmotion left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

@devmotion
devmotion merged commit d9ef0f7 into JuliaDiff:main Nov 9, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

frule for sum doesn't work for Generator

2 participants