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

iterator for diagonal of a matrix ? #30250

Closed
jlapeyre opened this issue Dec 3, 2018 · 14 comments · Fixed by #56175
Closed

iterator for diagonal of a matrix ? #30250

jlapeyre opened this issue Dec 3, 2018 · 14 comments · Fixed by #56175

Comments

@jlapeyre
Copy link
Contributor

jlapeyre commented Dec 3, 2018

This is so obvious that I guess it exists elsewhere, but I don't know about it. But, if not, maybe it belongs (after some modifications) in LinearAlgebra ? ... Or some other package ?

A prototype iterator over the diagonal of a matrix is in this package.

It's called diagonal. It's like diag but does very little allocation.

julia> N = 1000;

julia> m = rand(N, N);

julia> @btime sum(diag($m));         # copy
  1.718 μs (1 allocation: 7.94 KiB)

julia> @btime sum(diagonal($m));    # no copy
  936.793 ns (1 allocation: 32 bytes)

Off diagonals are not yet supported.

@yuyichao
Copy link
Contributor

yuyichao commented Dec 3, 2018

If you just want a iterator diagonal(m) = (m[i, i] for i in 1:checksquare(m))?

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 3, 2018

diagonal(m) = (m[i, i] for i in 1:checksquare(m))

Yes. I'm thinking of something like that. But,

  1. For users coming from, say, MATLAB, you can just say that diagonal(m) gets you the diagonal (maybe with some caveats in the fine print)

  2. The prototype I pointed to is constructed slightly faster. It supports getindex, setindex!, ==, etc. (maybe you get some of that by subtyping on AbstractVector, I did not do that.) If it's in a package, it can be modified, improved, debugged, etc.

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 3, 2018

I see (JuliaLang/LinearAlgebra.jl#542) that formerly diag sometimes returned a copy and sometimes a view. It was changed so that it always returns a copy. So, diagonal would be complementary; it always returns a view.

@ararslan
Copy link
Member

ararslan commented Dec 4, 2018

To always return a view, you could just use view(A, diagind(A, k)) (for the kth diagonal).

julia> A = rand(5, 5);

julia> diagonal(A::AbstractMatrix, k::Integer=0) = view(A, diagind(A, k))
diagonal (generic function with 2 methods)

julia> diagonal(A)
5-element view(::Array{Float64,1}, 1:6:25) with eltype Float64:
 0.46312443951994453
 0.9667117622181021 
 0.08385355771707848
 0.2588063486420449 
 0.7049707946935773 

Since it's just a SubArray, it supports all of those things like indexing, equality, mutating elements, etc.

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 4, 2018

That may be the best choice. I put a note about it in the README. I'll probably replace the existing code with your one liner.

UPDATE: Thanks @ararslan . Your submission is the winner

@mbauman
Copy link
Member

mbauman commented Dec 4, 2018

So I guess the actionable question is if we want a view version of diag in base Julia or the LinearAlgebra stdlib?

@ararslan
Copy link
Member

ararslan commented Dec 4, 2018

I almost kind of think that diag should always return a view, then people can call copy if they need a copy. That is potentially disruptive and breaking though.

@KristofferC
Copy link
Member

I think the lazy adjoint has shown us that we should not underestimate how many new methods that need to be implemented for lazy wrappers to not hit the slow AbstractArray fallback. We still have multiple regressions from that, so I would personally think that holding off on more lazy wrappers for a while is perhaps for the best?

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 4, 2018

I almost kind of think that diag should always return a view, then people can call copy if they need a copy. That is potentially disruptive and breaking though.

It also brings up again the question of how non-coders react to this. Julia the language vs. Julia the better MATLAB-like tool. I think (have no statistics) that many are upset with the disappearance of eye. It's too late now, but pre v1.0, I would have said that if eye goes, so does diag.

Having just copy(diagonal(m)) or copy(eye(n)) (or Eye, or whatever) is obviously more orthogonal. Maybe the question is whether 1) with enough documentation, examples, culture change, etc., everyone finds copy(x) natural. (umm, or another verb, but that's another question) or 2) One could have a package that gives people the matlab (or python or whatever) -like things that they miss. A high-quality package that people will trust.

The language is complicated enough that there is an argument for the core language and stdlib to have a set of composable pieces that is minimal, yet usable. For instance, Matrix{T}(I, n, n) is actually very confusing to some casual (and not so casual) coders. Maybe Eye should go in stdlib.

I don't have the breadth of perspective that others in this thread do. This summer, I heard Jeff say "whatever the question, putting it in base is not the solution".

Given all this, my answer to @mbauman's question is put it in stdlib.

And in answer to to @KristofferC, yeah. Leave it in a package for a while, and let things shake out. Then, maybe stdlib.

@jlapeyre
Copy link
Contributor Author

jlapeyre commented Dec 4, 2018

There are 19 methods (just in base and stdlib) for diag. The default method, for AbstractMatrix, calls diagind. There are only generic methods for diagind. The current implementation of diagonal, the iterator, calls diagind. So copy(diagonal(m)) is currently not a generic replacement.

@mbauman
Copy link
Member

mbauman commented Dec 4, 2018

One thing I've considered in the past is that this could (or even should) be expressed via indexing. I often just use A[1:size(A, 1)+1:end] for this computation without giving it a first-class name. But since I is an "array-like" object of booleans that expands to the correct size, we could even implement this as the logical indexing expression A[I] — that is, A[LinearAlgebra.I], not some generic placeholder I. That makes the view/copy dichotomy crystal clear. Easy to implement, too:

julia> Base.to_index(A::AbstractArray, ::UniformScaling{Bool}) = diagind(A, 0)

julia> A = reshape(1:25, 5, 5);

julia> A[I]
5-element Array{Int64,1}:
  1
  7
 13
 19
 25

julia> @view A[I]
5-element view(::UnitRange{Int64}, 1:6:25) with eltype Int64:
  1
  7
 13
 19
 25

If this is a common thing folks reach for, we should probably specialize diagind(::SparseMatrix, …) to return a range of appropriate cartesian indices — it'd take a new type, but that's simple enough.

@ararslan
Copy link
Member

ararslan commented Dec 4, 2018

I like that, but then you don't have a way to specify other diagonals. (Though I guess you could just use good ol' diagind(A, k) there.)

@cbartondock
Copy link

cbartondock commented Jan 27, 2021

I think the fastest way in 2021 to get the diagonal of m is m[CartesianIndex.(axes(m,1),axes(m,2))]

@jishnub
Copy link
Contributor

jishnub commented Jul 12, 2023

Ideally, the indices may be a StepRangeLen of CartesianIndexes if #50457 is approved. Currently, this allocates a Vector:

julia> m = Diagonal(1:4)
4×4 Diagonal{Int64, UnitRange{Int64}}:
 1      
   2    
     3  
       4

julia> CartesianIndex.(axes(m)...)
4-element Vector{CartesianIndex{2}}:
 CartesianIndex(1, 1)
 CartesianIndex(2, 2)
 CartesianIndex(3, 3)
 CartesianIndex(4, 4)

jishnub added a commit that referenced this issue Nov 11, 2024
A function to obtain a view of a diagonal of a matrix is useful, and
this is clearly being used widely within `LinearAlgebra`.

The implementation here iterates according to the `IndexStyle` of the
array:
```julia
julia> using LinearAlgebra

julia> A = reshape(1:9, 3, 3)
3×3 reshape(::UnitRange{Int64}, 3, 3) with eltype Int64:
 1  4  7
 2  5  8
 3  6  9

julia> diagview(A,1)
2-element view(::UnitRange{Int64}, 4:4:8) with eltype Int64:
 4
 8

julia> T = Tridiagonal(1:3, 3:6, 4:6)
4×4 Tridiagonal{Int64, UnitRange{Int64}}:
 3  4  ⋅  ⋅
 1  4  5  ⋅
 ⋅  2  5  6
 ⋅  ⋅  3  6

julia> diagview(T,1)
3-element view(::Tridiagonal{Int64, UnitRange{Int64}}, StepRangeLen(CartesianIndex(1, 2), CartesianIndex(1, 1), 3)) with eltype Int64:
 4
 5
 6
```

Closes #30250
KristofferC pushed a commit to JuliaLang/LinearAlgebra.jl that referenced this issue Nov 14, 2024
A function to obtain a view of a diagonal of a matrix is useful, and
this is clearly being used widely within `LinearAlgebra`.

The implementation here iterates according to the `IndexStyle` of the
array:
```julia
julia> using LinearAlgebra

julia> A = reshape(1:9, 3, 3)
3×3 reshape(::UnitRange{Int64}, 3, 3) with eltype Int64:
 1  4  7
 2  5  8
 3  6  9

julia> diagview(A,1)
2-element view(::UnitRange{Int64}, 4:4:8) with eltype Int64:
 4
 8

julia> T = Tridiagonal(1:3, 3:6, 4:6)
4×4 Tridiagonal{Int64, UnitRange{Int64}}:
 3  4  ⋅  ⋅
 1  4  5  ⋅
 ⋅  2  5  6
 ⋅  ⋅  3  6

julia> diagview(T,1)
3-element view(::Tridiagonal{Int64, UnitRange{Int64}}, StepRangeLen(CartesianIndex(1, 2), CartesianIndex(1, 1), 3)) with eltype Int64:
 4
 5
 6
```

Closes JuliaLang/julia#30250
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 a pull request may close this issue.

7 participants