-
Notifications
You must be signed in to change notification settings - Fork 230
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 thin wrapper around advi functionality #1365
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1365 +/- ##
===========================================
- Coverage 93.94% 55.97% -37.98%
===========================================
Files 48 48
Lines 5137 5156 +19
===========================================
- Hits 4826 2886 -1940
- Misses 311 2270 +1959 ☔ View full report in Codecov by Sentry. |
@wd60622 do you think it's more appropriate for @PabloRoque if your primary motivation is to speed up model fits, have you tried using |
Maybe long term but implementing now would require changing the fit api for all |
Thanks for the suggestion @ColtAllen. The issue is mainly that our customer base has 1E8 order of magnitude. I am afraid I'll hit the wall using mcmc when we put things on PROD, regardless of the sampler (normally I use numpyro), so I'm starting to explore advi. |
pymc_marketing/clv/models/basic.py
Outdated
stacklevel=2, | ||
) | ||
with self.model: | ||
mean_field_approx = pm.fit(**{"method": "advi"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this syntax? Will you add more kwargs here?
Just use method=advi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was planning to add more kwargs.
I added now functionality to parse the params and kwargs for both fit
and sample
Co-authored-by: Will Dean <57733339+wd60622@users.noreply.github.com>
I would encourage you to try For datasets with millions of customers I usually recommend just using MAP. MAP is generally discouraged in the Bayesian community, but these CLV models are an exception due to the strong population assumptions underlying them. Beyond 30-50k customers, mean values for MAP and MCMC fits are identical. The only downside is that you'll lose the credibility intervals with MAP. |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Thanks for the advices @ColtAllen. I added a gist comparing DEMZ with ADVI and FULLRANK:
I the light of your advices, and the little evidence I gathered myself, I'll be closing this PR Thank you all! |
@PabloRoque glad to help! Looks like the gist you did for |
I am sorry to be the bearer of "bad news", but I may be reopening this PR. I've been able to improve the fit adjusting The updated gist shows:
Some considerations, though.
On a side note, I also tried to use |
I'm not opposed to getting this through even if it is not "recommended" way of sampling the current models. Support for minibatching is another beast. What work would be required there? |
Yes, I briefly looked into a My preference would be to make this more of a long-term PR for @PabloRoque I've got a lot on my plate at the moment, but will make time to view your other PRs tomorrow & over the weekend. We're pushing to release pymc-marketing v0.11.0 ASAP. |
@ColtAllen No rush! I had a bit of bandwidth and attempted a few contributions, but I am not expecting a quick review in any case. |
Description
Introduces a thin wrapper around
pm.fit
so thatadvi
can be used in CLV modelsSome tests related to
advi
functionalityRelated Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1365.org.readthedocs.build/en/1365/