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

Dependent density notebook update #706

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

fonnesbeck
Copy link
Member

@fonnesbeck fonnesbeck commented Sep 21, 2024

Updated code to v5, sampling to NUTS


📚 Documentation preview 📚: https://pymc-examples--706.org.readthedocs.build/en/706/

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Copy link

review-notebook-app bot commented Sep 28, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-28T05:36:17Z
----------------------------------------------------------------

Line #9.        beta = pm.Normal("beta", 0.0, 5.0, dims=("one", "K"))

I quite dislike this one dummy dimension. We could eliminate it and use broadcast multiplication instead of a dot product. The dot seems inappropriate -- the shapes appear to be (n_obs, 1), (1, K),which is just an outer product between two vectors. So make everything 1d and use pt.outer.


Copy link

review-notebook-app bot commented Sep 28, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-28T05:36:18Z
----------------------------------------------------------------

Line #10.        x = pm.Data("x", std_range)

Add dims?


Copy link

review-notebook-app bot commented Sep 28, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-28T05:36:19Z
----------------------------------------------------------------

Remove references to PyMC3 here


Copy link

review-notebook-app bot commented Sep 28, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-28T05:36:19Z
----------------------------------------------------------------

Line #4.        mu = pm.Deterministic("mu", gamma + x @ delta)

Same comment as above, delta should have dims=("K",),and write gamma + pt.outer(x, delta)

Also mu is missing dims


Copy link

review-notebook-app bot commented Sep 28, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-28T05:36:20Z
----------------------------------------------------------------

Line #4.        obs = pm.NormalMixture("obs", w, mu, tau=tau, observed=y)

dims on obs (and y)


Copy link

review-notebook-app bot commented Sep 28, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-28T05:36:21Z
----------------------------------------------------------------

Looks like every draw diverged. Going to need to tune this to make it work with NUTS.


@jessegrabowski
Copy link
Member

Made some comments -- there are a few references to PyMC3 that need to be removed, plus a lot of nitpicks. The biggest problem is in the sampling. Every draw is divergent :( Maybe try nutpie?

@fonnesbeck
Copy link
Member Author

Oof, totally missed the divergences. Odd that the resulting fit was so good.

I tried nutpie and numpyro, but it does not seem to like the stick-breaking piece (even after fixing the outer products). Works okay with Metropolis, so for expediency I'm sticking with this.

Copy link

review-notebook-app bot commented Sep 30, 2024

View / edit / reply to this conversation on ReviewNB

jessegrabowski commented on 2024-09-30T14:47:46Z
----------------------------------------------------------------

typo: September 2024


@jessegrabowski
Copy link
Member

Random question: we have pm.StickBreakingWeights, is that something that could be used for this model instead of the hand-rolled function?

@fonnesbeck
Copy link
Member Author

We would have to reframe the problem somewhat, as StickBreakingWeights models them as a function of a concentration parameter, rather than the linear model used here.

@jessegrabowski
Copy link
Member

Ok. That doesn't sound worth it? I was just curious.

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.

2 participants