Extract a gradient the result cannot carry as zero - #848
Open
devmotion wants to merge 1 commit into
Open
Conversation
`extract_gradient!` dispatched on a bare `::Dual`. A `Dual{S,V,N}` with
`S ≺ T` carries no `T`-perturbation, so its gradient is zero everywhere and
the `::Real` method is what it needs -- but it *is* a `Dual`, so it took the
`::Dual` method, where `npartials` reports the `N` of the wrong layer.
Whenever that `N` was smaller than `structural_length(x)` the tail of the
result was never written:
ForwardDiff.derivative(1.0) do a
out = fill(a * 111.0, 3)
ForwardDiff.gradient!(out, z -> a * 2.0, [1.0, 2.0, 3.0])
@show ForwardDiff.value.(out) # [0.0, 111.0, 111.0]
return zero(a)
end
The allocating form returned uninitialized memory, and the `DiffResult` form
passed `partials(T, dual)` -- a scalar `Dual` rather than a `Partials` -- to
`DiffResults.gradient!` and errored. `gradient!` into a plain result was
affected for a `StaticArray` too, since it shares `extract_gradient!`; only
the allocating `gradient`, whose `@generated extract_gradient` is built from
`length(x)`, was correct.
Dispatch on `Dual{T}`, as the Hessian does on `Dual{TO,<:Dual{T}}`, so a
result carrying only an enclosing tag falls through to the `::Real` method,
which already fills the whole result.
The two `::Real` methods take their value from `value(T, y)` rather than `y`.
For a plain `Real` and for a `Dual{S}` with `S ≺ T` that is the identity, and
it keeps rejecting a tag with no such relation, which the `::Dual` methods did
through `partials(T, dual, i)`. Chunk mode rejects it regardless, from
`partials(T, dual, i)` and from `similar(x, valtype(T, ydual))`, so vector
mode has to agree or the result would depend on the chunk size.
Chunk mode needed no change: `extract_gradient_chunk!` is bounded by
`chunksize` rather than `npartials`, and `partials(T, ::Dual{S}, i)` already
answers zero for every position.
Fixes #847.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #848 +/- ##
==========================================
+ Coverage 90.68% 91.07% +0.38%
==========================================
Files 11 11
Lines 1052 1053 +1
==========================================
+ Hits 954 959 +5
+ Misses 98 94 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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 #847.
extract_gradient!dispatched on a bare::Dual. ADual{S,V,N}withS ≺ Tcarries noT-perturbation, so its gradient is zero everywhere and the::Realmethod is what it needs — but it is aDual, so it took the::Dualmethod, wherenpartialsreports theNof the wrong layer. Whenever thatNwas smaller thanstructural_length(x)the tail of the result was never written:The allocating form returned uninitialized memory, which is often zero and so hides the bug, and the
DiffResultform passedpartials(T, dual)— a scalarDualrather than aPartials— toDiffResults.gradient!and errored.gradient!into a plain result was affected for aStaticArraytoo, since it sharesextract_gradient!throughvector_mode_gradient!. Only the allocatinggradientwas correct, because its@generated extract_gradientis built fromlength(x)and uses the indexedpartials(T, y, i).Fix
Dispatch on
Dual{T}, as #844 does for the Hessian onDual{TO,<:Dual{T}}, so a result carrying only an enclosing tag falls through to the::Realmethod, which already fills the whole result.Dual{T} <: Real, so this is a strict narrowing with no new ambiguity.The two
::Realmethods take their value fromvalue(T, y)rather thany. For a plainRealand for aDual{S}withS ≺ Tthat is the identity, and it keeps rejecting a tag with no such relation, which the::Dualmethods did throughpartials(T, dual, i). Chunk mode rejects it regardless — frompartials(T, dual, i), fromsimilar(x, valtype(T, ydual)), and fromextract_value!'svalue(T, d)for aDiffResult— so vector mode has to agree or the result would depend on the chunk size.Chunk mode needed no change:
extract_gradient_chunk!is bounded bychunksizerather thannpartials, andpartials(T, ::Dual{S}, i)already answers zero for every position.jacobianandderivativeare likewise unaffected —extract_jacobian!broadcasts over allncolumns andextract_derivativeproduces a singlepartials(T, y, 1).Tests
A new testset over
Vector,SVectorandMVector, covering the allocating,AbstractArrayandDiffResultforms:::Realpath this PR touches.gradient!leaves part of the result unwritten whenfdoes not depend on its argument #847, plus@test_throws MethodErrorfor aFloat64buffer that cannot hold the enclosing tag, mirroring HessianTest's assertion.zero(y)turning aDualMismatchErrorinto a silent zero gradient.Against the unfixed source the testset gives 6 failures and 3 errors, on the array-buffer cases for all three input types and on chunk size 3 — the only chunk size that takes the vector-mode path for a length-3 input. With the fix the whole suite passes (Derivative, Gradient, Jacobian, Hessian, Confusion, Misc, Dual, Partials, Seed, Allocations).
Not addressed here
Two adjacent gaps, both pre-existing and neither introduced nor worsened by this change:
gradient!never checks the result's size, unlikehessian!'sreshape_hessian.ForwardDiff.gradient!(zeros(5), sum, [1.0, 2.0])writes two entries and leaves three stale. The::Realand::Dualbranches already disagreed about this —fill!covers the whole buffer — and this PR only routes one more case to thefill!side. Astructural_length(result) == structural_length(x)check would close it, but it would reject calls that are accepted today.gradient!into anImmutableDiffResult(DiffResults.GradientResult(::SVector)) hitsfill!on anSVectorand errors; there is nogradient!(::ImmutableDiffResult, f, ::StaticArray)specialisation the way there is forhessian!. Makingfill!work alone would not help, sincegradient!discards whatvector_mode_gradient!returns, so the functional update would be dropped. The new tests keep theirDiffResultassertions on mutable buffers.🤖 Generated with Claude Code