SubIndex, LinearSubIndex, and PermutedIndex types
#202
Conversation
Codecov Report
@@ Coverage Diff @@
## master #202 +/- ##
==========================================
- Coverage 86.20% 80.80% -5.40%
==========================================
Files 11 11
Lines 1812 1730 -82
==========================================
- Hits 1562 1398 -164
- Misses 250 332 +82
Continue to review full report at Codecov.
|
| function ArrayIndex{1}(x::PermutedDimsArray{<:Any,N,perm,iperm}) where {N,perm,iperm} | ||
| ComposedIndex(_to_cartesian(x), PermutedIndex{N,perm,iperm}()) | ||
| end | ||
|
|
There was a problem hiding this comment.
Because many SubArrays and PermutedDimArrays can be represented by StridedIndex, can we have ArrayIndex for these return a StridedIndex?
There was a problem hiding this comment.
The way I'm treating ArrayIndex here is a one layer deep index representation. The problem with going straight to StrideIndex is that whatever is calling ArrayIndex(::AbstractArray) doesn't know how deeply nested the buffer is. I figured the user level interface would be more like layout(A, accessor) = ArrayIndex(A), buffer(A) and we could special case for cartesian indexing going straight to StrideIndex.
There was a problem hiding this comment.
@chriselrod, should this PR have something like the layout method implemented so that it is immediately more useful? I was trying to make a PR that was still functional but didn't create too many new things at once, but I can see how that lack of a fully implemented interface runs the risk of breaking changes in the near future. Alternatively, I could add warnings to the docs that this is experimental, so that we can drop/change stuff without worrying about breaking changes for now.
There was a problem hiding this comment.
Alternatively, I could add warnings to the docs that this is experimental, so that we can drop/change stuff without worrying about breaking changes for now.
Sure, although why not have these things live in a separate, experimental repo?
Are they part of the array interface, or something other libraries would want to extend?
It's hard for me to assess changes like these if I don't know the vision for their future use.
There was a problem hiding this comment.
Hopefully recent commits involving the indexing pipeline make it more clear that these changes allow unique interactions between layers of nested arrays. If we were to have a separate package for this we'd need ArrayInterface.jl to depend on it so that we could complete the indexing interface.
That being said, we don't need to strictly use what I've proposed here. The point of the ArrayInterface.compose methods here is to support a simple rule based system for composing simplified/efficient index transformations. I'm sure will need the new types here, but I'm completely open to there being a more optimal way to doing this.
| c2 = nothing | ||
| elseif C === 1 | ||
| c2 = 2 | ||
| else |
There was a problem hiding this comment.
Should add
elseif C === 2
c2 = 1Although given the getfield(iperm, C) fallback in the other method, is this somehow not allowed?
Not sure what PermutedIndex is without looking at the code more closely.
| function ArrayIndex{1}(x::PermutedDimsArray{<:Any,N,perm,iperm}) where {N,perm,iperm} | ||
| ComposedIndex(_to_cartesian(x), PermutedIndex{N,perm,iperm}()) | ||
| end | ||
|
|
There was a problem hiding this comment.
Alternatively, I could add warnings to the docs that this is experimental, so that we can drop/change stuff without worrying about breaking changes for now.
Sure, although why not have these things live in a separate, experimental repo?
Are they part of the array interface, or something other libraries would want to extend?
It's hard for me to assess changes like these if I don't know the vision for their future use.
|
There's a lot that I'd like to try out with this but don't have a formalized interface prepared for. Some of it will be straightforward but some will take some more discussion:
While we could probably have a completely new package for this, I think there's enough codependence between these subtypes of @timholy and @johnnychen94, you had some input when I first proposed |
|
The most recent commit here illustrates why I think this PR is necessary for having a general array indexing interface. |
|
This now composes an efficient layout represent for indexing. function static_size(x, inds)
StaticArrays.Size(ArrayInterface._mapsub(ArrayInterface.known_length, inds))
end
function ArrayInterface.relayout_constructor(::Type{<:StaticArrays.SArray})
static_size
end
function ArrayInterface.:(∘)(x::NTuple{N}, y::StaticArrays.Size{S}) where {N,S}
StaticArrays.SArray{Tuple{S...},eltype(x),length(S),N}(x)
endMakes it so we can do: julia> A = view(SArray{Tuple{3,3}}((1, 2, 3, 4, 5, 6, 7, 8, 9)), :, static(1):static(2));
julia> B = ArrayInterface.getindex(A, static(1):static(2), 1)
2-element SVector{2, Int64} with indices SOneTo(2):
1
2
Using this design any array type that defines an appropriate function adapt(::Type{T}, A) where {T<:AbstractArray}
inds = ArrayInterface.to_indices(A, ntuple(_, :, Val(ndims(A))))
buf = ArrayInterface.getlayout(device(T), A, layout(A, static(ndims(A))))
ArrayInterface.relayout(buf, A, inds)
end |
|
Since you are defining It's already possible to remove Adapt.jl using GeoData.jl uses Flatten.jl/ See here: ConstructionBase.jl is probably a dependency of most of SciML already. |
|
I'm aware of ConstructionBase.jl and considered the potential overlap, but I think there's an important difference. That being said, it could make sense for specific array types to use |
|
Ok good to know you've been thinking about that already. I can see how wanting more specific behaviors for some types is useful, like returning an entirely different wrapper - and Reducing the amount of interface methods we have to define is good. |
|
|
|
Sure! My comment was much more general, for the long term of widely-used methods for reconstructing things. I wish I understood more of the details of your PRs to comment at a more immediately useful level, but currently I don't. So carry on :) |
|
Anything static should go to https://github.com/JuliaArrays/StaticArrayInterface.jl |
PermutedIndexforAdjoint,Transpose,PermutedDimsArraySubIndexforSubArrayLinearSubIndexforFastSubArrayandFastContiguousSubArraygetindex(::ArrayIndex, ::ArrayIndex)methods: convenient way of creating rules for combining subtypes ofArrayIndexArrayIndex{N}(A)constructor methods:ArrayIndex{1}(A)constructs a subtype appropriate for linear indexingArrayIndex{1+}(A)construct a subtype appropriate for cartesian indexingHopefully this PR helps make the utility of
ArrayIndexsubtypes a little more clear without committing to any controversial syntax or interface. I'm sure we will want a more flexible approach for constructing anArrayIndexin the future, but this at least gets things up and running.