-
Notifications
You must be signed in to change notification settings - Fork 152
diagm 0.7-style #397
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
diagm 0.7-style #397
Conversation
Add `diagm(k1 => v1, k2 => v2, ...)` style interface, where the diagonals `k` are given as `Val` instances (not types!). Also use this syntax in the tests when calling `Base.diagm` to avoid deprecation warnings on Julia 0.7. The old interface is left in place, but should be deprecated/deleted eventually.
Codecov Report
@@ Coverage Diff @@
## master #397 +/- ##
=======================================
Coverage 92.42% 92.42%
=======================================
Files 37 37
Lines 2813 2813
=======================================
Hits 2600 2600
Misses 213 213
Continue to review full report at Codecov.
|
All relevant tests passed on nightly before hitting the known failure (#272 (comment)). |
I'll note that there was some discussion of re-allowing an implicit |
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.
It's great to get this working. Thanks.
src/linalg.jl
Outdated
end | ||
end | ||
|
||
# old interface, to be deprecated/deleted eventually | ||
@inline diagm(v::StaticVector, k::Type{Val{D}}=Val{0}) where {D} = diagm(k() => 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.
This method should use Val
instances on v0.7.
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.
Or I guess not... maybe we just add a deprecation immediately...
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.
Or define as is on 0.6 and as deprecated on 0.7-? I was unsure what to do here.
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.
Yeah I think you are right - if we simply deprecate this method on v0.7 everything should follow Base
and LinearAlgerbra
(and leave as-is on v0.6).
OSX failure was a timeout. |
Thanks again :) |
Add
diagm(k1 => v1, k2 => v2, ...)
style interface, where the diagonalsk
are given asVal
instances (not types!). Also use this syntax in the tests when callingBase.diagm
to avoid deprecation warnings on Julia 0.7.The old interface is left in place, but should be deprecated/deleted eventually.