Repository navigation
Restrict the sum forward rule to AbstractArray - #867
Conversation
|
I bumped the patch version to 1.74.12. VersionVigilante now passes. The source and regression test are unchanged. |
Codecov Report✅ All modified and coverable lines are covered by tests. 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. 🚀 New features to boost your workflow:
|
devmotion
left a comment
There was a problem hiding this comment.
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
mapreduceis noticeably slower than the rule for arrays (not benchmarked) - whether it works at all on GPU arrays, where Base's
sumgoes to GPUArrays'mapreducekernels (not tested; the@gpu test_fruletests 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: nothingNon-blocking suggestions:
- Mention in the description that this fixes
sumfrules 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
NumberandNamedTuplecases above). It could also use a realistic tangent such asTangent{typeof(xs)}(f=NoTangent(), iter=NoTangent())instead ofZeroTangent(), 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.
|
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 |
Fixes #768.
The
sumforward rule passesdimsto inputs whoseBase.summethods reject that keyword. I restrict the rule toAbstractArrayso non-array, non-tuple inputs use backend differentiation. This includes Generators, Numbers and NamedTuples. The tuple rule remains unchanged.The regressions check that
frulereturnsnothingfor 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:
(100.0,)afterward. Seven array, tuple, scalar and dimension controls pass in both runs.Pkg.test(; test_args=["--jobs=2"])passes: 43,364 passed and 73 marked broken, including the JLArrays tests.git diff --checkpasses.The public Diffractor workflow still reaches a separate
Tangent.datafield error on Julia 1.13.1. The successful derivative result above uses Julia 1.10, which Diffractor supports.cc @oxinabox