JuliaMath / NaNMath.jl

Julia math built-ins which return NaN and accumulator functions which ignore NaN
Other
52 stars 26 forks source link

Add findmin, findmax, argmin, and argmax #53

Open sethaxen opened 2 years ago

sethaxen commented 2 years ago

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 commented 2 years ago

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

codecov-commenter commented 2 years ago

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.

mlubin commented 2 years ago

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.

sethaxen commented 2 years ago

@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.

mlubin commented 2 years ago

It looks like tests are failing on 1.6.

sethaxen commented 2 years ago

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 commented 2 years ago

@mlubin would you like to you re-review?

sethaxen commented 2 years ago

Done!

mlubin commented 2 years ago

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

codecov[bot] commented 1 year ago

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.

jd-foster commented 1 year ago

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