Construct ranges of CartesianIndexes

[jishnub]

This PR adds the ability to construct StepRangeLens of CartesianIndexes. This works if start/stop, step and length are specified. Methods with step unspecified are not added, and these error as before. The display of a StepRangeLen of CartesianIndexes is also changed to not use the a:b:c spelling, as this produces a CartesianIndices instead.

julia> a = CartesianIndex(1,1)
CartesianIndex(1, 1)

julia> b = StepRangeLen(a, a, 4)
StepRangeLen(CartesianIndex(1, 1), CartesianIndex(1, 1), 4)

julia> b |> collect
4-element Vector{CartesianIndex{2}}:
 CartesianIndex(1, 1)
 CartesianIndex(2, 2)
 CartesianIndex(3, 3)
 CartesianIndex(4, 4)

This is complementary to #37829 that introduced the start-step-stop pattern to produce CartesianIndices that are the iterator product of multiple ranges.

This PR currently also allows one to construct such a range using range as

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

This will be removed, as it is quite confusing to have range(start; stop, length) mean something else from range(start; stop, step). To illustrate why this should be removed:

julia> range(CartesianIndex(1,1), step = CartesianIndex(2,3), stop = CartesianIndex(3,4))
2×2 CartesianIndices{2, Tuple{StepRange{Int64, Int64}, StepRange{Int64, Int64}}}:
 CartesianIndex(1, 1)  CartesianIndex(1, 4)
 CartesianIndex(3, 1)  CartesianIndex(3, 4)

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

[johnnychen94]

So there are two valid interpretations of CartesianIndex{N} here:

• a point (x¹, x², ..., xᴺ)` in N-dimensional space: pⁱ = p₀ⁱ + xⁱ * Δⁱ where pⁱ are real numbers.
• a point (x,) in 1-dimensional space: p = p₀ + x * Δ where x is real number.

and the confusion shows up when the inputs to range are actually not some typical scalar values but instead vectors:

julia> range(start=[1,1], step=[2,3], length=3)
[1, 1]:[2, 3]:[5, 7]

Because order and distance are undefined for vector space, we can't expect range(; start, step, stop) to work here for generic vector types.

I think this is a good change, and the displayed results differences should be interpreted based on the concrete scenarios, it's not ideal but I think both usages are valid.

FWIW, at some point, I was trying to propose CartesianIndices((2:3, 4:5)) displayed as CartesianIndex(2, 4):CartesianIndex(3,5) (JuliaArrays/TiledIteration.jl#27 (comment))

[jishnub]

Tests fail because one is not defined for Char. I wonder what's the best way to handle such cases instead of hard-coding 1 in the function?

[johnnychen94]

I think it's clear and harmless to define:

oneunit(::Char) = Char(1)
zero(::Char) = Char(0)

Because string join is defined as the char multiplication, and because we don't have empty char literal(do we?), we can't define one(::Char) here.

To do this I think we need a separate PR.

[johnnychen94]

This could be a bad idea. Do you think it will make things clearer to add a depwarn when ndims(a) != 0 for the stop version?

Edit: I now think it's a bad idea because it enforces the input type to support ndims protocol, which can be too strong an assumption for a function that supports generic inputs.

[Seelengrab]

What does this PR have to do with string joining?

Since there is no addition on chars with other chars, having a zero(::Char) doesn't make much sense to me and arbitrarily fixing it to Char(0) (i.e. a null-byte) seems weird.

If we want to go with "strings multiply", then neither oneunit nor one on Chars makes sense - Char(1) * 'a' !== 'a' * Char(1) after all.

[Seelengrab]

The failing test in question is this one:

julia/test/ranges.jl

Lines 38 to 40 in 14ba473

	for T = (Int8, Rational{Int16}, UInt32, Float64, Char)
	@test typeof(range(start=T(5), length=3)) === typeof(range(stop=T(5), length=3))
	end

[johnnychen94]

Since there is no addition on chars with other chars, having a zero(::Char) doesn't make much sense to me and arbitrarily fixing it to Char(0) (i.e. a null-byte) seems weird.

Sorry, I only checked that there's char subtraction 'y'-'x' == 1 and didn't check there isn't char addition 'x'+Char(1).

Perhaps others have some ideas, but I don't see a clear path out if we want to extend range to vector space without breaking its scalar contract step = ofeltype(a-a, 1). It might make more sense to have a limited version vrange for vector-like inputs.

[timholy]

Hmm. I see what you mean about this being confusing. But this change is too. Echoing #41960 (comment), what does

a = CartesianIndex(1, 1)
range(a, step=a, stop=CartesianIndex(3, 4))

do? Having length-calls behave fundamentally differently from stop-calls seems really dangerous.

Personally I think this should be by directly calling StepRangeLen rather than via range, just so there is no ambiguity. And I would add an Base.Experimental.register_error_hint about range(a, step=b, length=n) for a and b both CartesianIndex values.

[jishnub]

Yes, given the scope for confusion, perhaps it's best to not go ahead with the general idea of ranges of CartesianIndexes. Perhaps I'll only add the missing constructor (which seems like a missing method)? That is needed to construct a StepRangeLen with CartesianIndex arguments.

[timholy]

Yeah, I agree that the constructor is missing. I don't know if you saw my edit, but I also think an error hint would be helpful here.

[jishnub]

Ah I had missed your edit. I have not used error hints before, where should this go?

Should I make range_start_step_length throw an error instead of the present behavior of calling StepRangeLen?

[jishnub]

Would it be better if range(start; step, length) produced the same result as range(start; step, stop) instead of throwing an error? At least this way range is consistent in what it does if a step is provided explicitly (or even implicitly). Other constructors such as StepRangeLen may do their own thing.

[johnnychen94]

The meaning of range(start; step, stop) is ambiguous and not well-defined for vector inputs so it should in general throw errors. The fact that it works for CartesianIndex should be considered undefined behavior.

Having length-calls behave fundamentally differently from stop-calls seems really dangerous.

On the other hand, the behavior of range(start; step, length) is well defined. And I agree that this can be dangerous/confusing to support the vector-input semantic in the range API. And if you also agree on this, then instead of making range(start; step, length) working for CartesianIndex, it is better to register an error hint or add a depwarn for it, and encourage users to call StepRangeLen explicitly.

[jishnub]

That makes sense, and is probably the best approach. Could you point me towards how to register an error hint? Which file should I put this in?

[johnnychen94]

Maybe just explicitly add some eager checks by dispatching on CartesianIndex and AbstractArray type? For example, make them errors on

# might not be a complete list
range_stop
range_start_step_length
range_stop_length
range_start_stop
range_start_length
range_start_step_stop

Some currently error so it's okay to keep them erroring, some are currently working so we need to deprecate them in favor of the explicit StepRangeLen version.

# The `range` function on non-scalar inputs is not always well defined and could cause confusion.
const UnsupportedRangeTypes = Union{AbstractArray, AbstractCartesianIndex}
range_stop(stop::UnsupportedRangeTypes) = throw(ArgumentError("Non-scalar inputs for `range` are not supported.")
@deprecate range_start_step_length(start::UnsupportedRangeTypes, stop::UnsupportedRangeTypes, length::Integer) StepRangeLen(start, stop, length)

Do you think this is doable?

This could be a breaking change, however, so better to run a pkgeval before approval or merging.

[jishnub]

Closing in favor of #50457, which I had opened after having forgotten about this PR.