Add findmin, findmax, argmin, and argmax
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)
@mlubin can you review this when you get the chance?
Codecov Report
Merging #53 (b1680ee) into master (3fe557e) will increase coverage by
0.79%. The diff coverage is100.00%.
@@ 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 dataPowered by Codecov. Last update 3fe557e...b1680ee. Read the comment docs.
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 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.
It looks like tests are failing on 1.6.
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.
@mlubin would you like to you re-review?
Done!
Hmm, there's a test failure on 1.6 (x86).
Codecov Report
:exclamation: No coverage uploaded for pull request base (
master@6e5f40d). Click here to learn what that means. The diff coverage isn/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 dataPowered by Codecov. Last update 6e5f40d...41b3e7e. Read the comment docs.
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