Skip to content
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

Merged
merged 2 commits into from
Apr 27, 2018
Merged

diagm 0.7-style #397

merged 2 commits into from
Apr 27, 2018

Conversation

martinholters
Copy link
Collaborator

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.

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.
@coveralls
Copy link

Coverage Status

Coverage remained the same at 92.428% when pulling e8e8dcd on martinholters:mh/diagm into 7a907c8 on JuliaArrays:master.

@codecov-io
Copy link

Codecov Report

Merging #397 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #397   +/-   ##
=======================================
  Coverage   92.42%   92.42%           
=======================================
  Files          37       37           
  Lines        2813     2813           
=======================================
  Hits         2600     2600           
  Misses        213      213
Impacted Files Coverage Δ
src/linalg.jl 97.05% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7a907c8...e8e8dcd. Read the comment docs.

@martinholters
Copy link
Collaborator Author

All relevant tests passed on nightly before hitting the known failure (#272 (comment)).

@andyferris
Copy link
Member

I'll note that there was some discussion of re-allowing an implicit 0 => ... in the single-argument diagm, since it seems convenient and not too harmful... I wonder if this likely to proceed for v1.0? (@mbauman?)

Copy link
Member

@andyferris andyferris left a 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)
Copy link
Member

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.

Copy link
Member

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...

Copy link
Collaborator Author

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.

Copy link
Member

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).

@martinholters
Copy link
Collaborator Author

OSX failure was a timeout.

@andyferris andyferris merged commit 2a74b04 into JuliaArrays:master Apr 27, 2018
@andyferris
Copy link
Member

Thanks again :)

@martinholters martinholters deleted the mh/diagm branch April 27, 2018 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants