Repository navigation
DArray: Infer the result type of reductions through f - #767
Conversation
OnlineStats' `value(::Variance)` multiplies by a `Float64` `NaN` literal
when it has no finite mean, so it is inferred as `Union{S,Float64}` for
any other precision `S`. Convert the value back to `S`, so that a
reduction over `dims` can carry a concrete eltype.
Nothing visible changes yet: the result eltype is `Any` until the
reduction infers its eltype through `f`.
`MapReduce` took its eltype from `op_outer(T, T)`, where `T` is the input
eltype. But `op_outer` combines what `f` returns, not the input elements.
When `f` changes the type, the result was labelled with the wrong eltype
and `collect` converted to it. For a `Float64` matrix `A` and a
`ComplexF64` matrix `C`:
sum(x->x>0.5, A) # 13.0 instead of 13
sum(abs2, C) # ComplexF64 instead of Float64
sum(x->complex(x, 1.0), A) # InexactError
When `op_outer` has no method for `T` at all, the eltype fell back to
`Any`: `mean`/`var`/`std` with `dims` (`merge` of two numbers), and
`maximum(abs, C; dims)` (`max` of two complex numbers).
`_onlinestats_mapreduce` now takes the statistic as a `::Type{S}`. Julia
1.11 does not infer what the closure returns when it captures the type
from an untyped argument.
An `init` takes part in the inference, so `sum(f, A; init=0.0)` keeps
the type of the `init` when that is the wider one.
The existing tests compare with `isapprox`, which accepts a `Matrix{Any}`
and a `13.0`, so the new ones also compare the types.
The tree reduction broadcasts `op_outer` over two partial results. When those hold `Bool`s the broadcast returns a `BitArray`, which `to_array` did not take for an array, so it wrapped it in a 1-element `Array`. For a matrix split into several partitions, `all(f, A; dims=1)` returned a matrix of `BitMatrix`, and `any`, `prod` and `maximum` with a `Bool` valued `f` threw.
`extrema` reduces `Extrema` values, which `_extrema_inner` builds from
what `f` returns. `_extrema_outer` has no method for the bare values, so
`extrema(A; dims=1)` had the eltype `Tuple{Any,Any}`. Tell `MapReduce`
what the partial results hold.
|
Heads up, one thing I saw while benchmarking this. On Julia 1.12.7 with 8 threads, the first I tried to make it happen again and couldn't. 40 more runs on this branch and 24 on master all finished fine. So I'm not sure if this PR is the cause. It might make it a bit more likely, because |
|
That sounds like a Julia bug (and I've seen stuff like this as well around calls into the compiler), and with it appearing to be quite rare, I'm not too worried about it right until it becomes a noticeable issue (and stuff like this tends to get fixed upstream anyway, so it may just "go away" without any action by us). Regardless, I much appreciate the report, and I'll keep an eye out for it! If you do manage to reproduce it with frequency, let me know 😄 Anyway, this PR is great, thank you! |
2 participants
Fixes #764.
MapReducetook its eltype fromop_outer(T, T)withTthe input eltype, althoughop_outercombines whatfreturns. Four commits:var/stdin the input precision withdims. OnlineStats'value(::Variance)is inferred asUnion{S,Float64}, because it multiplies by aFloat64NaNwhen it has no finite mean. Without this the next commit would givevarof aFloat32array that union as its eltype.f. Fixes the wrong type ofsum(x->x>0.5, DA), theInexactErrorofsum(x->complex(x, 1.0), DA), and theAnyeltype ofmean/var/stdandmaximum(abs, DC)withdims. Aninittakes part in the inference, sosum(f, DA; init=0.0)keeps the wider type._onlinestats_mapreducenow takes the statistic as::Type{S}, otherwise Julia 1.11 does not infer the closure.BitArraypartial reduction.to_arrayonly matchedArray, soall(f, DA; dims=1)returned a matrix ofBitMatrix, andany/prod/maximumwith aBoolvaluedfthrew. Add Reactant optimization support #738 changes the same line the same way, so whichever lands second just drops it.extremawithdimsa concrete eltype. It wasTuple{Any,Any}. This one is separate from the bug and easy to drop if you would rather not have the extra_mapped_typemethod.The new tests compare types as well as values, since
isapproxaccepts aMatrix{Any}. With the source of master, about half of them fail or error (50 of 96, counted before I added the twoinitcases).array/mapreducepasses on Julia 1.10, 1.11 and 1.12 (421 tests, 3 workers), and each commit passes on its own. Before theinitchange I also ran broadcast, indexing, copyto, permute, allocation, linalg/core and linalg/arithmetic on 1.12, and all passed.Nothing here adds work per element. The only new cost is one more
Base._return_typecall per reduction.array/corehas one error inmap!on my machine with Julia 1.12.7. It is the same on master, so I left it alone.