Add rrule for logpdf of NegativeBinomial

[simsurace]

This needs to be tested, and not entirely sure about the edge case.

[devmotion]

Can you add tests with ChainRulesTestUtils.test_rrule? It should cover in-support, out-support values, and the special case.

[codecov-commenter]

Codecov Report

Merging #1568 (be49b02) into master (6ab4c1f) will increase coverage by 0.00%.
The diff coverage is 85.71%.

@@           Coverage Diff           @@
##           master    #1568   +/-   ##
=======================================
  Coverage   85.52%   85.52%           
=======================================
  Files         128      128           
  Lines        7861     7882   +21     
=======================================
+ Hits         6723     6741   +18     
- Misses       1138     1141    +3     

Impacted Files	Coverage Δ
src/univariate/discrete/negativebinomial.jl	90.54% <85.71%> (-1.92%)	⬇️

---

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 6ab4c1f...be49b02. Read the comment docs.

[simsurace]

There are some errors in testing the rrule (as opposed to test failures). I'm not completely sure I'm using the test correctly, but the fact that the test is hitting negative parameters could be an issue of the finite differencing scheme, right?

[devmotion]

Possibly. Probably you can fix these by using e.g. forward_fdm (https://github.com/JuliaDiff/FiniteDifferences.jl#dealing-with-singularities=) in the test_rrule (see e.g. https://github.com/JuliaStats/Distributions.jl/pull/1534/files).

[matbesancon]

https://github.com/JuliaDiff/FiniteDifferences.jl#dealing-with-singularities
(the link had an equal at the end that broke it)

[simsurace]

Tests for NegativeBinomial are passing locally.

EDIT: scratch that, apparently there are some parameters that still fail.

[simsurace]

I managed to make all tests pass except the ones for p = 1 - eps(). I could not find any finite difference scheme that would give close-to-reasonable results. Maybe someone else knows how to fix this.

[simsurace]

I made the following changes:

• Use randomly sampled parameters
• Include the r derivative in the tests for ForwardDiff
• Test the edge case p=1, k=0 also for ForwardDiff
• Compare the ForwardDiff to the finite difference result instead of the analytical formula

As it now turns out, the ForwardDiff derivative wrt. p for p=1 seems to be incorrect as well.
Do we need a diffrule for this?

[devmotion]

I'm not really happy with the changes of the tests. Generally, I think we should try to avoid making any unrelated changes also of the tests since that possibly unintentionally relaxes some existing tests and hence makes it easier for bugs to sneak in in the future.

My suggestion is to keep it really simple and just add some test_rrules that cover all branches. Due to the numerical issues of finite differencing we can't just not add it inside the loop for ForwardDiff. IMO that's fine, just test some other parameters in a separate loop (if you want to test multiple parameters).

The ForwardDiff issue might be fixed by JuliaDiff/ForwardDiff.jl#481 (assuming it's about a measure-zero branch). DiffRules won't help here since then the result will be non-deterministic: If users load ForwardDiff before Distributions, the rules won't be picked up by ForwardDiff.

[simsurace]

Ok, I think I might have misunderstood your previous comment then:

I would prefer to use finite differencing in the tests - you don't want to use it in practice due to the numerical issues and inaccuracies but it's the simplest way to compute derivatives, and hence the one least likely to be systematically incorrect. ForwardDiff derivatives can be incorrect in some special cases but it can be tricky to notice and debug, and analytical derivatives can be wrong and were used to define the rules.

I took this to mean that while we're at it, we should use finite differencing also to test ForwardDiff against, given that previously it was tested against the analytical derivative, to have uniformity between ForwardDiff and the rrule.

The new ForwardDiff tests are more comprehensive because they now also cover edge cases and the derivative wrt. r. But they don't sample the more extreme values as well as the previous tests. Of course this is a consequence of changing to finite differencing to compare ForwardDiff results against.

Please let me know how you would like me to proceed. Some of the following options may not be independent of each other:

• Would you like to keep the new parameter ranges for ForwardDiff, or have separate ranges for ForwardDiff and rrule?
• Would you like to keep the new test of ForwardDiff for the derivative wrt. r?
• Would you like to keep the new tests of ForwardDiff in the edge case?
• Would you like to test ForwardDiff against analytical or finite differences?

[simsurace]

I rearranged the tests. ForwardDiff is now tested against analytical derivatives and in the old parameters ranges as before, but additionally there are tests

• In the edge case
• For the derivative wrt. to r

As some of them fail for some parameters or for all parameters, they are marked with @test_skip or @test_broken, respectively.

[devmotion]

I noticed that even the current iteration of the tests doesn't actually test all branches and contains changes unrelated to this PR. I thought maybe it's easier and faster if I implement directly what I had in mind, so I opened #1579 since I was not sure if you are OK with me pushing to this PR. I could also push my commit to this PR if you prefer. In any case I'd like to merge one of the PRs since I think the rrule is very useful 🙂

[simsurace]

No worries, you can close this PR and merge the other one.

[simsurace]

This is superseded by #1579