JuliaLang / julia

The Julia Programming Language
https://julialang.org/
MIT License
45.89k stars 5.49k forks source link

Min/max of CartesianIndex are implicitly "vectorized" and conflict with ordering #31208

Open mbauman opened 5 years ago

mbauman commented 5 years ago

I hesitate to open this issue because gosh this behavior is so very useful. But it's probably worth thinking about a better name/idiom here. In short min and max for CartesianIndex break the function's contract: they don't return one of the arguments.

julia> min(CartesianIndex(5, 1), CartesianIndex(1, 5))
CartesianIndex(1, 1)

Of course this means that minimum also returns an element that is different from first(sort(idxs)) (where idxs is a vector of cartesian indices).

While strange, it is a hugely helpful behavior — e.g., to select the rectangular extents of a found object, you can just do: idxs = findall(...); region = minimum(idxs):maximum(idxs). I similarly don't really want to change sorting of cartesian indices — it'll put them in column-major order for you! So if we change this, we'll need to find a new way of spelling min/minimum/max/maximum.

One possible alternative is broadcasting: it's currently acting as though we've broadcast min across the elements. We don't support broadcasting over CartesianIndex, but perhaps we should implement tuple-like broadcasting semantics:

julia> Broadcast.broadcastable(c::CartesianIndex) = c.I

julia> min.(CartesianIndex(5, 1), CartesianIndex(1, 5))
(1, 1) # would need a style and tuple-like implementation to preserve the type

That would probably also be helpful in other situations, but it's of no help for minimum and maximum.

jw3126 commented 5 years ago

AFAICT the operation op(CartesianIndex(5, 1), CartesianIndex(1, 5)) = CartesianIndex(1, 1) is is the infimum with respect to the order isless on CartesianIndex. So one option would be to introduce functions inf, sup, infimum, supremum that behave on CartesianIndex like min, max, minimum, maximum does currently.

martinholters commented 5 years ago
julia> isless(CartesianIndex(5, 1), CartesianIndex(1, 5))
true

so shouldn't the infimum just be CartesianIndex(5, 1) for these two?

jw3126 commented 5 years ago

Sorry you are right. Now I am confused about the order on CartesianIndex. I thought it was the product order, but your example shows it is not. The current minimum is infimum with respect to the product order.

mbauman commented 5 years ago

Right, ordering on CartesianIndex is defined such that if you sort them, they go in column major order.

Seelengrab commented 2 years ago

I think CartesianIndex should not have an ordering, similar to how Complex doesn't (Ref https://github.com/JuliaLang/julia/issues/42060). Recovering the extent of rectangles you mentioned could be done/finished by implementing https://github.com/JuliaLang/julia/issues/43336, which I haven't gotten to yet.

goretkin commented 2 years ago

You can multiply and add Complex, and there is no consistent ordering possible to satisfy e.g. a > b && c > d implies a + c > b + d.

You cannot multiply and add CartesianIndex values, so the same issue does not arise.

Note also that Tuple and AbstractVector are ordered.

Seelengrab commented 2 years ago

Not sure what you mean with "you cannot multiply and add CartesianIndex values", that's what I've been doing for the past few years now:

julia> CartesianIndex(1,1) * 5 + CartesianIndex(2,2)
CartesianIndex(7, 7)

CartesianIndex behaves like a mathematical vector in that regard. You could also interpret it as lattice points. To me your reply just means that CartesianIndex is relatively obscure & its behavior unknown in the community.

goretkin commented 2 years ago

You're right that they behave like vectors, with scalar multiplication and addition. I stand corrected. Part of the distinction that I meant to draw, however, is that there is no *(::CartesianIndex, ::CartesianIndex).