-
Notifications
You must be signed in to change notification settings - Fork 25
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
Broadcast operations with LowerTriangularMatrix #135
Comments
@giordano just raised another related issue julia> A = LowerTriangularMatrix(rand(3,3))
3×3 LowerTriangularMatrix{Float64}:
0.382579 0.0 0.0
0.554634 0.779331 0.0
0.453919 0.236713 0.559563
julia> sum(A)
2.966738421294002
julia> prod(A)
0.009942566052321158 The sum is correct, but the product should obviously be zero. |
Is it a bug or a feature? It's currently implemented solely with spherical harmonics coefficients in mind and for those If the product of the full matrix is meant, yes of course that's always zero, but then one could also just add Base.prod(A::LowerTriangularMatrix{T}) where T = zero(T) |
To original problem: I never customised |
There are also Here's the manual on it PS: It's still fascinating to me how customizable Julia is I made a quick attempt and just added Base.BroadcastStyle(::Type{<:LowerTriangularMatrix}) = Broadcast.DefaultArrayStyle{1} but that's not working, we'll have to look a bit deeper into that. Your suspicion about the created Matrix should be correct though, as the manual states, this is indeed the default fallback. But the |
But that's a different operation than the product of all the elements of an array. Changing the semantic of a function can cause surprising and confusing results. |
Rename But whatever, I have no strong feelings about this. Add Base.prod(A::LowerTriangularMatrix{T}) where T = zero(T) and you have the correct answer for the full 2D matrix that's consistent with using them for general purposes. I'll have a bit further look in the broadcasting issue in the next days. |
So, a hacky way to fix it, is just to make For a way how to fix this and keep it a subtype of
The manual gives one concrete example for this, but especially for the |
Thanks for looking into this!
I tried that earlier, and thought it was a good idea as 2D indexing then comes as a bonus rather than assumed to exist. However, I ran into even more problems down the line, which gave me the feeling that I'd suggest to try first changing the broadcasting and if that imposes even bigger obstacles then we can try |
I give up. No helpful replies on Slack and I tried a lot of different variations of what's given in the manual. Doesn't seem to work. So I'd change it to @milankl The printing / show will be no problem to adjust. There are one or two routines that need slight adjustment that assume Overall there a few routines that allow spectral input to be an |
I've added a method like function gridded( alms::AbstractMatrix, # spectral coefficients
S::SpectralTransform{NF}, # struct for spectral transform parameters
) where NF # number format NF
map = zeros(S.Grid{NF},S.nresolution) # preallocate output
almsᴸ = LowerTriangularMatrix{Complex{NF}}(alms) # drop the upper triangle and convert to NF
gridded!(map,almsᴸ,S) # now execute the in-place version
return map
end So that someone can also come with a |
Thanks for trying!! Then let's do |
The only request I have it that we retain show similar to this julia> rand(LowerTriangularMatrix,4,4)
4×4 LowerTriangularMatrix{Float64}:
0.0025516 0.0 0.0 0.0
0.2128 0.906056 0.0 0.0
0.589329 0.0822436 0.0122374 0.0
0.865687 0.838233 0.741264 0.685711 As otherwise troubleshooting with spectral fields isn't easy... |
In case we ever come back to this, to remind myself and archive it, this was my best effort so far: # Broadcasting
import Base.BroadcastStyle, Base.similar
struct LowerTriangularStyle <: Broadcast.AbstractArrayStyle{1} end
Base.BroadcastStyle(::Type{<:LowerTriangularMatrix}) = LowerTriangularStyle()
Base.similar(bc::Broadcast.Broadcasted{LowerTriangularStyle}, ::Type{ElType}) where ElType = similar(LowerTriangularMatrix{Eltype}, axes(bc))
LowerTriangularStyle(::Val{0}) = LowerTriangularStyle()
LowerTriangularStyle(::Val{1}) = LowerTriangularStyle()
LowerTriangularStyle(::Val{2}) = LowerTriangularStyle() No errors, just the same wrong results |
Broadcasting with LowerTriangularMatrix still returns unexpected results. The biggest problem I found was dotted operations like .*= , see how suddenly a zero appears where it shouldn’t
This is (I think) because it loops over each index (i,j) with an
@inbounds
such that the@boundscheck
insetindex!
is disabled and then converts i,j to a single index k which isn’t safe ifj>i
… The problem is apparently that the broadcasting of*
creates aMatrix
which is then fused with.=
and writes the entries back into aLowerTriangularMatrix
The text was updated successfully, but these errors were encountered: