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

Natural parameter distributions #189

Merged
merged 9 commits into from
Jan 19, 2022
Merged

Natural parameter distributions #189

merged 9 commits into from
Jan 19, 2022

Conversation

ThijsvdLaar
Copy link
Collaborator

@ThijsvdLaar ThijsvdLaar commented Dec 8, 2021

Implement natural parameter definitions from https://github.com/semihakbayrak/ForneyLab.jl/tree/CVI Additionaly:

  • Refactor definitions
  • Add tests

@codecov-commenter
Copy link

codecov-commenter commented Dec 8, 2021

Codecov Report

Merging #189 (eb89500) into master (208861e) will increase coverage by 0.22%.
The diff coverage is 98.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
+ Coverage   86.50%   86.72%   +0.22%     
==========================================
  Files          95       95              
  Lines        4395     4461      +66     
==========================================
+ Hits         3802     3869      +67     
+ Misses        593      592       -1     
Impacted Files Coverage Δ
src/factor_nodes/log_normal.jl 87.80% <80.00%> (+3.59%) ⬆️
...c/engines/julia/update_rules/nonlinear_extended.jl 100.00% <100.00%> (ø)
.../engines/julia/update_rules/nonlinear_unscented.jl 89.52% <100.00%> (ø)
src/factor_nodes/bernoulli.jl 84.21% <100.00%> (+1.85%) ⬆️
src/factor_nodes/beta.jl 87.50% <100.00%> (+1.38%) ⬆️
src/factor_nodes/categorical.jl 77.19% <100.00%> (+1.72%) ⬆️
src/factor_nodes/dirichlet.jl 87.50% <100.00%> (+1.25%) ⬆️
src/factor_nodes/gamma.jl 85.00% <100.00%> (+1.66%) ⬆️
src/factor_nodes/gaussian.jl 100.00% <100.00%> (ø)
src/factor_nodes/poisson.jl 93.33% <100.00%> (+1.33%) ⬆️
... and 3 more

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 208861e...eb89500. Read the comment docs.

@albertpod
Copy link
Collaborator

albertpod commented Dec 15, 2021

Looks good. But why do we add this to FL?

EDIT: for PR#188

@ThijsvdLaar
Copy link
Collaborator Author

Great, thanks; it's some prep work for natural gradient descent. I tried to refactor Semih's work (#183) in smaller chunks that fit my head.

@semihakbayrak
Copy link
Contributor

Thanks @ThijsvdLaar for your contribution! I liked how you implemented StandardDist function directly with distribution and variate types rather than their instances. For the logpdf function of exponential family (EF) of distributions, how about implementing just one function: log(h(x)) + transpose(ϕ(x))*η - logNormalizer(dist,η) in probability_distributions.jl file instead of defining them separately for the distributions? We can then implement components of logpdf separately for each distribution, as it has been done for logNormalizer, so as to use them in other applications when necessary. For example, I can imagine that sufficient statistics is useful in many applications and would be nice to have this function in FL.

@ThijsvdLaar
Copy link
Collaborator Author

I agree that implementing a single log-pdf function is more beautiful from a mathematical point of view, but from a coding point-of view it would add complexity. We would have to define h and \phi functions for each distribution type, which are not used anywhere else outside of the log-pdf function. Since there is currently no case for reuse of these statistics, I would prefer the more direct approach.

Copy link
Contributor

@semihakbayrak semihakbayrak left a comment

Choose a reason for hiding this comment

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

Thanks Thijs! Ready to be merged, in my opinion.

@ThijsvdLaar ThijsvdLaar merged commit 30933f2 into master Jan 19, 2022
@ThijsvdLaar ThijsvdLaar deleted the natural branch January 19, 2022 18:14
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.

4 participants