-
Notifications
You must be signed in to change notification settings - Fork 193
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Preserve eltype where possible for moments #688
base: master
Are you sure you want to change the base?
Conversation
The failure on nightly is an upstream bug (JuliaLang/julia#40609). |
This looks good but the prior uses of |
Ugh, I'm conflicted on that front. Because that would be breaking, but technically a user hitting overflow on their custom narrow types is expected behavior. @nalimilan do we have enough for another breaking pre-1.0 release? |
We can break things if we want to tag 1.0. Though ideally I'd rather move these to Statistics, but that work has stalled (JuliaLang/julia#31395). |
src/moments.jl
Outdated
T = promote_type(eltype(v), eltype(wv), typeof(m)) | ||
s = zero(T) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To ensure type stability, how about following exactly what the loop does and using T = typeof(zero(eltype(v) - zero(m))^2 * zero(eltype(w)))
? That's what I did in my first attempt to moving these functions to Statistics: https://github.com/JuliaLang/julia/pull/31395/files#diff-6b82a0c4e60cbe68048e35375987a86688afb4aea60dda8f2d2d4938dc845776R375
Okay, so on the breaking / non breaking front:
In other words: given that we're not going to change the algorithm used (which I think is reasonable), the potential numeric issues from the eltype preservation should not be considered breaking. Perhaps the best solution is to add to each of the docstrings: !!! note
The computation is done in the narrowest type for which the underlying
arithmetic operations are defined based on the element types of the
vectors and the mean. For higher numerical accuracy and to reduce the risk
of overflow when dealing with values near the limits of the type (or many
values whose sum is near the limits of the type), we recommend casting to
a broader type / higher precision first. |
Yeah it's not super likely to break user code. Though we could collect the few changes which are breaking (some of them are marked with the breaking label) and take that occasion to tag 1.0. |
Co-authored-by: Milan Bouchet-Valat <[email protected]>
Fixes #680.