Convert range type in reduced_index
#34770
Conversation
The `reduced_indices` and `reduced_indices0` methods sometimes assert that the return axes type is the same as the input. Consequently, the implementation of `reduced_index` had better return a range of the same type as the input. This corrects the error in JuliaArrays/OffsetArrays.jl#92. I'll put a workaround in OffsetArrays.jl too.
This risks making these routines more fragile in cases where `reduced_index` is broken. However, this can be viewed as a good thing because it increases the odds that errors will be caught early. Moreover, it ensures that the return type is inferrable in cases where the reduction is over the first dimension but constant-propagation fails to detect this.
|
@nanosoldier |
|
I think my home folder ran out of space, and then the server crashed. Let's try again: @nanosoldier |
|
Your package evaluation job has completed - possible new issues were detected. A full report can be found here. cc @maleadt |
| @@ -20,7 +20,7 @@ reduced_indices0(a::AbstractArray, region) = reduced_indices0(axes(a), region) | |||
| function reduced_indices(inds::Indices{N}, d::Int) where N | |||
| d < 1 && throw(ArgumentError("dimension must be ≥ 1, got $d")) | |||
| if d == 1 | |||
| return (reduced_index(inds[1]), tail(inds)...) | |||
| return (reduced_index(inds[1]), tail(inds)...)::typeof(inds) | |||
There was a problem hiding this comment.
Should we backport this without these assertions? That is, I think this patch is good for master (and perhaps even back porting to 1.4) but I don't think we should add assertions in a back port — especially not for a backport to 1.0.
|
Agreed (I shared the same sentiment in the OP). It will be easy because the type asserts are a separate commit from the other changes. |
|
I should read better. Hah. 👍 |
|
It just shows how insightful your review was 😄. Thanks for taking a look! |
BACKPORTING NOTE: consensus is to backport the first commit but not the second.
The
reduced_indicesandreduced_indices0methods sometimes assert that the return axes type is the same as the input. Consequently, the implementation ofreduced_indexhad better return a range of the same type as the input.This corrects the error in JuliaArrays/OffsetArrays.jl#92. I'll put a workaround in OffsetArrays.jl too.
The second commit consistently asserts type-equality in
reduced_indices. This is a bit more aggressive, and if we backport it's possible we should backport the first but not the second. The potential problem is that it makes these methods more fragile in cases wherereduced_indexis broken. IMO, this is a good thing because it increases the odds that errors will be caught early. Moreover, it ensures that the return type is inferrable in cases where the reduction is over the first dimension but constant-propagation fails to detect this. However, for the LTS release this should be tested for trouble.