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

proposal: findNaNmax #31

Open
floswald opened this issue Jun 28, 2019 · 2 comments · May be fixed by #53
Open

proposal: findNaNmax #31

floswald opened this issue Jun 28, 2019 · 2 comments · May be fixed by #53

Comments

@floswald
Copy link

floswald commented Jun 28, 2019

how do people feel about a PR with

julia> function findNaNmax(x::AbstractArray{T}) where T<:AbstractFloat
           result = convert(eltype(x), NaN)
           indmax = 0
           for (i,v) in enumerate(x)
               if !isnan(v)
                   if (isnan(result) || v > result)
                       result = v
                       indmax = i
                   end
               end
           end
           return (indmax,result)
       end
findNaNmax (generic function with 1 method)

julia> Y1 = rand(10_000_000);

julia> Y3 = ifelse.(rand(length(Y1)) .< 0.9, Y1, NaN);

julia> @btime NaNMath.maximum(Y3);
  21.103 ms (1 allocation: 16 bytes)

julia> @btime findNaNmax(Y3);
  23.513 ms (1 allocation: 32 bytes)

?

or maybe even

function findNaNmax2(x::AbstractArray{T}) where T<:AbstractFloat
	result = convert(eltype(x), NaN)
    indmax = 0
    @inbounds @simd for i in eachindex(x)
    	v = x[i]
        if !isnan(v)
            if (isnan(result) || v > result)
                result = v
                indmax = i
            end
        end
    end
    return (indmax,result)
end

julia> @btime findNaNmax2(Y3);
  22.294 ms (1 allocation: 32 bytes)
@truedichotomy
Copy link

Don't have a strong preference, but findnanmax and findnanmin would make great additions to the package!

@truedichotomy
Copy link

To be consistent with findmin and findmax, I'd say return (result, ind) instead of (ind, result).

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 a pull request may close this issue.

2 participants