Rewrite spdiagm with pairs #23757

fredrikekre · 2017-09-18T18:24:56Z

I think this is nicer than the previous spdiagm(tuple_of_vectors, tuple_of_diags) API. And the same should also be implemented for diagm. See #23320 and specifically #23320 (comment)

This also deals with #23320 (comment) such that we always return a square matrix.

Some things to decide:

Should the pair be vector => diagonal or diagonal => vector? I think vector => diagonal is nicer.
Should we have a convenience constructor for the single vector case? spdiagm(v::AbstractVector) = spdiagm(0 => v). I imagine that case being quite common and it might be nice to have that?

Still need to implement proper deprecations and such.

fredrikekre · 2017-09-19T06:43:04Z

Actually, the single vector case could better be written as sparse(Diagonal(v)), that should also be faster.

fredrikekre · 2017-09-28T20:48:34Z

Here are some datapoints that support the changes here, specifically the removal of the option to supply m, n as the final size. In all registered packages there are ≈120 uses of spdiagm as follows:

69 % are simply spdiagm(x::AbstractVector)

21 % are calls to spdiagm(x::AbstractVector, k, m, n) where m, n are needed to obtain a square matrix.

julia> spdiagm(rand(2), -1)
3×2 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
  [2, 1]  =  0.0960447
  [3, 2]  =  0.190498

julia> spdiagm(rand(2), -1, 3, 3) # 3, 3 needed to obtain a square matrix
3×3 SparseMatrixCSC{Float64,Int64} with 2 stored entries:
  [2, 1]  =  0.64661
  [3, 2]  =  0.119494

5.9% are calls with multiple diagonals that will be unaffected of the removal of m, n arguments
4.2 % are calls where m != n, e.g. requesting a non-square matrix.

Sacha0 · 2017-10-01T03:58:33Z

base/deprecated.jl

+@deprecate spdiagm(x::AbstractVector) sparse(Diagonal(x))
+@deprecate spdiagm(x::AbstractVector, d::Number) spdiagm(x => d)
+@deprecate spdiagm(x, d) spdiagm((x[i] => d[i] for i in 1:length(x))...)
+@deprecate spdiagm(x, d, m::Integer, n::Integer) spdiagm((x[i] => d[i] for i in 1:length(x))...)


IIUC, the latter now always yields a square matrix whereas the former yields an m-by-n matrix?

Yea. I should rewrite these deprecations such that this change won't be breaking.

Sacha0 · 2017-10-01T04:11:30Z

base/sparse/sparsematrix.jl

        i += numel
    end
-
-    return (I,J,V)
+    return I, J, V
 end


The former descriptive variable names made this code a bit easier to follow :).

Sacha0 · 2017-10-01T04:12:10Z

base/sparse/sparsematrix.jl

-one diagonal, `B` can be a vector (instead of a tuple) and `d` can be the diagonal position
-(instead of a tuple), defaulting to 0 (diagonal). Optionally, `m` and `n` specify the size
-of the resulting sparse matrix.
+Construct a sparse diagonal matrix from pairs of vectors and diagonals.


"pairs" -> "Pairs"?

ViralBShah · 2017-10-01T19:21:06Z

Off topic, I also wish there was a better way to name this and similar functions. It is really annoying to have the sp prefixes.

StefanKarpinski · 2017-10-02T17:32:46Z

It is really annoying to have the sp prefixes.

The obvious solution would be something like diagm(SparseMatrixCSC, ...).

ViralBShah · 2017-10-02T18:53:51Z

diagm(SparseMatrixCSC, ...) is what I thought of too, but it feels too verbose. One other possibility is using the Sparse module and calling Sparse.diagm.

fredrikekre · 2017-10-02T19:30:04Z

That's what I proposed in #23320 and also what we should probably do, see #11557 and #16740. There is currently no API to extend for user arrays for this kind of thing, e.g. zeros, ones, rand etc. But this is unrelated to the current PR.

Sacha0 · 2017-10-06T17:09:28Z

base/deprecated.jl

+@deprecate spdiagm(x::AbstractVector) sparse(Diagonal(x))
+function spdiagm(x::AbstractVector, d::Number)
+    depwarn(string("spdiagm(x::AbstractVector, d::Number) is deprecated, use ",
+        "spdiagm(x => d) instead, which now return a square matrix. To preserve the old ",


"return" -> "returns"?

Sacha0 · 2017-10-06T17:11:27Z

base/deprecated.jl

+end
+function spdiagm(x, d)
+    depwarn(string("spdiagm((x1, x2, ...), (d1, d2, ...)) is deprecated, use ",
+        "spdiagm(x1 => d1, x2 => d2, ...) instead, which now return a square matrix. ",


"return" => "returns"?

Sacha0 · 2017-10-06T17:12:40Z

base/deprecated.jl

+end
+function spdiagm(x, d, m::Integer, n::Integer)
+    depwarn(string("spdiagm((x1, x2, ...), (d1, d2, ...), m, n) is deprecated, use ",
+        "spdiagm(x1 => d1, x2 => d2, ...) instead, which now return a square matrix. ",


"return" -> "returns"?

Sacha0

These changes strike me as a step forward, independent of where diagm and friends go eventually. Much thanks for thinking through the design here and in upstream threads @fredrikekre! :)

A minor, subjective point: I find k => v more natural than v => k, particularly with the pairs consistently identified as kv. But I'm sure this impression varies, and practically the difference is likely negligible :).

fredrikekre · 2017-10-07T09:15:56Z

A minor, subjective point: I find k => v more natural than v => k, particularly with the pairs consistently identified as kv. But I'm sure this impression varies, and practically the difference is likely negligible :).

I read v => k as "assign vector v to diagonal k" which I feel is more natural compared to "assign diagonal k to vector v" for k => v.
The reason it is currently called kv is that I implemented as k => v first and then changed. We should definitely change this to match whatever we decide.

Sacha0 · 2017-10-07T16:36:16Z

Either orientation sounds good on this end :).

fredrikekre · 2017-10-07T21:43:56Z

Either orientation sounds good on this end :).

I don't particularly care either. Perhaps other people have some opinions?

mschauer · 2017-10-08T08:33:39Z

A matrix can be viewed as an association mapping a number (the k in kth diagonal) to a vector (the kth diagonal). In this light, diagm(k =>d) would be more natural.

martinholters · 2017-10-10T10:38:14Z

Also, the same vector might be used for different diagonals, but not vice versa.

instead of using one tuple with diagonals and one tuple with vectors

fredrikekre · 2017-10-10T10:51:45Z

Should be ready now. Wanna take another look @Sacha0 ?

Sacha0

lgtm from a rapid skim of this iteration! :)

stevengj · 2019-03-22T02:35:16Z

Do we still have to call SparseArrays.spdiagm_internal in order to get a non-square matrix?

kshyatt added the sparse Sparse arrays label Sep 19, 2017

kshyatt requested a review from Sacha0 September 19, 2017 16:41

fredrikekre force-pushed the fe/spdiagm-pair branch from 2be114e to 6e0d456 Compare September 22, 2017 21:52

Sacha0 reviewed Oct 1, 2017

View reviewed changes

fredrikekre force-pushed the fe/spdiagm-pair branch from 6e0d456 to 3b107ae Compare October 3, 2017 20:27

fredrikekre changed the title ~~[WIP] Rewrite spdiagm with pairs~~ Rewrite spdiagm with pairs Oct 3, 2017

fredrikekre requested a review from Sacha0 October 3, 2017 20:27

fredrikekre mentioned this pull request Oct 3, 2017

diagm and spdiagm #23320

Closed

9 tasks

Sacha0 reviewed Oct 6, 2017

View reviewed changes

fredrikekre mentioned this pull request Oct 8, 2017

rewrite diagm with Pair's #24047

Merged

rewrite spdiagm with Pair API

f07157b

instead of using one tuple with diagonals and one tuple with vectors

fredrikekre force-pushed the fe/spdiagm-pair branch from 3b107ae to f07157b Compare October 10, 2017 10:51

fredrikekre requested a review from Sacha0 October 10, 2017 10:52

Sacha0 approved these changes Oct 10, 2017

View reviewed changes

KristofferC approved these changes Oct 11, 2017

View reviewed changes

KristofferC merged commit 5295feb into master Oct 11, 2017

KristofferC deleted the fe/spdiagm-pair branch October 11, 2017 07:35

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

stevengj mentioned this pull request Apr 9, 2019

diagm for non-square matrices #31654

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite spdiagm with pairs #23757

Rewrite spdiagm with pairs #23757

fredrikekre commented Sep 18, 2017 •

edited

Loading

fredrikekre commented Sep 19, 2017

fredrikekre commented Sep 28, 2017

Sacha0 Oct 1, 2017

fredrikekre Oct 1, 2017

Sacha0 Oct 1, 2017

Sacha0 Oct 1, 2017

ViralBShah commented Oct 1, 2017 •

edited

Loading

StefanKarpinski commented Oct 2, 2017

ViralBShah commented Oct 2, 2017

fredrikekre commented Oct 2, 2017

Sacha0 Oct 6, 2017

Sacha0 Oct 6, 2017

Sacha0 Oct 6, 2017

Sacha0 left a comment

fredrikekre commented Oct 7, 2017

Sacha0 commented Oct 7, 2017

fredrikekre commented Oct 7, 2017

mschauer commented Oct 8, 2017

martinholters commented Oct 10, 2017

fredrikekre commented Oct 10, 2017

Sacha0 left a comment

stevengj commented Mar 22, 2019

Rewrite spdiagm with pairs #23757

Rewrite spdiagm with pairs #23757

Conversation

fredrikekre commented Sep 18, 2017 • edited Loading

fredrikekre commented Sep 19, 2017

fredrikekre commented Sep 28, 2017

Sacha0 Oct 1, 2017

Choose a reason for hiding this comment

fredrikekre Oct 1, 2017

Choose a reason for hiding this comment

Sacha0 Oct 1, 2017

Choose a reason for hiding this comment

Sacha0 Oct 1, 2017

Choose a reason for hiding this comment

ViralBShah commented Oct 1, 2017 • edited Loading

StefanKarpinski commented Oct 2, 2017

ViralBShah commented Oct 2, 2017

fredrikekre commented Oct 2, 2017

Sacha0 Oct 6, 2017

Choose a reason for hiding this comment

Sacha0 Oct 6, 2017

Choose a reason for hiding this comment

Sacha0 Oct 6, 2017

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

fredrikekre commented Oct 7, 2017

Sacha0 commented Oct 7, 2017

fredrikekre commented Oct 7, 2017

mschauer commented Oct 8, 2017

martinholters commented Oct 10, 2017

fredrikekre commented Oct 10, 2017

Sacha0 left a comment

Choose a reason for hiding this comment

stevengj commented Mar 22, 2019

fredrikekre commented Sep 18, 2017 •

edited

Loading

ViralBShah commented Oct 1, 2017 •

edited

Loading