NaNMath.jl icon indicating copy to clipboard operation
NaNMath.jl copied to clipboard

Add findmin, findmax, argmin, and argmax

Open sethaxen opened this issue 3 years ago • 11 comments

This PR adds versions of findmin, findmax, argmin and argmax that ignore NaNs in the codomain. It does not include methods that take a dims keyword.

Fixes #31

A simple benchmark:

julia> using NaNMath, BenchmarkTools

julia> x = map(x -> x < 0.9 ? x : NaN, rand(1_000, 1_000));

julia> @btime findmax($x);
  1.914 ms (0 allocations: 0 bytes)

julia> @btime $(NaNMath.findmax)($x);
  2.809 ms (0 allocations: 0 bytes)

julia> @btime $(NaNMath.maximum)($x);
  2.100 ms (0 allocations: 0 bytes)

julia> @btime findmax(cos, $x);
  9.590 ms (0 allocations: 0 bytes)

julia> @btime $(NaNMath.findmax)(cos, $x);
  9.226 ms (0 allocations: 0 bytes)

julia> @btime argmax($x);
  1.915 ms (0 allocations: 0 bytes)

julia> @btime $(NaNMath.argmax)($x);
  2.725 ms (0 allocations: 0 bytes)

julia> @btime argmax(cos, $x);
  8.348 ms (0 allocations: 0 bytes)

julia> @btime $(NaNMath.argmax)(cos, $x);
  8.560 ms (0 allocations: 0 bytes)

sethaxen avatar Feb 06 '22 20:02 sethaxen

@mlubin can you review this when you get the chance?

sethaxen avatar Feb 14 '22 11:02 sethaxen

Codecov Report

Merging #53 (b1680ee) into master (3fe557e) will increase coverage by 0.79%. The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #53      +/-   ##
==========================================
+ Coverage   94.89%   95.68%   +0.79%     
==========================================
  Files           1        1              
  Lines          98      116      +18     
==========================================
+ Hits           93      111      +18     
  Misses          5        5              
Impacted Files Coverage Δ
src/NaNMath.jl 95.68% <100.00%> (+0.79%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 3fe557e...b1680ee. Read the comment docs.

codecov-commenter avatar Feb 16 '22 02:02 codecov-commenter

Please rebase on top of https://github.com/mlubin/NaNMath.jl/pull/54. I saw the 1.0 build is failing, so I dropped support for 1.0.

mlubin avatar Feb 16 '22 02:02 mlubin

@mlubin thanks for the review! I have addressed all of your suggestions. Because this PR adds new functions to the API, I incremented the minor version, as specified by the Pkg.jl docs.

sethaxen avatar Feb 16 '22 10:02 sethaxen

It looks like tests are failing on 1.6.

mlubin avatar Feb 16 '22 13:02 mlubin

It looks like tests are failing on 1.6.

Should be fixed now. A few tests were testing against Base methods not introduced until Julia 1.7 or hitting Base bugs on 1.6 fixed on 1.7.

sethaxen avatar Feb 16 '22 20:02 sethaxen

@mlubin would you like to you re-review?

sethaxen avatar Feb 22 '22 09:02 sethaxen

Done!

sethaxen avatar Feb 23 '22 19:02 sethaxen

Hmm, there's a test failure on 1.6 (x86).

mlubin avatar Feb 23 '22 20:02 mlubin

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@6e5f40d). Click here to learn what that means. The diff coverage is n/a.

@@            Coverage Diff            @@
##             master      #53   +/-   ##
=========================================
  Coverage          ?   95.08%           
=========================================
  Files             ?        1           
  Lines             ?      122           
  Branches          ?        0           
=========================================
  Hits              ?      116           
  Misses            ?        6           
  Partials          ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6e5f40d...41b3e7e. Read the comment docs.

codecov[bot] avatar Jul 14 '22 04:07 codecov[bot]

The failure on x86 but not x64 is likely due to a different coincidental ordering of Dict keys, since this occurs on x86 on the PR branch:

julia> NaNMath.argmin(Dict(:x => 3, :w => NaN, :z => -1.0, :y => NaN))
:w

julia> NaNMath.argmax(Dict(:x => 3, :w => NaN, :z => -1.0, :y => NaN))
:w

jd-foster avatar Aug 25 '22 13:08 jd-foster