Skip to content

rewrite diagm with Pair's #24047

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

@fredrikekre fredrikekre commented Oct 8, 2017

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

StefanKarpinski commented Oct 9, 2017

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

@fredrikekre
Copy link
Member Author

fredrikekre commented Oct 10, 2017

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

@fredrikekre fredrikekre Oct 11, 2017

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
@fredrikekre fredrikekre requested a review from Sacha0 October 13, 2017 06:59
Sacha0
Sacha0 reviewed Oct 14, 2017
View reviewed changes
NEWS.md Outdated

* REPL Undo via Ctrl-/ and Ctrl-_

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

@Sacha0 Sacha0 Oct 14, 2017

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
Vector `kv.second` will be placed on the `kv.first` diagonal.
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

@Sacha0 Sacha0 Oct 14, 2017

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

@Sacha0 Sacha0 Oct 14, 2017

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

@fredrikekre fredrikekre Oct 15, 2017

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

@Sacha0 Sacha0 Oct 15, 2017

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

@fredrikekre fredrikekre Oct 15, 2017

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
3
julia> rank(diagm([1, 0, 2]))
julia> rank(0 => diagm([1, 0, 2]))
Copy link
Member

@Sacha0 Sacha0 Oct 14, 2017

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

@Sacha0 Sacha0 Oct 14, 2017

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! :)

@fredrikekre fredrikekre requested a review from Sacha0 October 16, 2017 05:28
@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

andyferris commented Jan 21, 2018

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, JuliaLang/LinearAlgebra.jl#457, but if we have it then it may as well be easy to use).

@StefanKarpinski
Copy link
Member

StefanKarpinski commented Jan 21, 2018

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

deprecation This change introduces or involves a deprecation linear algebra Linear algebra

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@fredrikekre @StefanKarpinski @Sacha0 @andyferris @kshyatt