exploit symmetry in the hessian - #837
Conversation
instead of relying on the jacobian of gradient for the hessian explicitly seed dual numbers and only calculate the upper triangular part when chunking, gives ~2x speedup as input length becomes big
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #837 +/- ##
==========================================
+ Coverage 90.68% 91.30% +0.61%
==========================================
Files 11 11
Lines 1052 1115 +63
==========================================
+ Hits 954 1018 +64
+ Misses 98 97 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks, this is a nice improvement. I went through it with the help of Claude. The core of it holds up. I reproduced the speedup locally with rosenbrock and the default chunk size: 49.3 -> 31.8 µs at The failing A few comments. The single-chunk case became slowerSince there is no vector-mode path anymore, seed_hessian_chunk!(xdual, x, 1, nothing, nothing, xlen) # zeroes everything
seed_hessian_chunk!(xdual, x, 1, iseeds, oseeds) # ... then overwrites block 1
seed_hessian_chunk!(xdual, x, 1, iseeds, oseeds)
seed_hessian_chunk!(xdual, x, N + 1, nothing, nothing, xlen - N)
...
nblocks > 1 && seed_hessian_chunk!(xdual, x, 1, nothing, nothing)turns a ~6% regression into a ~12-16% improvement over master, and gives identical results for every chunk size I tried:
It matters more than it looks, because the buffer is While you are in there: The StaticArrays path is left behind
So after this PR function ForwardDiff.hessian(f::F, x::StaticArray) where {F}
T = typeof(Tag(f, eltype(x)))
return extract_hessian(T, partials(T, f(dualize(T, dualize(T, x)))), x)
endplus
Structured inputs silently change shapeFor julia> size(ForwardDiff.hessian(z -> sum(abs2, z), UpperTriangular(rand(3, 3))))
(9, 6) # master
(6, 6) # this PRThe new shape is the right one (master mixes linear and structural indices, which I would call a bug), and I checked that the values are correct. But we have no Hessian test for structured inputs at all, so this is completely untested, and it is breaking for Docs
Smaller things
CoverageThe
Unrelated to the coverage numbers, but while adding tests: |
|
Addressed the review feedback in Highlights:
Benchmarks compare
Memory and allocation counts were unchanged. The complete test suite passes: 9,396/9,396 tests. |
409b6ea to
0c04e90
Compare
|
Thanks, this addresses everything from the last round. Suite is green here (9396/9396), and I re-checked the kernel against references for structured inputs at chunk sizes 1/2/ I want to revisit the
|
| call | master | 0c04e90 |
|---|---|---|
hessian!(fill(NaN, 4, 4), f, rand(3)) |
DimensionMismatch |
no error, row/col 4 left NaN |
hessian!(HessianResult(UpperTriangular(randn(3, 3))), f, x) |
DimensionMismatch |
no error, rows 7:9 are uninitialized memory |
hessian!(fill(NaN, 4, 4), f, SVector(1., 2., 3.)) |
DimensionMismatch |
no error, row/col 4 left NaN |
A shared helper used by both hessian! methods and the extension:
function hessian_result_matrix(result, hlen)
H = result isa AbstractMatrix ? result : reshape(result, hlen, hlen)
axes(H) == (Base.OneTo(hlen), Base.OneTo(hlen)) ||
throw(DimensionMismatch(lazy"hessian result has size $(size(H)), expected ($hlen, $hlen)"))
return H
endThis also turns the remaining breakage from the shape change into a clear error rather than a partial fill.
One triangle everywhere, for free
Diagonal blocks read outer <= inner, off-diagonal blocks read outer > inner, and the StaticArrays path reads outer <= inner everywhere. So the result is not reproducible across chunk sizes: at n = 16, chunk 1, 2, 3, 5, 7, 11 each differ from chunk 16 and from the SVector path (maxdiff ~1e-16). Swapping which layer block q carries fixes it:
seed_hessian_chunk!(xdual, x, qoffset + 1, iseeds, nothing, qsize) # was: nothing, oseeds
for p in 1:(q - 1)
poffset = (p - 1) * N
seed_hessian_chunk!(xdual, x, poffset + 1, nothing, oseeds) # was: iseeds, nothing
ydual = f(xdual)
extract_hessian_chunk!(T, H, ydual, poffset, qoffset, N, qsize) # was: qoffset, poffset, qsize, N
seed_hessian_chunk!(xdual, x, poffset + 1, nothing, nothing)
endSame number of evaluations and seed writes, and q is still seeded once outside the loop. Afterwards every chunk size gives bitwise-identical results, identical to the StaticArrays path. It also makes symmetric_static == hessian(symmetry_f, x) robust — right now that only passes because n = 9 is below DEFAULT_CHUNK_THRESHOLD, so the array path uses a single block.
The structured testset cannot fail
dot(weights, abs2.(x)) has a strictly diagonal Hessian and expected is diagm(...), so the testset cannot detect wrong row/column offsets, and it cannot detect a structure-preserving H container either — writing 0.0 off-diagonal into a Diagonal or UpperTriangular is legal. That matters because n = 5 with the triangular types is the only structured case that reaches the chunked path (xlen = 15, Chunk(x) gives N = 8), i.e. exactly the new off-diagonal code, and it is only ever compared against zeros. Could you use something coupled, e.g. z -> exp(dot(w, z)) with expected = f * wv * wv', and loop over explicit chunk sizes instead of relying on Chunk(x)?
Smaller things
- Please don't try to fold the four
seed_hessian_chunk!(..., nothing, nothing, ...)calls intoseed_zero_partials!. It looks equivalent butDual{T,V,N}(::V, ::Partials{N,V})takes the value at its exact type, so itMethodErrors on the nested buffer. seed_hessian_chunk!materialises bothizeroandozeroeven when the seeds are supplied. Free for isbitsV; 288 bytes per call forBigFloatatN = 3, identical for all three seed combinations.extract_hessian(::Type{T}, ::Partials{0}, x)deserves a comment. It is load-bearing for a constantfas well as for emptyx, and neither is obvious from the signature.- Coverage: the extension's
hessian!reshapebranch and its two newHESSIAN_ERRORthrows are untested, as are empty inputs.SeedTestcovers(iseeds, oseeds)and(nothing, nothing)but not the two mixed forms the off-diagonal loop actually uses, and it only asserts nonzero-ness rather than which layer received which seed. - The
AllocationsTesttestset is still called"Test seed!/seed_zero_partials! allocations". - The PR description still doesn't mention the structured-input shape change.
Unrelated, filed separately
Both are on master and predate this PR, but the second one is why I don't trust jacobian as precedent above:
gradient!writes to the wrong entries for structured inputs #838 —gradient!takes its target positions fromstructural_eachindex(result)rather than fromx.gradient!(fill(NaN, 3, 3), f, UpperTriangular(...))silently writes the 6 derivatives into linear positions1:6at every chunk size, andgradient!(GradientResult(x), f, x)throwsArgumentError: cannot set index (2, 1) in the lower triangular partfrom DiffResults' linearcopyto!.- Chunked
jacobianthrows forDiagonal/LowerTriangular/UpperTriangularinputs #839 — chunkedjacobianon structured inputs throwsDimensionMismatch—reshape_jacobianuseslength(xdual)while the result hasstructural_length(x)columns. Vector mode useschunksize(cfg)and works.
|
Maybe it is easier if you just point your bot on this branch instead of going through me, would save us both some tokens :). |
Instead of relying on the Jacobian of gradient for the Hessian, explicitly seed dual numbers and only calculate the upper triangular part when chunking, gives ~2x speedup as input length becomes big
Fixes #836 cc @gdalle
Basically one-shotted by Claude and then decringified with gpt.