Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
mbauman opened this issue Feb 28, 2019 · 8 comments
Labels
arrays [a, r, r, a, y, s] broadcast Applying a function over a collection design Design of APIs or of the language itself

Comments

@mbauman
Copy link
Sponsor Member

mbauman commented Feb 28, 2019

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.

@mbauman mbauman added arrays [a, r, r, a, y, s] design Design of APIs or of the language itself broadcast Applying a function over a collection labels Feb 28, 2019
@jw3126
Copy link
Contributor

jw3126 commented Mar 1, 2019

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
Copy link
Member

julia> isless(CartesianIndex(5, 1), CartesianIndex(1, 5))
true

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

@jw3126
Copy link
Contributor

jw3126 commented Mar 1, 2019

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
Copy link
Sponsor Member Author

mbauman commented Mar 1, 2019

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

@Seelengrab
Copy link
Contributor

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

@goretkin
Copy link
Contributor

goretkin commented Dec 10, 2021

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
Copy link
Contributor

Seelengrab commented Dec 10, 2021

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
Copy link
Contributor

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s] broadcast Applying a function over a collection design Design of APIs or of the language itself
Projects
None yet
Development

No branches or pull requests

5 participants