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

ForwardDiff of logpdf of NegativeBinomial gives wrong results for p==1 #1582

Closed
simsurace opened this issue Jul 4, 2022 · 4 comments · Fixed by #1583
Closed

ForwardDiff of logpdf of NegativeBinomial gives wrong results for p==1 #1582

simsurace opened this issue Jul 4, 2022 · 4 comments · Fixed by #1583

Comments

@simsurace
Copy link
Contributor

simsurace commented Jul 4, 2022

This was discovered and documented in #1568 via tests, but #1579 reverted those tests. I still think this is worth tracking:

using Distributions, ForwardDiff
using Distributions: digamma

mydiffp(r, p, k) = isone(p) && iszero(k) ? r : r/p - k/(1 - p)
mydiffr(r, p, k) = log(p) - inv(k + r) - digamma(r) + digamma(r + k + 1)

f1(_p) = logpdf(NegativeBinomial(1.0, _p), 0)
f2(_r) = logpdf(NegativeBinomial(_r, 1.0), 0)

ForwardDiff.derivative(f1, 1.0)  mydiffp(1.0, 1.0, 0) # false
ForwardDiff.derivative(f2, 1.0)  mydiffr(1.0, 1.0, 0) # false
@simsurace
Copy link
Contributor Author

Should be solved by JuliaDiff/ForwardDiff.jl#481

@devmotion
Copy link
Member

That is not required. I found a fix (and a bug on the way) without it.

@devmotion
Copy link
Member

devmotion commented Jul 4, 2022

Basically,

  • we can eliminate the special case branch alltogether by using xlogy and xlog1py
  • we just have to fix the derivatives of these functions, defined in DiffRules and LogExpFunctions

I'll open the three required PRs.

@simsurace
Copy link
Contributor Author

These tests pass with JuliaDiff/DiffRules.jl#85 and #1583.

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