-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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, minmax use isless for comparison (fix #23094) #23155
Conversation
test/operators.jl
Outdated
@test_throws MethodError max(s1, s2) | ||
@test_throws MethodError minmax(s1, s2) | ||
|
||
struct TO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use a more specific type name, like T23094
or something for the issue number? Also, 4 space indent for the x
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
understood!
test/operators.jl
Outdated
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All of the test below here pass for me on master, are they supposed to test something that this PR fixes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Need a space after |
It would indeed make sense to change |
I just wanted to say that this is a great first PR to Julia – thanks for taking the time and effort to do it! |
test/arrayops.jl
Outdated
@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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
===
would be better here, ==
considers both signs of zero to be equal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will change it. (-0.0, 1) == (0.0, 1) but (-0.0, 1) !== (0.0, 1).
base/array.jl
Outdated
@@ -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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Want to change to "Except of that, the result is in line with max
".
(+ removed trailing whitespace)
base/array.jl
Outdated
@@ -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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit unclear. I'd maybe phrase this as "Other than the treatment of NaN
, the result..."
base/array.jl
Outdated
@@ -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`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`NaN`
and same below :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You must have eagle's eyes! :-)
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 |
Sorry about that --- I looked over all the comments and somehow got the impression they'd all been addressed. |
@StefanKarpinski : discrepancy between
|
Agree, this is not your doing, we should fix it, however. |
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`.
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