Skip to content
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

RFC: Make strides into a generic trait #30432

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

andyferris
Copy link
Member

My take on a pathway for replacing StridedArray with a more generic and extensible trait. This simply makes the strides function a bit more generic, but does not rearrange any of the dispatch patterns around StridedArray or anything like that.

Returns nothing for non-strided arrays, otherwise gives the strides in memory. Useful as an extensible trait in generic contexts and for arranging dispatch patterns, and simpler to overload for cases of "wrapped" arrays where "stridedness" can be deferred to the parent rather than a complex (and inextensible) method signature.

Thoughts welcome. Functions on instances seem to be the current trend in trait patterns (in part because the answers of interest may not be available from type information alone - in this context take a::Transpose{Float64, AbstractMatrix} (note the lack of a <:), where we can't know if the parent array type is strided or not until we query parent(a)). I'm not sure how I feel about nothing as a sentinel value here, or the generic overload of stride.

Fixes #29705. Closes #29135 and closes #30429. Touches on JuliaLang/LinearAlgebra.jl#186, #10064, #10889, maybe more. CC @mbauman @timholy @raghav9-97

  • Needs tests

@andyferris andyferris added arrays [a, r, r, a, y, s] needs tests Unit tests are required for this change needs news A NEWS entry is required for this change traits Traits, sometimes called Tim Holy Traits labels Dec 18, 2018
Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using nothing as the default here is interesting. On balance I think this is a good way to go.

stride(A::AbstractArray, k::Integer) = strides(A)[k]
function stride(A::AbstractArray, k::Integer)
str = strides(A)
if str === nothing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More compact, if desired:

return str === nothing ? nothing : str[k]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing function allows k>ndims(A), like stride([1,2,3], 99) == 3.

# provide strides, but only for eltypes that are directly stored in memory (i.e. unaffected
# by recursive `adjoint` and `transpose`, being `Real` and `Number` respectively)
function Base.strides(a::Union{Adjoint{<:Real, <:AbstractVector}, Transpose{<:Number, <:AbstractVector}})
str = strides(a.parent)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use A and strides(parent(A))?

@mbauman
Copy link
Member

mbauman commented Dec 18, 2018

Clever, clever, clever. So instead of having a separate trait hierarchy of storage types, we just pass strides itself as a value. I like it. Now my question is — what is a strides at its heart? Is it at all possible to use values (instead of traits) to encapsulate the rigorous demands of @dlfivefifty's storage system from #25558?

In other words, this makes me think that strides is really a function that takes an index as input and outputs a storage location. Thus, in contrast to #25558, you don't look at the Base.MemoryLayout to see if you can call strides; instead, you simply ask it for the function that computes a memory location for you. We'd of course want to use first-class callable structs such that you can dispatch directly on the StridedLocationFunction and access its tuple for calling into BLAS. And then we could eventually implement a SparseCSCLocationFunction or BandedLocation or whatever.

Now, I don't think a function from index -> storage is the right concept here… what is the right concept? Is there one? Am I going off the deep end?

@dlfivefifty
Copy link
Contributor

There were two conflating uses of MemoryLayout in #25558: looking up data, and dispatching on mul!. I like @mbauman's idea for the looking up data part. But I don't see how it helps with mul!, where we need to handle different permutations of strided, banded, triangular, etc.

@andyferris
Copy link
Member Author

andyferris commented Dec 18, 2018

Nice idea, Matt. That’s some next level stuff!

@dlfivefifty I think the point is that there would be functions of different types returned by strides. These would be different types of structs containing the strides etc and with a call (or getindex?) overload. It would be designed so you can dispatch on the type of this mapping for thing like mul!.

@andyferris andyferris closed this Dec 18, 2018
@andyferris andyferris reopened this Dec 18, 2018
@dlfivefifty
Copy link
Contributor

I get that. I just don't see how it would work in dispatch with composed layouts like Symmetric(UpperTriangular(...)). Unless the proposal is essentially 1-to-1 with the layouts in #25558, just with an added call override.

But conflating memory layout with mul! behaviour was never a good design anyways. Maybe something like the following would work.

  1. Use your proposal for strides
  2. Make a lazy Applied{Style<:ApplyStyle} that mimics Broadcasted{Style<:BroadcastStyle} for lazy function application
  3. Have a default:
struct StridedApplyStyle{F<:Function, ST<:Tuple} <: ApplyStyle
    f::F
    strides::ST
end

ApplyStyle(::typeof(*), A::AbstractArray...) = StridedApplyStyle(*, strides.(A))

as the type that determines the apply style from the strides.

@andyferris
Copy link
Member Author

Right - we’ll definitely need a “promotion” mechanism to control dispatch of operations on multiple containers.

@Jutho
Copy link
Contributor

Jutho commented Dec 23, 2018

+1

@oxinabox
Copy link
Contributor

oxinabox commented Jun 5, 2019

What is the status on this?

@mbauman
Copy link
Member

mbauman commented May 4, 2020

Just a note that our potential design space just got significantly larger with #34126 — we no longer need to be fearful of holding onto arrays in structs.

@oscardssmith
Copy link
Member

Triage likes this. How much work is needed to merge?

@oscardssmith oscardssmith removed triage This should be discussed on a triage call linalg triage labels Jan 6, 2022
Returns `nothing` for non-strided arrays, otherwise gives the
give strides in memory. Useful as an extensible trait in generic
contexts, and simpler to overload for cases of "wrapped" arrays where
"stridedness" can be deferred to the parent rather than a complex
(and inextensible) method signature.
@andyferris
Copy link
Member Author

I have rebased this and update stride to accept larger dimensions as currently supported.

How much work is needed to merge?

It's been such a long time and I haven't been following the changes closely - does anyone else know if more should be done?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] needs news A NEWS entry is required for this change needs tests Unit tests are required for this change traits Traits, sometimes called Tim Holy Traits
Projects
None yet
Development

Successfully merging this pull request may close these issues.

strides missing methods for various AbstractArray subtypes