-
Couldn't load subscription status.
- Fork 195
Use zero() for type-stable calculation of _momentN()
#969
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
base: master
Are you sure you want to change the base?
Conversation
src/moments.jl
Outdated
| function _moment2(v::AbstractArray{<:Real}, m::Real; corrected=false) | ||
| n = length(v) | ||
| s = 0.0 | ||
| s = zero(m * m) |
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.
In general, s does not have the correct/desired type here as this initialization purely depends on m and neglects v.
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.
I was making the assumption that m and v would have compatible/identical types, since m is calculated from v and the function is only called internally. Would an implementation involving v be something like zero(v[begin] * v[begin])?
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.
IMO we shouldn't make that assumption. For instance, in var(...; mean=...) users can easily provide a mean that is of different type.
Would an implementation involving v be something like zero(v[begin] * v[begin])
No, that would error if the array is empty and would neglect the type of m.
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.
Would the most recent commit (6a236d4) work for this?
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.
No, not in general.
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.
I'll have to say, I'm confused where it would fail 😅
Solves #968