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

Additional LowerTriangularArray functionality #558

Open
milankl opened this issue Jul 5, 2024 · 17 comments · Fixed by #562
Open

Additional LowerTriangularArray functionality #558

milankl opened this issue Jul 5, 2024 · 17 comments · Fixed by #562
Labels
array types 🔢 LowerTriangularMatrices and RingGrids user interface 🎹 How users use our user interface

Comments

@milankl
Copy link
Member

milankl commented Jul 5, 2024

Spillover functionality from #543 that we could consider implementing or, alternatively, this is an issue to document the stuff that doesn't work but that user might expect to work

  1. Indexing with trailing colon :
julia> L[1, :]
ERROR: MethodError: no method matching isless(::Int64, ::Colon)
  1. Indexing with end
julia> L[:, end]
3-element Vector{Float64}:
 1.0
 1.0
 1.0

should be 0.0, 0.0, ..., 1.0 (= zeros for the upper triangle)

  1. Broadcasting of vector with LowerTriangularMatrix
k = 1:32
A = randn(LowerTriangularMatrix{Complex{Float32}}, 32, 32)
A .*= k.^-2

doesn't work because A and k are technically both vectors of different sizes (560 for A vs 32 for k)

@milankl milankl added user interface 🎹 How users use our user interface array types 🔢 LowerTriangularMatrices and RingGrids labels Jul 5, 2024
@milankl
Copy link
Member Author

milankl commented Jul 6, 2024

  1. LowerTriangularMatrix as ones, zeros, rand, randn generator for any size

Currently this fails

julia> L = zeros(LowerTriangularMatrix{ComplexF32}, 5, 5, 1);
ERROR: MethodError: no method matching Vector{ComplexF32}(::Matrix{ComplexF32})

Although zeros(LowerTriangularMatrix{ComplexF32}, 5, 5) works fine. One needs to use Array for any higher dimensions, which I find confusing.

@milankl
Copy link
Member Author

milankl commented Jul 6, 2024

  1. .= between Array and LowerTriangularArray
julia> L = zeros(LowerTriangularArray{ComplexF32}, 5, 5)
15-element, 5x5 LowerTriangularMatrix{ComplexF32}
 0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im
 0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im
 0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im
 0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im
 0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im  0.0+0.0im

julia> M = randn(ComplexF32, size(L)...)
15-element Vector{ComplexF32}:
 -0.020682817f0 + 0.41503236f0im
    0.7638009f0 - 0.2483163f0im
   0.25317496f0 - 0.17696327f0im
   -1.3516215f0 + 0.16562235f0im
   0.31752998f0 + 0.42242694f0im
  -0.22476453f0 - 0.48526806f0im
    0.8692698f0 - 0.13144933f0im
   0.28967917f0 + 0.692557f0im
    0.4887116f0 - 0.14684263f0im
   -0.5528534f0 + 0.6710106f0im
   0.23252673f0 - 0.54855406f0im
 -0.066379264f0 - 0.30891386f0im
  -0.12736541f0 - 0.067103244f0im
    1.1546654f0 + 1.010668f0im
  -0.83623916f0 - 0.08629082f0im

julia> L .= M
ERROR: BoundsError
Stacktrace:
 [1] copyto!(L::LowerTriangularMatrix{ComplexF32}, M::Vector{ComplexF32})
   @ Main.SpeedyWeather.LowerTriangularMatrices ~/git/SpeedyWeather.jl/src/LowerTriangularMatrices/lower_triangular_matrix.jl:481

@maximilian-gelbrecht
Copy link
Member

#561 adresses point 5

I don't think point 2 is realistically doable. Making end work simultaneously for vector and matrix-style indexing would require a lot of work, and I am not sure it's even possible.

@milankl
Copy link
Member Author

milankl commented Jul 8, 2024

I don't think point 2 is realistically doable. Making end work simultaneously for vector and matrix-style indexing would require a lot of work, and I am not sure it's even possible.

Yes that one is more to list the issue not to resolve it.

@maximilian-gelbrecht
Copy link
Member

  1. LowerTriangularMatrix as ones, zeros, rand, randn generator for any size

Currently this fails

julia> L = zeros(LowerTriangularMatrix{ComplexF32}, 5, 5, 1);
ERROR: MethodError: no method matching Vector{ComplexF32}(::Matrix{ComplexF32})

Although zeros(LowerTriangularMatrix{ComplexF32}, 5, 5) works fine. One needs to use Array for any higher dimensions, which I find confusing.

Do you just want the generators to be the same for LowerTriangularMatrix{ComplexF32} and LowerTriangularArray{ComplexF32}?

@maximilian-gelbrecht
Copy link
Member

  1. Broadcasting of vector with LowerTriangularMatrix
k = 1:32
A = randn(LowerTriangularMatrix{Complex{Float32}}, 32, 32)
A .*= k.^-2

doesn't work because A and k are technically both vectors of different sizes (560 for A vs 32 for k)

I am surprised this even works for regular arrays to be honest. For me it's not a well defined operation, but yeah, it does it row-wise multiplication.

I had a brief look but I couldn't find where this is implemented for Array or CuArray

@maximilian-gelbrecht
Copy link
Member

I don't think point 2 is realistically doable. Making end work simultaneously for vector and matrix-style indexing would require a lot of work, and I am not sure it's even possible.

Yes that one is more to list the issue not to resolve it.

I added a warn note to the documentation about this in #561

@milankl
Copy link
Member Author

milankl commented Jul 8, 2024

Do you just want the generators to be the same for LowerTriangularMatrix{ComplexF32} and LowerTriangularArray{ComplexF32}?

I think so, the size should just be given my arguments. I think we have the same for <:AbstractGrid vs <:AbstractGridArray
You agree?

@maximilian-gelbrecht
Copy link
Member

maximilian-gelbrecht commented Jul 8, 2024

Ok, done. Also added to #561

@maximilian-gelbrecht
Copy link
Member

I accidentally closed this with #562 being merged.

We can keep this issue, in case we still want to tackle the broadcasting issue 3)

@milankl
Copy link
Member Author

milankl commented Jul 9, 2024

Yes, or just to collect similar issues in the future, I bet there might be a few more coming in the next weeks.

@maximilian-gelbrecht
Copy link
Member

When we have some spare time, it would great to implement more support for mixed indexing with ranges and colon. Like given a LxMxN LTA, currently you can't do L[1:10,1,:].

@milankl
Copy link
Member Author

milankl commented Nov 13, 2024

Yes, also this one is not intuitive

julia> L = randn(LowerTriangularMatrix, 5, 5)
15-element, 5x5 LowerTriangularMatrix{Float64}
 -1.27161     0.0        0.0       0.0      0.0
  0.666496   -0.788112   0.0       0.0      0.0
 -0.800069   -1.83198    0.928561  0.0      0.0
  0.0481627  -1.80951    0.953103  0.32636  0.0
  0.166488   -0.172987  -3.3219    1.77804  0.219669

julia> L[2:4, :] .= 0
3×1 view(reshape(::LowerTriangularMatrix{Float64}, 15, 1), 2:4, :) with eltype Float64:
 0.0
 0.0
 0.0

julia> L
15-element, 5x5 LowerTriangularMatrix{Float64}
 -1.27161    0.0        0.0       0.0      0.0
  0.0       -0.788112   0.0       0.0      0.0
  0.0       -1.83198    0.928561  0.0      0.0
  0.0       -1.80951    0.953103  0.32636  0.0
  0.166488  -0.172987  -3.3219    1.77804  0.219669

and actually led to a bug in our definition or the RandomWaves initial conditions

@milankl
Copy link
Member Author

milankl commented Nov 13, 2024

Note to self to write :, 2 as

@inline function Base.getindex(L::LowerTriangularMatrix, ::Colon, j::Integer)
    k1 = ij2k(j, j, L.m)
    kend = ij2k(L.m, j, L.m)
    return vcat(zeros(eltype(L), j-1), L.data[k1:kend])
end

to avoid the matrix allocation

@milankl
Copy link
Member Author

milankl commented Nov 29, 2024

#618 adds getindex for [l, m, k] with l, m integers but k::CartesianIndex

@milankl
Copy link
Member Author

milankl commented Dec 13, 2024

[:, end:-1:1] also escapes for RingGrids, see here #630 (comment)

@maximilian-gelbrecht
Copy link
Member

[:, end:-1:1] also escapes for RingGrids, see here #630 (comment)

Addressed by #637

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
array types 🔢 LowerTriangularMatrices and RingGrids user interface 🎹 How users use our user interface
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants