min, max, minmax use isless for comparison (fix #23094) #23155

KlausC · 2017-08-06T17:33:16Z

Changed only min, max, minmax in operators.jl. maybe have to look at findmin, findmax in order to make the results of those compatible with min, max, minmax. That is currently not the case for floating point numbers.

fix #23094

fredrikekre · 2017-08-06T18:10:56Z

test/operators.jl

+@test_throws MethodError max(s1, s2)
+@test_throws MethodError minmax(s1, s2)
+
+struct TO


Perhaps use a more specific type name, like T23094 or something for the issue number? Also, 4 space indent for the x.

understood!

fredrikekre · 2017-08-06T18:11:44Z

test/operators.jl

+Base.isless(a::TO, b::TO) = isless(a.x, b.x)
+Base.isequal(a::TO, b::TO) = isequal(a.x, b.x)
+
+@test min(TO(1), TO(2)) == TO(1)


All of the test below here pass for me on master, are they supposed to test something that this PR fixes?

yes. min, max now fails when called with Set instead of returning an invalid result, but succeed when called with a Type, which defines isless.

I think the Set tests are clear enough, it's the ones testing TO(1) and such for which it's unclear what they do.

The test verify, that min and max work as expected, if the type TO defines a total ordering (by defining isless. Remember, that in the initial bug, they did not. Maybe I should have called isequal instead of ==, which falls back to ===. So actually I am testing an unessential implementation detail.

What I meant is that that is already working on master:

julia> using Base.Test julia> struct TO x end julia> Base.isless(a::TO, b::TO) = isless(a.x, b.x) julia> Base.isequal(a::TO, b::TO) = isequal(a.x, b.x) julia> @testset begin @test min(TO(1), TO(2)) == TO(1) @test min(TO(2), TO(1)) == TO(1) @test max(TO(1), TO(2)) == TO(2) @test max(TO(2), TO(1)) == TO(2) @test minmax(TO(1), TO(2)) == (TO(1), TO(2)) @test minmax(TO(2), TO(1)) == (TO(1), TO(2)) end Test Summary: | Pass Total test set | 6 6

Becase < for TO falls back to

julia/base/operators.jl

Line 200 in ed746ed

<(x, y) = isless(x, y)

It is fine to test working code, but I guess the point was to test something that this PR fixes? In that case the test case should be updated :)

I had not recognized that, sorry. I changed the test case and pushed to my fork.
I see that my commit arrived in this PR.

If you wanted something where the fallbacks don't work as desired, you could define

Base.isless(a::TO23094, b::TO23094) = isless(a.x >> 1, b.x >> 1) Base.isequal(a::TO23094, b::TO23094) = isequal(a.x >> 1, b.x >> 1)

I.e. every pair of integers are in the same isequal equivalence class.

JeffBezanson · 2017-08-06T18:58:49Z

Need a space after fix in fix#23094 so github can parse it.

nalimilan · 2017-08-07T14:18:03Z

It would indeed make sense to change findmin and findmax in this PR too.

StefanKarpinski · 2017-08-07T16:28:50Z

I just wanted to say that this is a great first PR to Julia – thanks for taking the time and effort to do it!

tkelman · 2017-08-07T21:20:02Z

test/arrayops.jl

+    @test findmax(Set(["abc"])) == ("abc", 1)
+    @test findmin(["abc", "a"]) == ("a", 2)
+    @test_throws MethodError findmax([Set([1]), Set([2])])
+    @test findmin([0.0, -0.0]) == (-0.0, 2)


=== would be better here, == considers both signs of zero to be equal

Will change it. (-0.0, 1) == (0.0, 1) but (-0.0, 1) !== (0.0, 1).

StefanKarpinski · 2017-08-07T19:52:47Z

base/array.jl

@@ -1697,7 +1697,7 @@ end

 Returns the maximum element of the collection `itr` and its index. If there are multiple
 maximal elements, then the first one will be returned. `NaN` values are ignored, unless
-all elements are `NaN`.
+all elements are `NaN`. The result is in line with `max`.


The min and max functions return NaN if any of their arguments are NaN: #7866, #12563.

Want to change to "Except of that, the result is in line with max".

(+ removed trailing whitespace)

…23094

tkelman · 2017-08-08T23:25:44Z

base/array.jl

@@ -1697,7 +1697,7 @@ end

 Returns the maximum element of the collection `itr` and its index. If there are multiple
 maximal elements, then the first one will be returned. `NaN` values are ignored, unless
-all elements are `NaN`.
+all elements are `NaN`. Except of that, the result is in line with `max`.


This is a bit unclear. I'd maybe phrase this as "Other than the treatment of NaN, the result..."

fredrikekre · 2017-08-09T12:24:45Z

base/array.jl

@@ -1697,7 +1697,7 @@ end

 Returns the maximum element of the collection `itr` and its index. If there are multiple
 maximal elements, then the first one will be returned. `NaN` values are ignored, unless
-all elements are `NaN`.
+all elements are `NaN`. Other than the treatment of NaN, the result is in line with `max`.


`NaN` and same below :)

You must have eagle's eyes! :-)

StefanKarpinski · 2017-08-09T23:14:12Z

Unfortunately, we now have this discrepancy:

julia> min(1.0, NaN)
NaN

julia> minimum([1.0, NaN])
NaN

julia> findmin([1.0, NaN])
(1.0, 1)

I remarked on this in one of the comments, but it seems to have gone unnoticed. The findmin and findmax functions should consider NaN to be a maximum and a minimum as well.

JeffBezanson · 2017-08-10T04:05:20Z

Sorry about that --- I looked over all the comments and somehow got the impression they'd all been addressed.

KlausC · 2017-08-10T10:06:13Z

@StefanKarpinski : discrepancy between min / minimum vs. findmin.

that is by design, not by implementation. The docu of findmin states the actual behaviour wrt NaN.
I tried to not touch the behaviour of any of these function when applied to floating point numbers
Only exception to 2. is, that now findmin makes a difference between -0.0 and 0.0.
The mentioned discrepancy has been inherited from v0.6.

StefanKarpinski · 2017-08-11T00:32:30Z

Agree, this is not your doing, we should fix it, however.

mbauman · 2017-08-28T05:21:10Z

Does this need a NEWS.md mention?

In #23155, we made the generic `min/max` use `isless` rather than `<`. This is motivated by the observation that the implementation min(a, b) = cmp(a, b) ? a : b implies that `cmp` satisfies the same properties as `isless` (either `cmp(a, b)` in which case we return `a`, otherwise either `cmp(b, a)` or they are equal, in which case returning `b` is correct). However, this is not true for `Real` types, including AbstractFloat (due to `NaN`). Part of the motivation here is that many packages seem to subtype `Real` when, theoretically, perhaps they shouldn't. Prominent examples include `IntervalArithmetic.Interval` and `ForwardDiff.Dual`.

min, max, minmax use isless for comparison (fix#23094)

4a8fffa

fredrikekre reviewed Aug 6, 2017

View reviewed changes

KlausC changed the title ~~min, max, minmax use isless for comparison (fix#23094)~~ min, max, minmax use isless for comparison (fix #23094) Aug 6, 2017

ararslan added kind:breaking This change will break code domain:maths Mathematical functions labels Aug 6, 2017

improved test for fix #23094

fef0d6f

changed findmin and findmax in array.jl (fix #23094)

bef60e1

tkelman reviewed Aug 7, 2017

View reviewed changes

StefanKarpinski reviewed Aug 7, 2017

View reviewed changes

KlausC added 3 commits August 8, 2017 10:07

changed findmin and findmax in array.jl (fix #23094)

ef31713

(+ removed trailing whitespace)

Merge branch 'fix#23094' of https://github.com/KlausC/julia into fix#…

a9c70ae

…23094

Changed == to ===; changed doc text

24ac9c6

tkelman reviewed Aug 8, 2017

View reviewed changes

changed inline docu for findmin and findmax

f3930ad

fredrikekre reviewed Aug 9, 2017

View reviewed changes

Improved inline doc

542c26a

JeffBezanson merged commit 5f582ae into JuliaLang:master Aug 9, 2017

KlausC deleted the fix#23094 branch August 10, 2017 16:41

StefanKarpinski mentioned this pull request Aug 11, 2017

make findmin/findmax behavior match min/max #23209

Closed

mbauman added needs news A NEWS entry is required for this change and removed needs news A NEWS entry is required for this change labels Aug 28, 2017

JeffBezanson mentioned this pull request Jan 22, 2018

Inconsistent behavior for min, findmin, and indmin wrt negative zero #14621

Closed

vasslitvinov mentioned this pull request Jul 29, 2019

nans do not propagate through a reduction chapel-lang/chapel#13560

Closed

timholy mentioned this pull request Sep 17, 2023

Make min/max use isless even for Real #51356

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

min, max, minmax use isless for comparison (fix #23094) #23155

min, max, minmax use isless for comparison (fix #23094) #23155

KlausC commented Aug 6, 2017 •

edited by fredrikekre

Loading

fredrikekre Aug 6, 2017

KlausC Aug 6, 2017

fredrikekre Aug 6, 2017

KlausC Aug 6, 2017

StefanKarpinski Aug 6, 2017

KlausC Aug 7, 2017

fredrikekre Aug 7, 2017

KlausC Aug 7, 2017 •

edited

Loading

StefanKarpinski Aug 7, 2017

JeffBezanson commented Aug 6, 2017

nalimilan commented Aug 7, 2017

StefanKarpinski commented Aug 7, 2017

tkelman Aug 7, 2017

KlausC Aug 8, 2017

StefanKarpinski Aug 7, 2017

KlausC Aug 8, 2017

tkelman Aug 8, 2017

fredrikekre Aug 9, 2017

KlausC Aug 9, 2017

StefanKarpinski commented Aug 9, 2017 •

edited

Loading

JeffBezanson commented Aug 10, 2017

KlausC commented Aug 10, 2017

StefanKarpinski commented Aug 11, 2017

mbauman commented Aug 28, 2017

min, max, minmax use isless for comparison (fix #23094) #23155

min, max, minmax use isless for comparison (fix #23094) #23155

Conversation

KlausC commented Aug 6, 2017 • edited by fredrikekre Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KlausC Aug 7, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeffBezanson commented Aug 6, 2017

nalimilan commented Aug 7, 2017

StefanKarpinski commented Aug 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanKarpinski commented Aug 9, 2017 • edited Loading

JeffBezanson commented Aug 10, 2017

KlausC commented Aug 10, 2017

StefanKarpinski commented Aug 11, 2017

mbauman commented Aug 28, 2017

KlausC commented Aug 6, 2017 •

edited by fredrikekre

Loading

KlausC Aug 7, 2017 •

edited

Loading

StefanKarpinski commented Aug 9, 2017 •

edited

Loading