Preserve input types for various rules

[ptiede]

This pull request ensures that the input type is preserved for various rules.
Previously there were potentially a few places where 64 bit NaN's would always be produced
regardless of the input. To fix this I replaced any instance of :NaN with oftype($x, NaN).

Additionally, this pull-request fixes the issue with the ldexp rule from #88 which is similar in nature.

[codecov-commenter]

Codecov Report

Base: 97.31% // Head: 97.31% // No change to project coverage 👍

Coverage data is based on head (5767bca) compared to base (489e294).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##           master      #89   +/-   ##
=======================================
  Coverage   97.31%   97.31%           
=======================================
  Files           3        3           
  Lines         186      186           
=======================================
  Hits          181      181           
  Misses          5        5           

Impacted Files	Coverage Δ
src/rules.jl	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

[devmotion]

To fix this I replaced any instance of :NaN with oftype($x, NaN).

:NaN is the DiffRules way to say that some derivative does not exist or is not implemented. You never want to use these "derivatives" anyway, every time you do you are already screwed.

Generally, oftype($x, NaN) seems like the wrong approach as x might not even be a floating point number. Just using float($x) instead seems also wrong in the two-argument functions since it might cause undesired promotions if x is not a floating point number (imagine eg the other argument being of type Float32).

[ptiede]

Ok I reverted the :NaN for the two argument functions.

However for the ldexp I think the point remains. In that case you do need the oftype(float($x), exp2($y)) to prevent a spurious promotion for the derivative with respect to x.

[devmotion]

Could we even just use

Suggested change

	@define_diffrule Base.ldexp(x, y) = :( oftype(float($x), exp2($y)) ), :(oftype(float($x), NaN))
	@define_diffrule Base.ldexp(x, y) = :( oftype($x, exp2($y) ), :NaN

? At least it seems the definitions in Base already assume that x is a floating point number:

julia> methods(ldexp)
# 7 methods for generic function "ldexp":
[1] ldexp(x::Float16, q::Integer) in Base.Math at math.jl:826
[2] ldexp(x::T, e::Integer) where T<:Union{Float16, Float32, Float64} in Base.Math at math.jl:783
[3] ldexp(x::BigFloat, n::Int64) in Base.MPFR at mpfr.jl:648
[4] ldexp(x::BigFloat, n::Union{Int16, Int32, Int64, Int8}) in Base.MPFR at mpfr.jl:658
[5] ldexp(x::BigFloat, n::UInt64) in Base.MPFR at mpfr.jl:653
[6] ldexp(x::BigFloat, n::Union{UInt16, UInt32, UInt64, UInt8}) in Base.MPFR at mpfr.jl:659
[7] ldexp(x::BigFloat, n::Integer) in Base.MPFR at mpfr.jl:660

But maybe

Suggested change

	@define_diffrule Base.ldexp(x, y) = :( oftype(float($x), exp2($y)) ), :(oftype(float($x), NaN))
	@define_diffrule Base.ldexp(x, y) = :( oftype(float($x), exp2($y) ), :NaN

is safer - although maybe even safer would be something like oftype(ldexp($x, $y), ... (or something similar without evaluating the primal) in case the arguments are promoted in some way in some other, non-Base definitions.

[ptiede]

I tend to agree for the first option. I would be worried about including the primal in the calculation since that is a much more expensive operation than exp2(y) which is just some bit shuffling since y is a integer.

[devmotion]

Looks good to me.