Repository navigation
Conversation
Summing integers before dividing overflowed for the whole-array mean (`mean(fill(typemax(Int)÷2, 4))` gave -1.0), while the `dims` variant pre-scaled every term by 1/n, which loses precision (a mean of ones that is not one) and captured `f` in a `ComposedFunction`, so a type like `mean(Float32, A; dims=1)` could not be passed to the kernel. Follow Statistics.jl instead: convert each mapped element to the result's floating-point type, sum, then divide. The type is inferred rather than taken from `first(A)` to avoid scalar indexing, and the conversion closure keeps `f` as a static parameter when it is a type, so it stays isbits. The two-argument `_mean` methods are dropped: Statistics has only called `_mean(f, A, dims)` since 1.9. Fixes #819, fixes #611, supersedes #453.
This branch has not been deployed
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
1 participant
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.
Accumulate
meanin floating point and divide afterwardsmeanon GPU arrays had three separate bugs, all from not following what Statistics.jl does:mean(CuArray(fill(typemax(Int)÷2, 4)))returned-1.0(meancan overflow on integer inputs #819).dimsmean multiplied every term by1/nbefore summing, which loses precision: themean of
ones(Float32, 640, 64)alongdims=1came out as1.0000001(Fix rounding problems in _mean function #453).fin aComposedFunction. Whenfis a type, the result is notisbits, so
mean(Float32, A; dims=1)failed with aKernelError(Statistics.mean(Float32, arr, dims=1)throwsKernelError#611).This PR does what Statistics.jl does: convert each mapped element to the floating-point type of
the result, sum, then divide. Statistics.jl takes that type from
f(first(A)), which would needscalar indexing here, so the PR infers it with
promote_opinstead. The conversion closure takesfas a static parameter whenfis a type, so the closure stays isbits. Empty inputs returnsum(f, A; dims) / 0, as in Statistics.jl.#453 as written moves the division after the sum, but keeps summing in the input type. That
brings the integer overflow to the
dimscase too. This PR replaces it.The two-argument
_mean(A, dims)methods are removed. Statistics has only called_mean(f, A, dims)since 1.9, and the package requires Julia 1.10.Fixes #819, fixes #611, closes #453.