Skip to content

Support negative-binomial observations in the Horseshoe sampler#101

Merged
brandonwillard merged 1 commit into
AmpersandTV:mainfrom
xjing76:gibbs_nb_expansion
Dec 13, 2021
Merged

Support negative-binomial observations in the Horseshoe sampler#101
brandonwillard merged 1 commit into
AmpersandTV:mainfrom
xjing76:gibbs_nb_expansion

Conversation

@xjing76

@xjing76 xjing76 commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

Here is a functionally working branch of HSstep with NB expansion. However, in terms of sampling results of betas. the result is still a bit off.

@brandonwillard brandonwillard added enhancement New feature or request samplers labels Dec 1, 2021
brandonwillard
brandonwillard suggested changes Dec 1, 2021
View reviewed changes
Comment thread pymc3_hmm/step_methods.py Outdated
Comment thread pymc3_hmm/step_methods.py Outdated
Comment thread pymc3_hmm/step_methods.py Outdated
Comment thread tests/test_step_methods.py
@brandonwillard

brandonwillard commented Dec 1, 2021

Copy link
Copy Markdown
Contributor

I've pushed some of the updates mentioned in my comments.

brandonwillard
brandonwillard suggested changes Dec 1, 2021
View reviewed changes

@brandonwillard brandonwillard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While updating the tests, I noticed that there's an unsupported observation distribution case for HSStep that isn't raising an exception (i.e. the second test in test_HSStep_unsupported).

@brandonwillard brandonwillard force-pushed the gibbs_nb_expansion branch 6 times, most recently from 46c079b to ead5a00 Compare December 2, 2021 00:53
@brandonwillard brandonwillard changed the title Add NB expansion-in HS step Support negative-binomial observations in the Horseshoe sampler Dec 2, 2021
brandonwillard
brandonwillard suggested changes Dec 2, 2021
View reviewed changes

@brandonwillard brandonwillard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just added the missing polyagamma requirement to the CI test setup. We still need coverage for some lines, though. @xjing76, can you add tests for those cases?

@xjing76

xjing76 commented Dec 2, 2021

Copy link
Copy Markdown
Contributor Author

I just added the missing polyagamma requirement to the CI test setup. We still need coverage for some lines, though. @xjing76, can you add tests for those cases?

Yes!

@xjing76 xjing76 force-pushed the gibbs_nb_expansion branch 7 times, most recently from 24d79cf to cab572e Compare December 3, 2021 15:36
@xjing76

xjing76 commented Dec 6, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Did some additional exploration of the convergence issue with NB expansion.
We found that the HS step would converge very slowly towards the true beta.

So I did some more exploration, on the NegativeBinomial portion itself. And I set up the problem without the HSstep.

For N_draws = 50
Even settingbeta as a Normal prior, with transformation (exp or abs_) on the eta, would some what affect the ability for the NUTS sampler to converge unless the initial values starts very closely from the true_beta.

However, with Metropolis sampling on beta the convergence much better with same number of draws. I am not 100% sure where the issue is coming from

https://gist.github.com/xjing76/1fe297253adb5a58c721c6cc4f38b446

@xjing76 xjing76 force-pushed the gibbs_nb_expansion branch 3 times, most recently from 05c4804 to 98698e2 Compare December 6, 2021 22:07
@brandonwillard brandonwillard force-pushed the gibbs_nb_expansion branch 2 times, most recently from bd463d3 to e5e02cd Compare December 7, 2021 00:44
@xjing76 xjing76 force-pushed the gibbs_nb_expansion branch 4 times, most recently from d94e4ca to 892fe52 Compare December 9, 2021 12:32
brandonwillard
brandonwillard suggested changes Dec 10, 2021
View reviewed changes

@brandonwillard brandonwillard left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good; just needs to be rebased and squashed (there are two commits with the same title).

@brandonwillard brandonwillard merged commit a90b139 into AmpersandTV:main Dec 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@brandonwillard brandonwillard Awaiting requested review from brandonwillard

Assignees

@xjing76 xjing76

Labels

enhancement New feature or request samplers

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xjing76 @brandonwillard