Fix tests, implement copyto! for #383 #385
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## CachedStyle #385 +/- ##
================================================
+ Coverage 0.00% 95.97% +95.97%
================================================
Files 17 17
Lines 3273 3333 +60
================================================
+ Hits 0 3199 +3199
+ Misses 3273 134 -3139 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| @eval layout_broadcasted(::ZerosLayout, ::CachedLayout, ::typeof($op), a::AbstractVector, b::AbstractVector) = broadcast(DefaultArrayStyle{1}(), $op, a, b) | ||
| end | ||
|
|
||
| function resize_bcargs(bc::Broadcasted{<:CachedArrayStyle}, dest) |
There was a problem hiding this comment.
| function resize_bcargs(bc::Broadcasted{<:CachedArrayStyle}, dest) | |
| function resize_bcargs!(bc::Broadcasted{<:CachedArrayStyle}, dest) |
| rsz_args = let len = length(dest) | ||
| map(bc.args) do arg | ||
| resizedata!(arg, len) | ||
| iscached = arg isa AbstractCachedArray || (arg isa SubArray && parent(arg) isa AbstractCachedArray) |
There was a problem hiding this comment.
This should be all based on memory layouts not types.
| map(bc.args) do arg | ||
| resizedata!(arg, len) | ||
| iscached = arg isa AbstractCachedArray || (arg isa SubArray && parent(arg) isa AbstractCachedArray) | ||
| iscached ? cacheddata(arg) : arg |
There was a problem hiding this comment.
This seems very unlikely to be type stable. I think we want to rewrite this whole map in a functional programming style, something like:
_bc_resizecacheddata!(n) = ()
_bc_resizecacheddata!(n, a, b...) = __bc_resizecacheddata!(n, MemoryLayout(a), a, b...)
__bc_resizecacheddata!(n, _, a, b...) = (a, _bc_resizecacheddata!(b...))
function __bc_resizecacheddata!(n, ::AbstractCachedLayout, a::AbstractVector, b...)
resizedata!(a, n)
(view(cacheddata(a), 1:n), _bc_resizecacheddata!(b...))
endThere was a problem hiding this comment.
I think that since bc.args is a Tuple these should be functionally equivalent? But I'll change it anyway to not rely on that
| κ[1:2] | ||
| leads to a stack overflow. | ||
| =# | ||
| rsz_bc = resize_bcargs(Base.Broadcast.flatten(bc), dest) |
There was a problem hiding this comment.
I'm not convinced flatten is preferable to a recursive call. I.e. something like
_bc_resizedata!(n, a::Broadcasted, b...) = (broadcasted(a.f, _bc_resizedata!(n, a.args...)...), _bc_resizedata!(n, b...))But I think it is fine to flatten for now and come back later if there's an issue.
|
Didn't mean to merge it but whatever, I'll address the comments directly on the other PR |
2 participants
Finishes off #383 (no perms to commit directly)