-
Notifications
You must be signed in to change notification settings - Fork 31
Add missing BroadcastStyle and MemoryLayouts for cache styles #387
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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #387 +/- ##
=======================================
Coverage 95.88% 95.89%
=======================================
Files 17 17
Lines 3351 3359 +8
=======================================
+ Hits 3213 3221 +8
Misses 138 138 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/cache.jl
Outdated
| const _cache = cache_layout # TODO: deprecate | ||
| cacheddata(A::AbstractCachedArray) = view(A.data,OneTo.(A.datasize)...) | ||
| cacheddata(A::Adjoint{<:Any, <:AbstractCachedArray}) = adjoint(cacheddata(parent(A))) | ||
| cacheddata(A::Transpose{<:Any, <:AbstractCachedArray}) = transpose(cacheddata(parent(A))) |
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.
There's no reason to assume the parent is an AbstractCachedArray (it could just conform to the cached array interface):
| cacheddata(A::Transpose{<:Any, <:AbstractCachedArray}) = transpose(cacheddata(parent(A))) | |
| cacheddata(A::Adjoint) = adjoint(cacheddata(parent(A))) | |
| cacheddata(A::Transpose) = transpose(cacheddata(parent(A))) |
src/cache.jl
Outdated
|
|
||
| MemoryLayout(::Type{<:AbstractCachedArray}) = GenericCachedLayout() | ||
|
|
||
| transposelayout(::GenericCachedLayout) = GenericCachedLayout() |
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.
Should this be:
| transposelayout(::GenericCachedLayout) = GenericCachedLayout() | |
| transposelayout(::AbstractCachedLayout) = GenericCachedLayout() |
(also next line)
| CachedArrayStyle{M}(::Val{N}) where {N,M} = CachedArrayStyle{N}() | ||
|
|
||
| BroadcastStyle(::Type{<:AbstractCachedArray{<:Any,N}}) where N = CachedArrayStyle{N}() | ||
| BroadcastStyle(::Type{<:AdjOrTrans{<:Any, <:AbstractCachedArray{<:Any,N}}}) where N = CachedArrayStyle{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.
Do we also need to support adjoints of views of cached arrays?
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 don't think that comes up but I can add support for it anyway
WIP for what we were discussing @dlfivefifty. WIP because I need to fix this new StackOverflow
Also doesn't really change any of the timing issues we found - they're still on the same scale. Gotta do more digging. Creating the PR now just incase I can see what other downstream stuff might be breaking.