Skip to content

Differentiation Dirichlet #1534

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 30 commits into from
Jul 31, 2022
Merged

Differentiation Dirichlet #1534

merged 30 commits into from
Jul 31, 2022

Conversation

matbesancon
Copy link
Member

This adds the ChainRules rules for Dirichlet, constructor and logpdf

@matbesancon matbesancon requested a review from devmotion April 24, 2022 08:21
@matbesancon
Copy link
Member Author

Some tests are performed with a manual finite diff with specific tangents that remain in the domain

devmotion
devmotion reviewed Apr 24, 2022
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll do a proper review later, here just two quick initial comments.

@codecov-commenter
Copy link

codecov-commenter commented Apr 24, 2022 •
edited
Loading

Codecov Report

Merging #1534 (7297260) into master (7c3af32) will increase coverage by 0.10%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1534      +/-   ##
==========================================
+ Coverage   85.74%   85.84%   +0.10%     
==========================================
  Files         129      129              
  Lines        7975     8018      +43     
==========================================
+ Hits         6838     6883      +45     
+ Misses       1137     1135       -2
Impacted Files Coverage Δ
src/multivariate/dirichlet.jl 77.91% <100.00%> (+5.09%) ⬆️
src/show.jl 100.00% <0.00%> (+2.32%) ⬆️

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 7c3af32...7297260. Read the comment docs.

devmotion
devmotion reviewed Apr 24, 2022
View reviewed changes
@matbesancon
Copy link
Member Author

On why _logpdf and not logpdf directly, it was to be matching the implementation signature.
We could add an rrule and frule for logpdf on all distributions that would just call into that of _logpdf?

@matbesancon
Copy link
Member Author

For the tests, the rrule one fails even with forward finite diff, the finite diff version ends ups being NaN

devmotion
devmotion reviewed Apr 25, 2022
View reviewed changes
@devmotion
Copy link
Member

For the tests, the rrule one fails even with forward finite diff, the finite diff version ends ups being NaN

I assume the problem with finite differencing in general is that perturbing x leads to values that are not in the support anymore which therefore yield infinite log probabilities.

devmotion
devmotion reviewed Apr 28, 2022
View reviewed changes
@devmotion devmotion mentioned this pull request May 23, 2022
Merged
devmotion and others added 2 commits May 24, 2022 15:28
@devmotion
Simplify implementation and tests in #1534 (#1555)
9234155 
* Simplify implementation and tests

* Precompute `digamma(alpha0)`

* Relax type signature
@matbesancon
conflict
2c7100e
devmotion
devmotion approved these changes May 26, 2022
View reviewed changes
Copy link
Member

@devmotion devmotion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, is there anything else you would like to include in this PR?

@matbesancon matbesancon marked this pull request as draft May 27, 2022 15:25
@matbesancon
Copy link
Member Author

setting it as draft to not merge it yet, I have to sort out some things before

@matbesancon matbesancon marked this pull request as ready for review July 31, 2022 07:11
@matbesancon
Copy link
Member Author

Merging as already discussed and approved

@matbesancon matbesancon merged commit b957872 into master Jul 31, 2022
@matbesancon matbesancon deleted the cr-dirichlet branch July 31, 2022 08:50
@matbesancon
Copy link
Member Author

matbesancon commented Oct 11, 2022 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@devmotion devmotion devmotion approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@matbesancon @codecov-commenter @devmotion