Detangle Slice and fix mixed-dimension in-place reductions #28941
Conversation
mbauman
force-pushed
the
mb/sliceupslices
branch
from
9c06cae
to
6f3bc6a
Compare
There was a problem hiding this comment.
Beautiful as always. I am really pleased to have the concepts split, 👍 . Most important question is regarding backwards compatibility, how much of an advantage is there in keeping both behaviors under a single "roof" by adding that type parameter? If that's not important then I think this could be made completely non-breaking?
| @@ -218,17 +218,20 @@ Extract first entry of slices of array A into existing array R. | |||
| """ | |||
| copyfirst!(R::AbstractArray, A::AbstractArray) = mapfirst!(identity, R, A) | |||
|
|
|||
| function mapfirst!(f, R::AbstractArray, A::AbstractArray) | |||
| function mapfirst!(f, R::AbstractArray, A::AbstractArray{<:Any,N}) where {N} | |||
There was a problem hiding this comment.
What's up with the type signature here? Forced specialization?
There was a problem hiding this comment.
I had been having trouble getting ntuple to specialize with just constant propagation, so I switched to Val(N) instead… but you're probably right that Val(ndims(A)) would do just as well.
base/indices.jl
Outdated
| Slice(r::AbstractUnitRange) = Slice{typeof(r), false}(r) | ||
| Slice(S::Slice) = Slice{typeof(S.indices), false}(S.indices) | ||
| Slice(S::Slice{<:Any, false}) = S | ||
| const WholeSlice{T} = Slice{T, true} |
| map!(f, R, view(A, t...)) | ||
| end | ||
| # We know that the axes of R and A are compatible, but R might have a different number of | ||
| # dimensions than A, which is trickier than it seems due to offset arrays and type stability |
There was a problem hiding this comment.
Nice to have this inferrable/nonallocating.
We use axes in many downstream computations that may not re-index directly into the original array, so we add a second type parameter to `Slice` that is turned on when converting `:` in indexing expressions -- and really only `SubArray` cares about.
…ty edge-cases Alleviates #28928 but does not completely remove allocations due to the allocation of the view that gets passed to `map!`.
that we will encourage offset array implementations to use instead of Base.Slice
mbauman
force-pushed
the
mb/sliceupslices
branch
from
6f3bc6a
to
7702adc
Compare
|
I tried out Tim's suggestion of a completely different type and I like it. On the downside, it's basically an exact copy of We just introduce a new type (I have named it |
|
Here's another fun one: julia> copyto!([1:3;], OffsetArray(1:5, -2:2))
3-element Array{Int64,1}:
4
5
3The fact that it doesn't throw an error is due to this method. The core problem being that it's a |
|
Let's make this PR happen. It fixes many such issues. I'm surprised it hasn't picked up any conflicts since I last touched it… let's see if CI is still happy. |
|
I say merge at will. |
The change in #50429 moves around some dispatch boundaries and pushes the allocations in the offsetarrays `maximum!` test over the limit. The implementation of that code is massively type unstable. Somewhat, ironically, the whole original point of that test was to test that the implementation was not type-unstable (#28941), so actually opt our OffsetArrays implementation into the interface that's supposed to guarantee that. If this PR is fine here, I'll submit the same upstream to avoid diverging the implementations too much.
The change in #50429 moves around some dispatch boundaries and pushes the allocations in the offsetarrays `maximum!` test over the limit. The implementation of that code is massively type unstable. Somewhat, ironically, the whole original point of that test was to test that the implementation was not type-unstable (#28941), so actually opt our OffsetArrays implementation into the interface that's supposed to guarantee that. If this PR is fine here, I'll submit the same upstream to avoid diverging the implementations too much. Co-authored-by: Jameson Nash <[email protected]>
Well this was a fun adventure. Here I thought fixing #28928 would just be a simple conversion to
ntuple… it turned into a big rabbit hole that glaringly exposed the need to split our two uses ofBase.Slicebetween flagging "whole dimensions" for SubArray and offset axes. This is something @timholy and I had talked about for quite some time and I thought would be more challenging and not a terribly pressing issue, but it turns out this conflation was producing incorrect results in our reductions.