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

Add findmin, findmax, argmin, and argmax #53

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

sethaxen
Copy link

@sethaxen sethaxen commented Feb 6, 2022

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
Copy link
Author

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

@codecov-commenter
Copy link

codecov-commenter commented Feb 16, 2022

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%) ⬆️

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.

Copy link
Collaborator

@mlubin mlubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I could accept this after some reworking.

src/NaNMath.jl Outdated Show resolved Hide resolved
src/NaNMath.jl Outdated Show resolved Hide resolved
src/NaNMath.jl Outdated Show resolved Hide resolved
src/NaNMath.jl Outdated Show resolved Hide resolved
src/NaNMath.jl Outdated Show resolved Hide resolved
src/NaNMath.jl Outdated Show resolved Hide resolved
src/NaNMath.jl Outdated Show resolved Hide resolved
@mlubin
Copy link
Collaborator

mlubin commented Feb 16, 2022

Please rebase on top of #54. I saw the 1.0 build is failing, so I dropped support for 1.0.

@sethaxen
Copy link
Author

@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
Copy link
Collaborator

mlubin commented Feb 16, 2022

It looks like tests are failing on 1.6.

@sethaxen
Copy link
Author

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
Copy link
Author

@mlubin would you like to you re-review?

Copy link
Collaborator

@mlubin mlubin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reminder. The new version looks good. I have a minor comment on the tests. Please also update the README to mention the new functions.

test/runtests.jl Outdated Show resolved Hide resolved
@sethaxen
Copy link
Author

Done!

@mlubin
Copy link
Collaborator

mlubin commented Feb 23, 2022

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

@sethaxen sethaxen closed this Mar 9, 2022
@sethaxen sethaxen reopened this Mar 9, 2022
@codecov
Copy link

codecov bot commented Jul 14, 2022

Codecov Report

❗ 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
Copy link

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

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

Successfully merging this pull request may close these issues.

proposal: findNaNmax
5 participants