CachedArrayStyle for broadcasting with cached arrays #383
Conversation
|
@DanielVandH I guess I forgot to click on "create"... |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #383 +/- ##
==========================================
- Coverage 95.91% 95.88% -0.04%
==========================================
Files 17 17
Lines 3333 3351 +18
==========================================
+ Hits 3197 3213 +16
- Misses 136 138 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@DanielVandH do you want to have a go getting the tests to pass again (before we add any functionality)? I think the best way to do this is to make an |
|
Yeah I was just taking a look now, but I'll fix the tests first. That |
|
Do I not have perms to contribute to this repository, or do you need to allow contributions from maintainers for me to commit directly to this PR? |
|
Assuming I used Git correctly, #385 should be my commit that I intended to add into this PR |
|
Yes that should work! I can also add you as a contributor to this repos if you wanted to push directly to this PR, but I don't think it matters too much |
|
One issue with the layout approach.. is this intentional? julia> a = Accumulate(*, 1:5)
5-element Accumulate{Int64, 1, typeof(*), Vector{Int64}, UnitRange{Int64}}:
1
2
6
24
120
julia> a isa LazyArrays.AbstractCachedArray
true
julia> MemoryLayout(a)
LazyLayout()
julia> a isa LazyArrays.AbstractCachedArray
true
julia> MemoryLayout(a) isa LazyArrays.CachedLayout
false
julia> MemoryLayout(a) isa LazyArrays.AbstractCachedLayout
ERROR: UndefVarError: `AbstractCachedLayout` not defined in `LazyArrays`
Suggestion: check for spelling errors or missing imports.
Stacktrace:
[1] getproperty(x::Module, f::Symbol)
@ Base .\Base_compiler.jl:47
[2] top-level scope
@ REPL[109]:1Surely this should be a cachedlayout(::Data, ::Array) where {Data,Array} = CachedLayout{Data,Array}()
MemoryLayout(C::Type{CachedArray{T,N,DAT,ARR}}) where {T,N,DAT,ARR} = cachedlayout(MemoryLayout(DAT), MemoryLayout(ARR))For The approach I had in the PR previously would still work if we want to just avoid this issue for now, but maybe it's worth looking at anyway |
|
We probably need to add an |
|
Like abstract type AbstractCachedLayout <: MemoryLayout end
struct CachedLayout{Data,Array} <: AbstractCachedLayout end
struct GenericCachedLayout <: AbstractCachedLayout end
cachedlayout(::Data, ::Array) where {Data,Array} = CachedLayout{Data,Array}()
MemoryLayout(C::Type{CachedArray{T,N,DAT,ARR}}) where {T,N,DAT,ARR} = cachedlayout(MemoryLayout(DAT), MemoryLayout(ARR))
MemoryLayout(::Type{<:AbstractCachedArray}) = GenericCachedLayout()or do you mean something else? |
|
Hm.. the layout change is a bit annoying. One of the downstream tests (just as an example, there's a lot of breakage downstream) A = BandedMatrices._BandedMatrix(Vcat(2*Ones(1,∞), ((1 ./(1:∞)).+1/4)', Ones(1,∞)./3), ℵ₀, 1, 1)
Q, L = ql(A)
@test Q' * zeros(∞,2) isa CachedArraybreaks now because of changing to a is not used and instead this finite version is used. Do you see any good way around this, beyond just going back to the type-based approach for now? Maybe it should be |
|
I can't seem to reproduce the downstream error in ClassicalOrthogonalPolynomials.jl at all. Everything passes for me. Does it fail on your machine? Nevermind, somehow I managed to Ah, it's just not handling matrices properly, an MWE is: julia> copyto!(zeros(2, 6), Accumulate(*, 1:2) .\ zeros(2, 6))
ERROR: ArgumentError: Cannot resize beyond size of operator
Stacktrace:
[1] _vec_resizedata!(B::Accumulate{Int64, 1, typeof(*), Vector{Int64}, UnitRange{Int64}}, n::Int64)
@ LazyArrays C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:235
[2] resizedata!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:254 [inlined]
[3] resizedata!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:222 [inlined]
[4] _bc_resizecacheddata!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:400 [inlined]
[5] resize_bcargs!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:409 [inlined]
[6] copyto!(dest::Matrix{…}, bc::Base.Broadcast.Broadcasted{…})
@ LazyArrays C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:428
[7] materialize!
@ .\broadcast.jl:905 [inlined]
[8] materialize!
@ .\broadcast.jl:902 [inlined]
[9] copyto!_layout
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazybroadcasting.jl:43 [inlined]
[10] copyto!_layout
@ C:\Users\djv23\.julia\packages\ArrayLayouts\FxFFC\src\ArrayLayouts.jl:272 [inlined]
[11] copyto!(dest::Matrix{Float64}, src::BroadcastMatrix{Float64, typeof(\), Tuple{Accumulate{…}, Matrix{…}}})
@ ArrayLayouts C:\Users\djv23\.julia\packages\ArrayLayouts\FxFFC\src\ArrayLayouts.jl:274
[12] top-level scope
@ REPL[275]:1
Some type information was truncated. Use `show(err)` to see complete types.
julia> copyto!(zeros(2, 6), view(Accumulate(*, 1:10), 1:2) .\ zeros(2, 6))
ERROR: BoundsError: attempt to access 2-element UnitRange{Int64} at index [12]
Stacktrace:
[1] throw_boundserror(A::UnitRange{Int64}, I::Int64)
@ Base .\essentials.jl:15
[2] _getindex
@ .\range.jl:952 [inlined]
[3] getindex
@ .\array.jl:3134 [inlined]
[4] _broadcast_getindex_evalf
@ .\broadcast.jl:699 [inlined]
[5] _broadcast_getindex
@ .\broadcast.jl:672 [inlined]
[6] #copy##0
@ .\broadcast.jl:1124 [inlined]
[7] ntuple
@ .\ntuple.jl:50 [inlined]
[8] copy
@ .\broadcast.jl:1124 [inlined]
[9] materialize
@ .\broadcast.jl:894 [inlined]
[10] resizedata!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:585 [inlined]
[11] _bc_resizecacheddata!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:400 [inlined]
[12] resize_bcargs!
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:409 [inlined]
[13] copyto!(dest::Matrix{…}, bc::Base.Broadcast.Broadcasted{…})
@ LazyArrays C:\Users\djv23\.julia\dev\LazyArrays.jl\src\cache.jl:428
[14] materialize!
@ .\broadcast.jl:905 [inlined]
[15] materialize!
@ .\broadcast.jl:902 [inlined]
[16] copyto!_layout
@ C:\Users\djv23\.julia\dev\LazyArrays.jl\src\lazybroadcasting.jl:43 [inlined]
[17] copyto!_layout
@ C:\Users\djv23\.julia\packages\ArrayLayouts\FxFFC\src\ArrayLayouts.jl:272 [inlined]
[18] copyto!(dest::Matrix{Float64}, src::BroadcastMatrix{Float64, typeof(\), Tuple{SubArray{…}, Matrix{…}}})
@ ArrayLayouts C:\Users\djv23\.julia\packages\ArrayLayouts\FxFFC\src\ArrayLayouts.jl:274
[19] top-level scope
@ REPL[276]:1
Some type information was truncated. Use `show(err)` to see complete types.Will fix |
|
Seems like everything downstream is fine |
|
LGTM! (Does that mean "Looks Good To Me" or "Let's Get This Merged"?) I guess it's ready to merge? |
|
I think it means "looks good to me" Yeah from what I can tell it's ready to merge |
No description provided.