DanielVandH commented 1 year ago

Ah, I'm guessing the integer issues are a reason for not converting it initially? Unless perhaps

sqrt(x::T) where {T<:Number}

and

sqrt(x::Integer)

ought to be separated? The point being for floats to at least be able to avoid e.g.

julia> @code_warntype NaNMath.sqrt(-0.3)
MethodInstance for NaNMath.sqrt(::Float64)
  from sqrt(x::Real) in NaNMath at C:\Users\licer\.julia\packages\NaNMath\MNJRI\src\NaNMath.jl:21
Arguments
  #self#::Core.Const(NaNMath.sqrt)
  x::Float64
Body::Float64
1 ─ %1 = (x < 0.0)::Bool
└──      goto #3 if not %1
2 ─      return NaNMath.NaN
t(::Float32)
  from sqrt(x::Real) in NaNMath at C:\Users\licer\.julia\packages\NaNMath\MNJRI\src\NaNMath.jl:21
Arguments
  #self#::Core.Const(NaNMath.sqrt)
  x::Float32
Body::Union{Float32, Float64}
1 ─ %1 = (x < 0.0)::Bool
└──      goto #3 if not %1
2 ─      return NaNMath.NaN
3 ─ %4 = Base.getproperty(NaNMath.Base, :sqrt)::Core.Const(sqrt)
│   %5 = (%4)(x)::Float32
└──      return %5

This doesn't happen for the other functions like log since the methods for Floats and others are separated into

f(x::Float64) = ...
f(x::Float32) = ...
f(x::Real) = f(float(x))

which could also be done here, though you still get Unions as above.

mlubin commented 1 year ago

For which types T is T(NaN) supposed to work? I'd guess T <: AbstractFloat is ok, but we'd need to check to be sure.

DanielVandH commented 1 year ago

I believe only AbstractFloats, based on this definition of isnan from Base

isnan(x::AbstractFloat) = (x != x)::Bool
isnan(x::Number) = false

checking only AbstractFloats.

mlubin commented 1 year ago

sqrt(x::T) where {T<:AbstractFloat} = x < 0.0 ? T(NaN) : Base.sqrt(x)
sqrt(x::Real) = sqrt(float(x))

seems reasonable then

codecov[bot] commented 1 year ago

Codecov Report

Base: 97.02% // Head: 97.05% // Increases project coverage by +0.02% :tada:

Coverage data is based on head (2727977) compared to base (65a5928). Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #64 +/- ## ========================================== + Coverage 97.02% 97.05% +0.02% ========================================== Files 1 1 Lines 101 102 +1 ========================================== + Hits 98 99 +1 Misses 3 3 ``` | [Impacted Files](https://codecov.io/gh/JuliaMath/NaNMath.jl/pull/64?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath) | Coverage Δ | | |---|---|---| | [src/NaNMath.jl](https://codecov.io/gh/JuliaMath/NaNMath.jl/pull/64?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath#diff-c3JjL05hTk1hdGguamw=) | `97.05% <100.00%> (+0.02%)` | :arrow_up: | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaMath)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

JuliaMath / NaNMath.jl

Convert NaN to correct type in sqrt #64

Codecov Report