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

Add negative binomial likelihood #63

Merged
merged 26 commits into from
Feb 9, 2022
Merged

Add negative binomial likelihood #63

merged 26 commits into from
Feb 9, 2022

Conversation

simsurace
Copy link
Member

@simsurace simsurace commented Jan 28, 2022

I opted for the p = logistic(f) parameterization. I'm not quite sure whether there are any tradeoffs in choosing this vs. e.g. \mu = exp(f), where \mu is the conditional mean of the negative binomial distribution. The latter parametrization leads to the mean to be only affected by the variance and mean of the latent GP, and not by the likelihood parameter. I haven't thought about whether this has any benefits for parameter learnability that might make it preferable to the p=logistic(f) parameterization.

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #63 (3aa88fb) into master (dc7b997) will increase coverage by 0.39%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #63      +/-   ##
==========================================
+ Coverage   94.66%   95.06%   +0.39%     
==========================================
  Files           8        9       +1     
  Lines          75       81       +6     
==========================================
+ Hits           71       77       +6     
  Misses          4        4              
Impacted Files Coverage Δ
src/likelihoods/gamma.jl 100.00% <ø> (ø)
src/likelihoods/negativebinomial.jl 100.00% <100.00%> (ø)

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 dc7b997...3aa88fb. Read the comment docs.

@simsurace
Copy link
Member Author

Hope the dog is happy now! :)

src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
@simsurace
Copy link
Member Author

simsurace commented Jan 28, 2022

Apparently the keyword argument constructor breaks the test for when the second argument logistic is not being wrapped in the link.
How shall we resolve this? Also introduce the keyword constructor for GammaLikelihood and change the tests?

EDIT: Or shall I revert the constructors and we address this in a future PR?

@devmotion
Copy link
Member

Change the tests similar to https://github.com/JuliaGaussianProcesses/GPLikelihoods.jl/pull/61/files#diff-d19095b0c530c06b29a86657ed746b5d40a0ebf453a75de8b02aa31b6acb119f, I'd suggest. In particular, it would be good to add tests with the keyword argument and to use also non-default values (e.g., different values for r).

src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Show resolved Hide resolved
@simsurace simsurace requested a review from theogf February 7, 2022 18:33
Copy link
Member

@theogf theogf 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, I just made some modifications in the docstring, but feel free to reject/modify them.
Can you also bump the patch version?

src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
src/likelihoods/negativebinomial.jl Outdated Show resolved Hide resolved
simsurace and others added 5 commits February 9, 2022 10:45
Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>
Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>
Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>
Co-authored-by: Théo Galy-Fajou <theo.galyfajou@gmail.com>
@theogf theogf merged commit 21aa15f into JuliaGaussianProcesses:master Feb 9, 2022
@simsurace simsurace deleted the ss/neg_binomial branch February 9, 2022 16:57
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.

3 participants