-
Notifications
You must be signed in to change notification settings - Fork 30
Various fixes for propagating CachedArrayStyle correctly #391
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #391 +/- ##
==========================================
- Coverage 95.90% 95.81% -0.10%
==========================================
Files 17 17
Lines 3372 3466 +94
==========================================
+ Hits 3234 3321 +87
- Misses 138 145 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Similar cases missing a julia> v = Accumulate(*, 1:10);
julia> (Base.BroadcastStyle ∘ typeof).((v, unitblocks(v), BlockBroadcastArray(vcat, unitblocks(v), unitblocks(v))))
(LazyArrays.CachedArrayStyle{1}(), BlockArrays.BlockedStyle{1}(), LazyArrays.LazyArrayStyle{1}())So I might have to eventually go and fix these, assuming it would be appropriate that these all return julia> Base.BroadcastStyle(typeof(vec(Vcat(v', v')))) # interlaced
Base.Broadcast.DefaultArrayStyle{1}()will get a fix (this last one is now fixed in JuliaLinearAlgebra/ArrayLayouts.jl#268) |
This allows e.g.
```julia-repl
julia> v = Accumulate(*, 1:10); Base.BroadcastStyle(typeof(vec(Vcat(v', v'))))
LazyArrays.LazyArrayStyle{2}()
```
(and, after JuliaArrays/LazyArrays.jl#391, this
would return `CachedArrayStyle{2}()`)
|
I guess we don't need a |
|
I think that's right. I wasn't too sure exactly about cases like that (and e.g. the block matrices like I mentioned in my previous comment), but I think I reasoned similarly that |
|
This BroadcastStyle(::Type{<:LinearAlgebra.QRCompactWYQ}) = DefaultArrayStyle{2}()
BroadcastStyle(::Type{<:LinearAlgebra.AdjointQ}) = DefaultArrayStyle{2}()it would be fine, but can't do that on our side without piracy. I don't really understand why they don't already do this. Solutions like defining an internal julia> using LazyArrays, LinearAlgebra
julia> B = LazyArrays.CachedArray(randn(3, 3)); Q = qr(randn(3, 3)).Q; collect(B); Q * B ≈ Q * B.data
ERROR: MethodError: no method matching ndims(::Type{LinearAlgebra.QRCompactWYQ{Float64, Matrix{Float64}, Matrix{Float64}}})
The function `ndims` exists, but no method is defined for this combination of argument types.
Closest candidates are:
ndims(::Type{Union{}}, Any...)
@ Base abstractarray.jl:276
ndims(::Type{<:Ref})
@ Base refpointer.jl:104
ndims(::Type{<:Applied{<:Any, typeof(*), Args}}) where Args
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\linalg\mul.jl:41
...
Stacktrace:
[1] Base.Broadcast.BroadcastStyle(::Type{LinearAlgebra.QRCompactWYQ{Float64, Matrix{Float64}, Matrix{Float64}}})
@ Base.Broadcast .\broadcast.jl:103
[2] tuple_type_broadcastlayout
@ c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyconcat.jl:445 [inlined]
[3] applybroadcaststyle(::Type{ApplyArray{Float64, 2, typeof(*), Tuple{…}}}, ::ApplyLayout{typeof(*)})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyapplying.jl:334
[4] Base.Broadcast.BroadcastStyle(M::Type{ApplyArray{Float64, 2, typeof(*), Tuple{…}}})
@ LazyArrays c:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazyapplying.jl:336
[5] combine_styles(c::ApplyArray{Float64, 2, typeof(*), Tuple{LinearAlgebra.QRCompactWYQ{…}, CachedArray{…}}})
@ Base.Broadcast .\broadcast.jl:433
[6] combine_styles(c1::ApplyArray{Float64, 2, typeof(*), Tuple{…}}, c2::Matrix{Float64})
@ Base.Broadcast .\broadcast.jl:438
[7] broadcasted
@ .\broadcast.jl:1353 [inlined]
[8] broadcast_preserving_zero_d
@ .\broadcast.jl:882 [inlined]
[9] -(A::ApplyArray{Float64, 2, typeof(*), Tuple{LinearAlgebra.QRCompactWYQ{…}, CachedArray{…}}}, B::Matrix{Float64})
@ Base .\arraymath.jl:8
[10] isapprox(x::ApplyArray{…}, y::Matrix{…}; atol::Int64, rtol::Float64, nans::Bool, norm::typeof(norm))
@ LinearAlgebra C:\Users\djv23\.julia\juliaup\julia-1.12.1+0.x64.w64.mingw32\share\julia\stdlib\v1.12\LinearAlgebra\src\generic.jl:1982
[11] isapprox(x::ApplyArray{Float64, 2, typeof(*), Tuple{LinearAlgebra.QRCompactWYQ{…}, CachedArray{…}}}, y::Matrix{Float64})
@ LinearAlgebra C:\Users\djv23\.julia\juliaup\julia-1.12.1+0.x64.w64.mingw32\share\julia\stdlib\v1.12\LinearAlgebra\src\generic.jl:1978
[12] top-level scope
@ REPL[100]:1
Some type information was truncated. Use `show(err)` to see complete types. |
…/BoundsError issue with cacheddata storing a Vcat+scalar
|
I think
is probably the best way around this? I have played around in the past with making a type so that Q's are |
Now going to use this PR to get various other fixes for propagating CachedArrayStyle. Makes it easier for me when debugging but if it's preferred I can later break it up into smaller PRs. Once all the changes are in, I'll edit in all the new features here. Original PR text below.
This changes
VcatandHcatto use the combinedBroadcastStylebetween aLazyArrayStyleand the combined style of its arguments. This fixes e.g.Vcatof cached arguments not being recognised as aCachedArrayStyle.The implementation needs to use some recursion to get the
BroadcastStyleof the arguments viatuple_type_broadcastlayout, but basically it's just doing the same as what you'd get fromresult_style(LazyArrayStyle{N}(), combine_style(A.args...))whereAis aVcat/Hcat