Add iszero(x) branches to xlogy and xlog1py

[simsurace]

It is unclear to me how the tests work here. I did not find the tests of corner cases.

[codecov-commenter]

Codecov Report

Merging #85 (d324416) into master (5a121b8) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master      #85   +/-   ##
=======================================
  Coverage   97.00%   97.00%           
=======================================
  Files           3        3           
  Lines         167      167           
=======================================
  Hits          162      162           
  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.

[simsurace]

The failures in Nabla.jl and EAGO.jl seem unrelated to this PR.

[simsurace]

Is this fine to be merged?

[simsurace]

I'm bumping this as it is still unclear to me what if anything is missing.

[simsurace]

Then I think we can merge and I'll change JuliaStats/LogExpFunctions.jl#54 to follow the same guidelines.

[devmotion]

Looks OK. We want to use the same rules in LogExpFunctions. Unfortunately, currently these rules cause test failures: JuliaStats/LogExpFunctions.jl#57 So let us wait with this PR until we have solved these issues in LogExpFunctions, in case we have to handle some other special case here.

[simsurace]

This should be fine to be merged now as well.

[devmotion]

Test failures are unrelated, see FluxML/Tracker.jl#125. Similar issues seem to exist for ReverseDiff on Julia 1.8.

Can we add some tests of the new branches though?

[simsurace]

Sure. I just don't know where to put them, since there currently do not seem to be any tests for xlogy and xlog1py. Or where should I look for them?

EDIT: or should new branches be created under the arity == 2 branch?

[devmotion]

Sure. I just don't know where to put them, since there currently do not seem to be any tests for xlogy and xlog1py.

All rules apart from the ones excluded in

DiffRules.jl/test/runtests.jl

Line 17 in 2c29d7d

non_diffeable_arg_functions = [(:Base, :rem2pi, 2), (:Base, :ldexp, 2), (:Base, :ifelse, 3)]

are tested in

DiffRules.jl/test/runtests.jl

Lines 19 to 110 in 2c29d7d

	@testset "($M, $f, $arity)" for (M, f, arity) in DiffRules.diffrules(; filter_modules=nothing)
	for T in [Float32, Float64]
	(M, f, arity) ∈ non_diffeable_arg_functions && continue
	if arity == 1
	@test DiffRules.hasdiffrule(M, f, 1)
	deriv = DiffRules.diffrule(M, f, :goo)
	@eval begin
	let
	goo = if $(f in (:asec, :acsc, :asecd, :acscd, :acosh, :acoth))
	# avoid singularities with finite differencing
	rand($T) + $T(1.5)
	elseif $(f in (:log, :airyaix, :airyaiprimex, :logmxp1))
	# avoid singularities with finite differencing
	rand($T) + $T(0.5)
	elseif $(f === :log1mexp)
	rand($T) - one($T)
	elseif $(f in (:log2mexp, :erfinv))
	rand($T) - $T(0.5)
	else
	rand($T)
	end
	# We're happy with types with the correct promotion behavior, e.g.
	# it's fine to return `1` as a derivative despite input being `Float64`.
	@test promote_type(typeof($deriv), $T) === $T
	# In older versions of LogExpFunctions `log1pmx(::Float32)` and `logmxp1(::Float32)` are not defined
	if hasmethod($M.$f, Tuple{$T})
	@test $deriv ≈ finitediff($M.$f, goo) rtol=1e-2 atol=1e-3
	end
	# test for 2pi functions
	if $(f === :mod2pi)
	goo = 4 * pi
	@test NaN === $deriv
	end
	end
	end
	elseif arity == 2
	@test DiffRules.hasdiffrule(M, f, 2)
	derivs = DiffRules.diffrule(M, f, :foo, :bar)
	@eval begin
	let
	foo, bar = if $(f === :mod)
	rand($T) + 13, rand($T) + 5 # make sure x/y is not integer
	elseif $(f === :polygamma)
	rand(1:10), rand($T) # only supports integers as first arguments
	elseif $(f in (:bessely, :besselyx))
	# avoid singularities with finite differencing
	rand($T), rand($T) + $T(0.5)
	elseif $(f === :log)
	# avoid singularities with finite differencing
	rand($T) + $T(1.5), rand($T)
	elseif $(f === :^)
	# avoid singularities with finite differencing
	rand($T) + $T(0.5), rand($T)
	else
	rand($T), rand($T)
	end
	dx, dy = $(derivs[1]), $(derivs[2])
	if !(isnan(dx))
	@test dx ≈ finitediff(z -> $M.$f(z, bar), foo) rtol=1e-2 atol=1e-3
	# Check type, if applicable.
	@test promote_type(typeof(real(dx)), $T) === $T
	end
	if !(isnan(dy))
	@test dy ≈ finitediff(z -> $M.$f(foo, z), bar) rtol=1e-2 atol=1e-3
	# Check type, if applicable.
	@test promote_type(typeof(real(dy)), $T) === $T
	end
	end
	end
	elseif arity == 3
	#=
	@test DiffRules.hasdiffrule(M, f, 3)
	derivs = DiffRules.diffrule(M, f, :foo, :bar, :goo)
	@eval begin
	foo, bar, goo = randn(3)
	dx, dy, dz = $(derivs[1]), $(derivs[2]), $(derivs[3])
	if !(isnan(dx))
	@test isapprox(dx, finitediff(x -> $M.$f(x, bar, goo), foo), rtol=0.05)
	end
	if !(isnan(dy))
	@test isapprox(dy, finitediff(y -> $M.$f(foo, y, goo), bar), rtol=0.05)
	end
	if !(isnan(dz))
	@test isapprox(dz, finitediff(z -> $M.$f(foo, bar, z), goo), rtol=0.05)
	end
	end
	=#
	end
	end
	end

.

Tests of such special cases might be best to add below

DiffRules.jl/test/runtests.jl

Line 142 in 2c29d7d

end

, i.e., below the special tests for rem2pi and ldexp.

[devmotion]

Looks good to me, thanks!

The test failure is a downstream issue in ReverseDiff that's also present on the master branch and in ReverseDiff's CI.