-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Make cov
's corrected argument a keyword argument and cleanup docstrings for cov
and cor
#21709
Conversation
worth an 0.7 news entry? |
Sure. I've added a 0.7. section. |
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.
LGTM
Why not the other way around? Have you measured the overhead? |
What do you mean by the other way around? |
Make |
I thought the trend was rather to move to keyword arguments where appropriate (since we won't be able to change this after 1.0), and improve their performance if needed. Maybe @StefanKarpinski can confirm? Clearly, the overhead will depend a lot on the size of the input vector/matrix. For a relatively small vector with 1000 entries, it's about 14%: covzm2(x::AbstractVector; corrected::Bool=true) = Base.unscaled_covzm(x) / (Base._length(x) - Int(corrected))
covm2(x::AbstractVector, xmean; corrected::Bool=true) = covzm2(x .- xmean; corrected=corrected)
cov2(x::AbstractVector; corrected::Bool=true) = covm2(x, Base.mean(x); corrected=corrected)
using BenchmarkTools
@benchmark cov(X)
@benchmark cov2(X) BTW, if we care about performance, we should get rid of |
@StefanKarpinski What's your position WRT keyword arguments? |
I think that unnamed boolean arguments are not nearly as self-documenting as keywords. If you see |
@JeffBezanson can verify, but I think we have semiconcrete plans to make keywords more efficient in 1.0 and should therefore not be overly worried about the efficiency gap between positional and keyword argument forms and choose based on better APIs, not performance. |
OK, thanks. Should be good to go then. |
Barring objections I'll merge this tomorrow. |
cov(x::AbstractVector, Y::AbstractMatrix) = cov(x, Y, 1, true) | ||
cov(X::AbstractMatrix, y::AbstractVector) = cov(X, y, 1, true) | ||
cov(X::AbstractMatrix, Y::AbstractMatrix) = cov(X, Y, 1, true) | ||
cov(X::AbstractVecOrMat, Y::AbstractVecOrMat, vardim::Int=1; corrected::Bool=true) = |
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.
Is this one inferred?
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.
All of these work:
@inferred cov([1, 2], [1, 2])
@inferred cov([1 2], [1 2], 1)
@inferred cov([1, 2], [1; 2])
@inferred cov([1; 2], [1, 2])
Did I miss other cases to test?
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 don't think so
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.
Good to go then?
…ings For consistency with var and std. Also remove methods which are no longer needed now that deprecations have been removed. Add types to signatures in docstrings.
Remove methods which are no longer needed now that deprecations have been removed. Add types to signatures in docstrings.
|
For consistency with var and std. Also remove methods which are no longer needed
now that deprecations have been removed. Add types to signatures in docstrings.
This only changes the public API for
cov
. The changes tocor
are cosmetic.