Extract derivatives at the indices of x, not of the result - #840
Open
devmotion wants to merge 6 commits into
Open
Extract derivatives at the indices of x, not of the result#840devmotion wants to merge 6 commits into
x, not of the result#840devmotion wants to merge 6 commits into
Conversation
Since #739 only the structurally non-zero entries of an input are seeded, but extraction was not updated to match, so the derivatives were written to positions taken from the result container instead of from `x`. `extract_gradient!`/`extract_gradient_chunk!` now take `x` and walk `structural_eachindex(x, result)`. Entries that receive no derivative are zeroed, which is their derivative; in chunk mode the first chunk does it. The `DiffResult` method splits on mutability, since an immutable result cannot be written to entry by entry (and only occurs for `StaticArray` inputs, all of whose entries are structural). Fixes #838, where a dense result got the derivatives at linear positions `1:structural_length(x)` and a `DiffResults.GradientResult` threw, and with it the mis-scattered gradient of `hessian!(::DiffResult, ...)`. The Jacobian is indexed by the linear indices of `x`: column `j` holds `∂f(x)[i]/∂x[j]`, as documented, with hard zeros in the columns of the structural zeros. Its allocations therefore use `length(x)` rather than `structural_length(x)`, which is what `reshape_jacobian` expected all along, so chunk mode stops throwing. Fixes #839. `structural_linearindices` maps a structural position to a linear index of `x` without materializing anything, and the single-broadcast path is kept when every index of `x` is structural, so no path allocates more than before. This changes the shape of the result for structured inputs: `jacobian` gains the zero columns and `hessian` inherits both conventions through `jacobian(∇f, x)`, becoming `length(x) x length(x)` with hard-zero rows and columns instead of mixing linear and structural indices. That also makes `hessian!(DiffResults.HessianResult(x), ...)` work. For a `Diagonal` the result now scales with `length(x)`, so differentiating with respect to the diagonal vector is the better choice there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #840 +/- ##
==========================================
+ Coverage 90.68% 91.24% +0.55%
==========================================
Files 11 11
Lines 1052 1085 +33
==========================================
+ Hits 954 990 +36
+ Misses 98 95 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…vered
The new allocation test failed on Julia <= 1.10 for `Diagonal` inputs, but
none of the allocations came from extraction: the target function reduced
with the no-function `sum(z)`, and `Base._sum(::Diagonal, ::Colon)` allocates
there (32 bytes even for a `Diagonal{Float64}`). Reducing with `sum(f, z)`
instead measures ForwardDiff rather than LinearAlgebra, and extraction turns
out to be allocation-free for every input type on both 1.10 and 1.12.
That left the chunk-mode comparison against a dense input, which was hiding a
real cost: `reshape_jacobian` reshapes the result even when it already is a
matrix, and since 1.11 `reshape` can no longer return its argument, so every
chunk-mode `jacobian!` allocated an `Array` wrapper. `extract_jacobian!` had
been given that short-circuit in #797; `reshape_jacobian` now shares it, with
an explicit size check in place of the one `reshape` performed on the way
past, and `extract_jacobian!` calls it instead of repeating the ternary. Both
modes now reject a wrongly shaped matrix result with the same error, and the
test can assert zero allocations outright.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`extract_gradient_chunk!`/`extract_jacobian_chunk!` recognised the first chunk by `index == 1` and used it to zero the whole result, which is neither part of extracting one chunk nor something a chunk can decide: the entries at stake -- those of the structural zeros of `x` -- belong to no chunk in particular. Both sweeps now do it once up front, and the chunk functions only write their chunk. The gradient's half is shared with `extract_gradient!` as `zero_unseeded!`, which also fixes the condition. `structural_length(x) != length(x)` zeroed a result that is itself structured, whose every stored entry the sweep goes on to write; comparing against `structural_length(result)` skips that, and leaves the mismatched-structure cases erroring at the same write as before. The Jacobian keeps its own test, since its result is not shaped like `x` and what has to be covered there is columns. Also drops the `map!` that `vector_mode_jacobian(f!, ...)` ran before `extract_jacobian!`, which reads only `ydual`, and that the `map!` after it repeats. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`hessian` inherits both conventions through `jacobian(gradient(f), x)`, so it changes shape for a structured `x` and `hessian!(DiffResults.HessianResult(x), f, x)` starts working, none of which was asserted. The new testset pins the value, the gradient and the Hessian of a function whose second derivative is `1 + (a == b)` on the structural entries, so the reference is exact and the hard-zero rows and columns are checked rather than approximated. Against master it fails everywhere: the smaller chunks throw, and the full-length one gets the wrong shape and cannot take a `HessianResult` at all. Also: the out-of-place `gradient` is shaped like `x`, so its zeros are the structural ones and its type is worth asserting; a structured result cannot hold the gradient of a dense `x`, which now throws where it used to write to the wrong entries; a Jacobian result that is not a matrix is reshaped; and the `f!` form takes a `JacobianResult` too. Both structured testsets take the chunk sizes from `length(sidx)` rather than `ForwardDiff.structural_length`, keeping the reference data in the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`extract_gradient!`, `extract_gradient_chunk!`, `extract_jacobian!` and
`extract_jacobian_chunk!` take `x` after the derivatives, like every other internal
function that pairs an output with an input: `reshape_jacobian(result, ydual, x)`, which
`extract_jacobian!` calls on its first line, `extract_value!(T, out, y, ydual)`, and the
`extract_jacobian(T, ydual, x::StaticArray)` of the StaticArrays extension. `x` is
annotated `AbstractArray` there, so a call written against an older signature fails on
the method rather than inside `structural_length`.
`extract_jacobian!` duplicated the structured branch of `extract_jacobian_chunk!` at
`offset == 0` and now delegates to it, and both Jacobian sweeps zero through one helper.
`structural_linearindices(x)` becomes `structural_columns(out, x)`: what a caller needs
are the columns of `out` that receive a derivative, and every method returns those,
`axes(out, 2)` or a subset of it, without materializing anything. It checks its
arguments the way `structural_eachindex` does, whose three bare `DimensionMismatch()`
now name the sizes they expected.
The chunk mode gradient sweep rejects an array-valued `f` up front, as the Jacobian
sweep does, in place of the `dual::AbstractArray` methods of `extract_gradient_chunk!`:
`zero_unseeded!` runs before them and would fail on `zero(::Type{<:AbstractArray})`
before dispatch got there. `zero_unseeded!` also dispatches on `DiffResult` rather than
`MutableDiffResult`, since a `StaticArray` gradient buffer makes the result immutable
even when the buffer itself can be written to entry by entry, as an `MVector` can.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…atic buffer A wrongly shaped `jacobian!` result and a `gradient!` result that is not shaped like `x` both throw now, the latter for a dense `x` too, and neither was asserted. Nor was the `MVector` gradient buffer of an `ImmutableDiffResult`, which chunk mode writes entry by entry. The structured Jacobian and Hessian testsets only ran chunk sizes that divide the number of seeded entries, so the final chunk was always a full one; they gain a size that leaves a remainder. `SeedTest` pins the columns `structural_columns` returns against the index sets it already writes out by hand, since the Jacobian misplaces its derivatives if they disagree with `structural_eachindex`. The allocation test measures a dense result as well, which is the case that zeroes before extracting, and the JET tests cover a structured input, which reaches the generator branches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
devmotion
marked this pull request as ready for review
August 18, 2026 15:49
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 #838 and #839. Both are the same omission from #739: seeding became structure-aware, extraction did not.
gradient!wrote to the wrong entries (#838)extract_gradient!/extract_gradient_chunk!took their positions fromstructural_eachindex(result)while the seeds are laid out alongstructural_eachindex(x). They now takexand walkstructural_eachindex(x, result). Entries that receive no derivative are zeroed -- that is their derivative -- byzero_unseeded!, skipped when the seeded entries ofxalready account for every entryresultstores, which covers both an unstructuredxand thesimilar(x)result thatgradientallocates for a structured one.The
(DiffResult, Dual)method splits on mutability:MutableDiffResultis written entry by entry,ImmutableDiffResultkeeps the wholesaleDiffResults.gradient!copy, since it cannot be written entry by entry and only arises fromStaticArraygradient buffers, for which every entry ofxis seeded.This also fixes the mis-scattered gradient of
hessian!(::DiffResult, f, x), whose bufferDiffResults.HessianResultallocates densely even for a structuredx(the case from the #837 review).Chunked
jacobianthrew (#839)The result was allocated with
structural_length(x)columns whilereshape_jacobianasked forlength(xdual). The allocation is the side that was wrong: the Jacobian is indexed by the linear indices ofx-- columnjholds∂f(x)[i]/∂x[j], exactly as the docstring says -- with hard zeros in the columns of the structural zeros. So the four allocations now uselength(x), and the redundantnargument ofextract_jacobian!is dropped (it was alwaysstructural_length(x)).structural_columns(out, x)returns the columns ofoutthat receive a derivative, in seeding order, without materializing anything:axes(out, 2)where every entry ofxis seeded,axes(out, 2)[diagind(x)]for aDiagonal, and a lazy generator overstructural_eachindexfor the triangular wrappers, whose seeded columns are runs of increasing length and hence no range. It checks its arguments the waystructural_eachindexdoes.Zeroing belongs to the sweep, not to a chunk
The chunk extractors first recognised the first chunk by
index == 1and used it to zero the whole result. That is neither part of extracting one chunk nor something a chunk can decide -- the entries at stake, those of the structural zeros ofx, belong to no chunk in particular -- and it made the generated expressions depend on an invariant that only a comment carried. Both sweeps now do it once up front and the chunk functions only write their chunk.reshape_jacobianallocated a wrapper on every chunked callFalling out of the allocation test below.
reshape_jacobianreshaped the result even when it already was a matrix, and since 1.12reshapecan no longer return its argument, so every chunk-modejacobian!allocated anArraywrapper -- for dense inputs too, not just structured ones.extract_jacobian!was given that short-circuit in #797;reshape_jacobiannow shares it, with an explicit size check that is stricter than the onereshapewas performing on the way past -- that only ruled out a wrong total length, so a matrix of the right length but the wrong shape was silently reinterpreted -- andextract_jacobian!calls it rather than repeating the ternary. Also drops themap!thatvector_mode_jacobian(f!, ...)ran beforeextract_jacobian!, which reads onlyydual, and that themap!after it repeats.xlast, and each job in one placeextract_gradient!,extract_gradient_chunk!,extract_jacobian!andextract_jacobian_chunk!takexafter the derivatives, like every other internal function that pairs an output with an input:reshape_jacobian(result, ydual, x), whichextract_jacobian!calls on its first line,extract_value!(T, out, y, ydual), and the extension'sextract_jacobian(T, ydual, x::StaticArray).xis annotatedAbstractArrayin the Jacobian extractors, so a call written against an older signature fails on the method rather than insidestructural_length.extract_jacobian!duplicated the structured branch ofextract_jacobian_chunk!atoffset == 0and now delegates to it, and both Jacobian sweeps zero through one helper, as the gradient sweep already did.The chunk mode gradient sweep rejects an array-valued
fup front, as the Jacobian sweep does, in place of thedual::AbstractArraymethods ofextract_gradient_chunk!:zero_unseeded!runs before them and dies onzero(::Type{<:AbstractArray})before dispatch gets there, sogradienton an array-valuedfreported aMethodErrorinstead of pointing atjacobian.zero_unseeded!also dispatches onDiffResultrather thanMutableDiffResult: aStaticArraygradient buffer makes the result immutable even when the buffer itself can be written to entry by entry, as theMVectorofDiffResult(0.0, @MVector zeros(n))can, which is what chunk mode does with it.Breaking for structured inputs
Both fixes follow the convention argued for in #839/#837 -- index the result by the indices of
x, hard zeros off the structure -- which is whatgradient(f, x)has always returned. Consequences:jacobiangains the zero columns:(2, 6)→(2, 9)forUpperTriangular(3×3).hessianinherits both conventions throughjacobian(∇f, x)and becomeslength(x) × length(x)with hard-zero rows and columns, instead of mixing linear rows with structural columns:(9, 6)→(9, 9).hessian!(DiffResults.HessianResult(x), f, x)starts working (it allocateslength(x)^2), as doesjacobian!(DiffResults.JacobianResult(y, x), ...).Diagonalthe Jacobian/Hessian now scale withlength(x) = n², so differentiating with respect to the diagonal vector is the better choice there.gradient!into a container whose size differs from a structuredxnow throwsDimensionMismatchinstead of packing the derivatives in structural order (gradient!(zeros(6), f, UpperTriangular(3×3))). The packed order is a ForwardDiff-internal detail. A densexis affected only by a length mismatch, whicheachindex(x, result)rejects where the entries the two had in common used to be filled (gradient!(zeros(5), f, rand(4))); a result of the same length but a different shape, such as a matrix for a vector input, keeps working.jacobian!into a matrix whose shape is notlength(y) × length(x)now throwsDimensionMismatchin chunk mode, where it used to be reshaped whenever the total length happened to match. Vector mode already used such a result as is; the two modes now agree.Tests
New testsets in
GradientTest.jl(#838),JacobianTest.jl(#839) andHessianTest.jl(both, inherited) over the three wrappers × sizes × every relevant chunk size, covering a dense result, a result shaped likex,DiffResults.GradientResult/JacobianResult/HessianResult, a denseDiffResultgradient buffer, aJacobianresult that is not a matrix, and both thefandf!Jacobian forms. Results are prefilled withNaNso that entries left untouched fail rather than pass, and the expected nonzero positions are written out by hand so a bug in the position mapping cannot hide inside the reference. The Hessian test differentiates a function whose second derivative is1 + (a == b)on the structural entries, so the reference is exact and the hard-zero rows and columns are asserted rather than approximated; against master that testset gets 0 passed, 3 failed, 4 errored. Also asserted: the out-of-placegradientreturns the structure ofx, and a structured result cannot hold the gradient of a densex.AllocationsTest.jlguards the new branches. Its first version failed on Julia ≤ 1.10 forDiagonal, but none of the allocations came from extraction: the target function reduced with the no-functionsum(z), andBase._sum(::Diagonal, ::Colon)allocates there -- 32 bytes even for aDiagonal{Float64}. Reducing withsum(f, z)measures ForwardDiff rather than LinearAlgebra, and with thereshape_jacobianfix above every path asserts zero outright, on 1.10 and on 1.12, for all four input types in both modes.Asserted on top of that: a wrongly shaped
jacobian!result and agradient!result that is not shaped likex, a densexincluded, both throw; chunk mode fills theMVectorgradient buffer of anImmutableDiffResult; the structured Jacobian and Hessian testsets run a chunk size that leaves a partial final chunk, which every size they had divided;SeedTestpins the columns ofstructural_columnsagainst the index sets it already writes out by hand, since the derivatives land in the wrong columns if they disagree withstructural_eachindex; the allocation test measures a dense result as well, which is the case that zeroes before extracting; and the JET tests cover a structured input, which reaches the generator branches. (On arm64 macOS with Julia 1.10.12, the olderTest jacobian! allocationstestset reports 16 bytes both here and on569af35-- they come from the target function's own broadcast overVector{Dual}, not from extraction, and 1.12 reports zero.)Beyond the suite: a sweep over the three wrappers ×
n ∈ {1,2,3,5,8}× every chunk size × every result container agrees with the closed-form expectation, with the Hessian symmetric and its structural rows/columns exactly zero; empty inputs, flatVectorresults andBigFloatinputs with unassigned entries behave. On the GPU side the dense path -- the only one a GPU array reaches, since the new branch requiresstructural_length(x) != length(x)-- is unchanged,partials_wrapand the single fused broadcast intact; aJLArrayrun fails identically on this branch and on master, at the scalar indexing inseed!that #816 addresses.🤖 Generated with Claude Code