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.

Sign up for GitHub

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

Jump to bottom

rewrite diagm with Pair's #24047

Merged
merged 2 commits into from
Oct 16, 2017
Merged

rewrite diagm with Pair's #24047

merged 2 commits into from
Oct 16, 2017

Conversation

fredrikekre
Copy link
Member

ref #23320, #23757

@kshyatt kshyatt added the domain:linear algebra Linear algebra label Oct 8, 2017
@fredrikekre fredrikekre added the kind:deprecation This change introduces or involves a deprecation label Oct 8, 2017
@StefanKarpinski
Copy link
Sponsor Member

value => index seems like the wrong ordering for this to me...

@fredrikekre
Copy link
Member Author

Yes, I agree. Will update #23757 and then fix up this.

@fredrikekre fredrikekre mentioned this pull request Oct 10, 2017
Closed
fredrikekre
fredrikekre commented Oct 11, 2017
View reviewed changes
base/linalg/dense.jl Outdated
for p in kv
inds = diagind(A, p.first)
for (i, val) in enumerate(p.second)
A[inds[i]] = val
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have this as += to be consistent with how the spdiagm works?

@fredrikekre fredrikekre changed the title [WIP] rewrite diagm with Pair's rewrite diagm with Pair's Oct 11, 2017
@fredrikekre fredrikekre mentioned this pull request Oct 12, 2017
Closed
9 tasks
Sacha0
Sacha0 reviewed Oct 14, 2017
View reviewed changes
NEWS.md Outdated
@@ -313,6 +313,8 @@ Library improvements

* REPL Undo via Ctrl-/ and Ctrl-_

* `diagm` now accepts several diagonal index/vector pairs ([#24047]).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps "pairs" -> "Pairs"?

Sacha0
Sacha0 reviewed Oct 14, 2017
View reviewed changes
base/linalg/dense.jl Outdated

Construct a matrix by placing `v` on the `k`th diagonal. This constructs a full matrix; if
you want a storage-efficient version with fast arithmetic, use [`Diagonal`](@ref) instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The note re. Diagonal might be worth retaining (beyond merely the reference)?

Sacha0
Sacha0 reviewed Oct 14, 2017
View reviewed changes
base/linalg/dense.jl
for p in kv
inds = diagind(A, p.first)
for (i, val) in enumerate(p.second)
A[inds[i]] += val
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why += rather than =?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be consistent with spdiagm, since combine in sparse defaults to +. See #24047 (comment) . Do you have another opinion about this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, do IIUC that its purpose is e.g. [sp]diagm(0 => [1, 2, 3], 0 => [4, 5, 6]) yielding [sp]diagm(0 => [5, 7, 9])? If so, cheers, and thanks! :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it is the most natural thing to do.

Sacha0
Sacha0 reviewed Oct 14, 2017
View reviewed changes
base/linalg/generic.jl Outdated
@@ -739,13 +739,13 @@ of the [`eltype`](@ref) of `A`.
julia> rank(eye(3))
3

julia> rank(diagm([1, 0, 2]))
julia> rank(0 => diagm([1, 0, 2]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rank(diagm(0 => [1, 0, 2]))?

Sacha0
Sacha0 reviewed Oct 14, 2017
View reviewed changes
test/linalg/bidiag.jl
@test Array(imag(T)) == imag(diagm(dv)) + imag(diagm(ev, uplo == :U ? 1 : -1))
@test Array(abs.(T)) == abs.(diagm(0 => dv, (uplo == :U ? 1 : -1) => ev))
@test Array(real(T)) == real(diagm(0 => dv, (uplo == :U ? 1 : -1) => ev))
@test Array(imag(T)) == imag(diagm(0 => dv, (uplo == :U ? 1 : -1) => ev))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes are a lovely illustration of how this API change improves things :).

Sacha0
Sacha0 approved these changes Oct 14, 2017
View reviewed changes
Copy link
Member

@Sacha0 Sacha0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm modulo the minor comments! Thanks @fredrikekre! :)

@Sacha0 Sacha0 merged commit 4b2b4f2 into master Oct 16, 2017
@Sacha0
Copy link
Member

Sacha0 commented Oct 16, 2017

Thanks @fredrikekre! :)

@Sacha0 Sacha0 deleted the fe/diagm-pair branch October 16, 2017 16:16
@andyferris
Copy link
Member

I was wondering if diagm(vector) really should be deprecated. Intuitively I would guess that this function was primarily designed to work with... the diagonal. Typing diagm(0 => vector) seems slightly unnecessary in this basic use case. (My understanding was some of this work happened with the thought of deprecating diagm entirely, #23320, but if we have it then it may as well be easy to use).

@StefanKarpinski
Copy link
Sponsor Member

Agree: passing a single vector for the main diagonal seems perfectly legit to me, we should keep it.

@cormullion cormullion mentioned this pull request Mar 18, 2018
Closed
@ararslan ararslan mentioned this pull request Feb 19, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sacha0 Sacha0 Awaiting requested review from Sacha0

Assignees
No one assigned
Labels
domain:linear algebra Linear algebra kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@fredrikekre @StefanKarpinski @Sacha0 @andyferris @kshyatt