Permit the construction of a 1D StepRangeLen of CartesianIndexes

[jishnub]

This PR adds a CartesianIndex constructor with ndims-matching CartesianIndex argument.

This allows constructing the following range:

julia> r = StepRangeLen(CartesianIndex(2,3), CartesianIndex(1,1), 10)
CartesianIndex(2, 3):CartesianIndex(1, 1):CartesianIndex(11, 12)

julia> collect(r)
10-element Vector{CartesianIndex{2}}:
 CartesianIndex(2, 3)
 CartesianIndex(3, 4)
 CartesianIndex(4, 5)
 CartesianIndex(5, 6)
 CartesianIndex(6, 7)
 CartesianIndex(7, 8)
 CartesianIndex(8, 9)
 CartesianIndex(9, 10)
 CartesianIndex(10, 11)
 CartesianIndex(11, 12)

[Seelengrab]

The wider discussion around how CartesianIndex is expected to behave in discussions like #43336 is likely related - not quite sure if this PR and the points discussed in that (and other issues linked within) are in conflict or not though.

[jishnub]

There is certainly some scope for ambiguity here, as the following will behave differently:

julia> range(CartesianIndex(1,1), step=CartesianIndex(1,1), stop=CartesianIndex(2,2))
CartesianIndices((1:1:2, 1:1:2))

julia> range(CartesianIndex(1,1), step=CartesianIndex(1,1), length=2)
CartesianIndex(1, 1):CartesianIndex(1, 1):CartesianIndex(2, 2)

However, a 1D range of CartesianIndexes is a vital cog in indexing into banded matrices, and the StepRangeLen constructor appears to be consistent here. Perhaps we may disallow using range in this case until a decision is reached?

[Seelengrab]

Yes that's a very good point - we'll likely have to decide one way or another at some point what we want the semantics to be. The core issue is in which direction that length ("how many steps to take") should be, i.e. whether a CartesianIndex is vector-like or not. The same issue is unfortunately also present if we take StepRangeLen in isolation..

[jishnub]

Looking at the history of the colon constructor for CartesianIndices, it seems like this was added as a syntactic sugar, and not with the properties of CartesianIndexes in mind. Given that CartesianIndexes obey all the axioms of a vector space, it'll be odd to not allow them to behave like vectors.

It seems like I had attempted a similar change in #41960, where the suggestion was the same: that StepRangeLen makes sense, but range is inconsistent and may be disallowed beyond the current behavior.

[Seelengrab]

Yes, there have been a few attempts :) Calling StepRangeLen directly makes most sense, because that at least removes the ambiguity on which axis the steps go, but it unfortunately doesn't remove the ambiguity whether the resulting object should open up the cartesian product/hypercube of the resulting extent, or just the axis connecting the endpoints.

[jishnub]

I think that since StepRangeLen <: AbstractRange <: AbstractVector, the constructor should only return an object of that type (i.e. an AbstractVector), and not an n-D CartesianIndices hypercube. The ambiguity in the meaning of range will probably persist, which is why it makes sense to disallow the start-step-length range method for CartesianIndexes (as in this PR).

[JeffBezanson]

Triage says 👍 on adding the constructor method, but we don't think adding the error method fixes the real problem. The real problem is the printing of the StepRangeLen object using colons, when that colon syntax gives a different result. The printing needs to be fixed. Some also object to this overloading of :, but that ship has sailed. Anyway we should try to fix the printing instead.

[jishnub]

I've specialized show to display the StepRangeLen constructor:

julia> StepRangeLen(CartesianIndex(1, 2), CartesianIndex(3, 4), 2)
StepRangeLen(CartesianIndex(1, 2), CartesianIndex(3, 4), 2)

This should fix #50784

[jishnub]

Gentle bump

[jishnub]

Gentle bump @JeffBezanson

[jishnub]

Gentle bump

[jishnub]

Gentle bump

[jishnub]

Gentle bump, is there anything that remains to be done here?

[Seelengrab]

I think it's good now, I added the triage label since I wasn't sure that triage approves too. I'll bring it up next session if I have time to join.

[LilithHafner]

Triage already approved this.

[jishnub]

Thanks everyone!

[LilithHafner]

Thank you for bearing with us.