-
Notifications
You must be signed in to change notification settings - Fork 192
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 BetaGeoBetaBinom
Distribution Block
#431
Conversation
I'll try to check this week @ColtAllen |
I pushed a version of the logp that works with Scan. I think we can make it work without a Scan, by evaluating on the maximum T-tx range. I just don't know if this results in much wasted computation in general use cases (for instance if there's one datapoint with a very large range, but most other datapoints have smaller ranges) |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #431 +/- ##
==========================================
- Coverage 92.25% 92.08% -0.17%
==========================================
Files 24 24
Lines 2414 2490 +76
==========================================
+ Hits 2227 2293 +66
- Misses 187 197 +10 ☔ View full report in Codecov by Sentry. |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
I added the likelihood expression to the docstrings, along with a dev notebook. Not sure if the LaTeX rendered properly in the docstrings though, because the docs build is failing due to a
|
In general I don't see very large values of |
It's now called |
Scan is the simplest approach |
Tests are still failing with |
Yes you need to bump the oldest supported pymc version to the most recent |
In here and in the ci wofkflow file: https://github.com/pymc-labs/pymc-marketing/blob/main/pyproject.toml#L20 |
I had to increase |
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.
@ColtAllen this looks good! I wanna merge this one and iterate (I can help with that). I have just two requests :D :
- Remove commented code
- Fix pre-commit (I think Ruff is complaining)
Otherwise LGTM.
First point is done, but Ruff is complaining about a LaTeX expression in the docstring that is too long. I'm not sure if there's any way to shorten it that doesn't prevent the docstring from rendering properly. |
Doesn't seem to be possible to turn off and on at the moment based on this ruff issue. Maybe you can define the string outside of the class then use f-string. Could reduce 8 something characters on a line. |
I think we can simply do it as in https://github.com/pymc-labs/pymc-marketing/blob/main/pymc_marketing/clv/distributions.py#L407 |
Thanks @ColtAllen ! Let's now work on the model 🎉 ! |
* init commit * removed scan * Fix logp * Remove print statement * Add test for logp notimplemented errors * docstrings * dev notebook added * updated to vectorize_graph * import order * update oldest pymc_version * Update ci.yml pymc version * Update pyproject.toml pymc version * WIP sample prior testing * sample prior compared against lifetimes * increase rtol * remove commented code, add logp reference * fix latex docstring * notebook testing and misc edits * revert latex in docstring * add ruff ignore comment --------- Co-authored-by: Ricardo Vieira <[email protected]>
* init commit * removed scan * Fix logp * Remove print statement * Add test for logp notimplemented errors * docstrings * dev notebook added * updated to vectorize_graph * import order * update oldest pymc_version * Update ci.yml pymc version * Update pyproject.toml pymc version * WIP sample prior testing * sample prior compared against lifetimes * increase rtol * remove commented code, add logp reference * fix latex docstring * notebook testing and misc edits * revert latex in docstring * add ruff ignore comment --------- Co-authored-by: Ricardo Vieira <[email protected]>
This PR resurrects #188 to partially address #176.
The Beta-Geometic/Beta Binomial model is a CLV model for discrete, non-contractual use cases - a good example would be sporting events. This PR adds a distribution block to be used for posterior predictive checks and the
logp
in the full model which will be added in a separate PR.For now this is a draft PR because I can't get the
logp
to return the correct values; for reference please see (5) on p4 of the research paper:https://www.brucehardie.com/papers/020/fader_et_al_mksc_10.pdf
I could use some help with the
pytensor
functions this requires.📚 Documentation preview 📚: https://pymc-marketing--431.org.readthedocs.build/en/431/