make findmin/findmax behavior match min/max (fix #23209) #23227
Conversation
| ai, s = next(a, s) | ||
| i += 1 | ||
| ai != ai && continue # assume x != x => x is a NaN | ||
| if m != m || isless(m, ai) | ||
| if ai != ai || isless(m, ai) |
There was a problem hiding this comment.
In the ai != ai case this can short-circuit, right?
There was a problem hiding this comment.
if ai != ai I expect isless is not called. That shall cover the NaN- case. This can occur at most once, since the next loop circle is broken by the m != m condition few lines before.
There was a problem hiding this comment.
Ah, yes, I didn't notice the m != m && break check previously.
base/reducedim.jl
Outdated
|
|
||
| zero2(t::Type) = zero(t) | ||
| zero2(::Type{T}) where T<:Number = zero(T) | ||
| zero2(::Type{T}) where T<:AbstractString = "" |
There was a problem hiding this comment.
requiring this is a bit of a red flag, is there any way to avoid needing this?
There was a problem hiding this comment.
The way is not to rely on zero behavior at all, but to use the first slice as the start value.
There was a problem hiding this comment.
let me improve this!
There was a problem hiding this comment.
The definition of zero2 will be removed in the next commit, it is not required any longer.
I guess the red flag is risen, because a bug fix shall not introduce new Base features?
There was a problem hiding this comment.
As long as this isn't exported, it's not a feature addition. It just seems fishy to need a function like zero but for which "" is the answer for strings. (Nevermind that "" is a reasonable 1 value for strings; there is no zero value unless you embed strings into regular patterns, in which case the empty pattern is the zero value – but it's not a string.)
rfourquet
changed the title
|
Tests failed because of buggy |
test/distributed.jl
Outdated
| @@ -3,6 +3,10 @@ | |||
| # Run the distributed test outside of the main driver since it needs its own | |||
| # set of dedicated workers. | |||
|
|
|||
| using Base.Test | |||
|
|
|||
| @test open("distributed_exec.jl") do io; true end | |||
There was a problem hiding this comment.
what happened here, how is this related?
|
This change is not related to the issue to solve. All test runs on the ci platforms errored during the |
|
The "No tests" is a consequence of #20362, not related. The failure was actually in offsetarray, but we seem to be having a problem in our testsystem right now where it isn't showing the actual backtrace of failures. It might work better if you run locally and only do |
|
Fix complete. Please review. |
test/reducedim.jl
Outdated
| ((2,), reshape([6.0,5.0], 2, 1), reshape([5,2], 2, 1)), | ||
| ((1,2), fill(6.0,1,1),fill(5,1,1))] | ||
| @test findmax(A, tup) == (rval, rind) | ||
| @test findmax!(similar(rval), similar(rind), A) == (rval, rind) | ||
| @test string(maximum(A, tup)) == string(rval) |
There was a problem hiding this comment.
why are you comparing all of these as strings?
There was a problem hiding this comment.
The comparisons using == do not work, when the result collections contain NaN, because NaN != NaN. Comparison with === does not compare array elements.
And I wanted to be able to identify the failed tests by the test values. With a specialized compare function that was not the case ( @test eq1(maximum(A, tup), rval) prints its line literally).
There was a problem hiding this comment.
Can't you use isequal for these cases?
There was a problem hiding this comment.
Yes, I did it. Actually it revealed some differences between Array{T,2} 2x1 and Array{T,1} with equal contents.
|
Are there more changes to do for this fix? |
mbauman
added
needs news
|
Very nice work! This is definitely needed. I took the liberty of removing the unrelated change in tests/distributed.jl. My only question is if we want to deprecate this behavior first. It's a little tricky (especially in the case of all-nan elements), but I think it should be doable. |
|
Is there anything, I should or could do about that question? |
|
Your call @mbauman – I'm not sure if it's worth it, but if you think so, go for it. |
[ci skip]
|
On balance, I think we should just leave this as a breaking change. I've added a NEWS.md bullet point. |
|
Thank you so much @KlausC for seeing this through! |
url: #23209
Same issue for sparse matrices not yet included - will follow.