-
Notifications
You must be signed in to change notification settings - Fork 33
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
Power spectrum for n-dimensional LowerTriangularArray #618
Conversation
@@ -221,6 +221,14 @@ end | |||
return getindex(L.data, k, I[3:end]...) | |||
end | |||
|
|||
@inline function Base.getindex(L::LowerTriangularArray{T, N}, i::Integer, j::Integer, c::CartesianIndex) where {T, N} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maximilian-gelbrecht I've added this method because L[l, m, k]
is quite a common access pattern arising in
for k in eachmatrix(spec)
for m in 1:mmax
for l in m:lmax
L[l, m, k]
to loop over integers l, m
but any additional dimensions k
(eachmatrix
loops over all of those, returning a CartesianIndex
. I added the type contraints of i::Integer, j::Integer, c::CartesianIndex
to avoid any dispatch issues, but not sure they're actually needed, e.g. i, j, c::CartesianIndex
could work equally well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i, j, c::CartesianIndex
might cause some problems with the getindex
functions that use Vararg
. I could be mistaking though.
Anyway, good to a function like this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out this method wasn't needed, but we had a @boundscheck M == N+1
in getindex with varargs which you added to guarantee that someones wants to do matrix indexing with e.g. 2 indices on a LowerTriangularMatrix. But checking this from the number of indices isn't helpful because L[1, 1, 1, 1, 1, 1]
is the same as L[1]
regardless the shape of L
and also you can add many trailing CartesianIndex()
that would increase the number of indices even though they don't actually count. Just removed that @boundscheck
to see what happens, I've tested it manually it doesn't seem necessary!
This allows to calculate the power spectrum on n-dimensional LowerTriangularArrays along the first/spherical harmonic dimension, e.g.