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

Inconsistency between findmax and maximum with respect to NaN #22337

Closed
wsshin opened this issue Jun 12, 2017 · 6 comments
Closed

Inconsistency between findmax and maximum with respect to NaN #22337

wsshin opened this issue Jun 12, 2017 · 6 comments

Comments

@wsshin
Copy link
Contributor

wsshin commented Jun 12, 2017

I read this discussion about max and maximum returning NaN when the collection includes NaN. For example,

julia> VERSION
v"0.7.0-DEV.82"

julia> v = [1.0, NaN, 2.0]
3-element Array{Float64,1}:
   1.0
 NaN
   2.0

julia> maximum(v)
NaN

However, findmax still returns the maximum among non-NaN values and its index:

julia> findmax(v)
(2.0, 3)

This seems inconsistent. Is this something intended or a bug?

@StefanKarpinski
Copy link
Sponsor Member

Seems like findmax needs to be updated to match maximum. There should also be tests to check that they agree.

@ararslan
Copy link
Member

Note that the behavior of findmax is documented to skip NaNs: https://docs.julialang.org/en/latest/stdlib/collections/#Base.findmax-Tuple{Any}

NaN values are ignored, unless all elements are NaN.

@StefanKarpinski
Copy link
Sponsor Member

Yes, it's documented to do this. The question is whether it should be changed to match maximum or not. It seems to me that these functions should have matching behavior.

@ararslan
Copy link
Member

Right, I'm just addressing "is this something intended or a bug." I agree that the behaviors should match.

@cafaxo
Copy link
Contributor

cafaxo commented Mar 22, 2019

In 1.1, this works correctly:

julia> v = [1.0, NaN, 2.0]
3-element Array{Float64,1}:
   1.0
 NaN  
   2.0

julia> findmax(v)
(NaN, 2)

julia> maximum(v)
NaN

I think this issue can be closed. (Might be a duplicate of #23209?)

@StefanKarpinski
Copy link
Sponsor Member

Seems like it on both counts. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants