Skip to content

Conversation

@devmotion
Copy link
Member

I am in the process of updating DistributionsAD to ChainRules 1. I thought it would be a good opportunity to reduce the amount of type piracy and move rules for functions in Distributions here. Currently, there's only one such rule in DistributionsAD. It defines the pullback for the pdf of the PoissonBinomial distribution, so the PR adds and tests only this rule.

ChainRulesCore 1 is a very lightweight package (see @oxinabox's comment for more details) and Distributions already depends on ChainRulesCore indirectly via SpecialFunctions and StatsFuns (support for ChainRules was added in version 0.9.10 which was released today). DistributionsAD does not support ChainRulesCore 1 yet, so the definitions in Distributions won't lead to any conflicts with DistributionsAD if we remove the rrules in DistributionsAD before we tag a CR1-compatible release.

The implementation is based on the same dynamic programming "trick" as the implementation of poissonbinomial_pdf. I didn't find any reference for it, I only derived it a while ago and added it to DistributionsAD. However, I am pretty confident that the ChainRules tests utilities would have revealed any errors.

/ cc: @bdeonovic who added the primal definition in #1198

@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2021

Codecov Report

Merging #1390 (bed3009) into master (59df675) will increase coverage by 0.09%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
+ Coverage   82.44%   82.54%   +0.09%     
==========================================
  Files         116      116              
  Lines        6911     6950      +39     
==========================================
+ Hits         5698     5737      +39     
  Misses       1213     1213              
Impacted Files Coverage Δ
src/univariate/discrete/poissonbinomial.jl 93.57% <100.00%> (+2.48%) ⬆️

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 59df675...bed3009. Read the comment docs.

@matbesancon
Copy link
Member

Looks good, I had tried to start something in https://github.com/matbesancon/DistributionsDiff.jl/ but never got around sitting to continue this

@matbesancon
Copy link
Member

do you want to implement frule in this PR or a following one?

@devmotion
Copy link
Member Author

I'll add the frules in this PR 👍 I did not include them in DistributionsAD since the only AD backend in DistributionsAD with CR support is Zygote, so it didn't seem necessary. However, I guess we should add frules in Distributions as well.

@devmotion
Copy link
Member Author

I added them 👍

@devmotion devmotion merged commit 0cda70a into master Aug 30, 2021
@devmotion devmotion deleted the dw/chainrules branch August 30, 2021 20:08
@storopoli
Copy link

Great! Is this is the first AD in Distributions.jl?

@devmotion
Copy link
Member Author

AFAIK yes. gradlogpdf has existed for a long time but there were no AD backends used in the package. There are many tests with ForwardDiff though.

@bdeonovic
Copy link
Contributor

Great! Is this is the first AD in Distributions.jl?

Yes; but only because AD generally works off the shelf without having to implement anything extra.

@oschulz
Copy link
Contributor

oschulz commented Aug 31, 2021

Awesome!

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 this pull request may close these issues.

7 participants