-
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
GammaGammaModel
Docstrings and API Standardization
#758
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #758 +/- ##
==========================================
+ Coverage 93.87% 93.89% +0.01%
==========================================
Files 28 28
Lines 2972 2948 -24
==========================================
- Hits 2790 2768 -22
+ Misses 182 180 -2 ☔ View full report in Codecov by Sentry. |
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.
Lookin good!! Just left some small comments. Just ping me again when you are ready for a final review.
He y @ColtAllen is this one ready fro a final review or merge? |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
It's ready for review now, but there's a merge conflict in the Quickstart notebook and I'm unable to resolve it. |
This reverts commit a3154d9.
Ups! Notebooks are a pain! I suggest removing it from the PR, rebase and commit it again (or open another PR with the notebook later) https://stackoverflow.com/questions/38743912/remove-a-file-from-a-git-pull-request |
Can you please push an empty commit to trigger the CI again? (I seems its stuck) |
I've reverted the CLV Quickstart changes. I'll do one more quick PR for this (and any other discrepancies in the docs) before the next release. |
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.
LGTM!
Before you merge, could you please fix the two small doc issues below? Thanks :D
@ColtAllen is this one ready to be merged 🟢 ? |
Aslo, do you think this is enough for 0.7.0 (in view of #527 ?) |
Yes; the remaining tasks in that issue are I'll get started on the PR today for the CLV Quickstart and docstring proof-reading. |
GammaGammaModel
API ImprovementsGammaGammaModel
Docstrings and API Standardization
* utils.customer_lifetime_value * expected_customer_lifetime_value * WIP clv.models.gamma_gamma.py * gamma_gamma API * fixed circular import * gamma_gamma tests * delete tests/datasets/test_summary.csv * clv test_utils.py * remove expected_purchases(future_t=0) * remove monetary_value arg * WIP docstrings * notebooks * docstrings * Revert "notebooks" This reverts commit a3154d9. * gamma-gamma notebook * docstrings
* utils.customer_lifetime_value * expected_customer_lifetime_value * WIP clv.models.gamma_gamma.py * gamma_gamma API * fixed circular import * gamma_gamma tests * delete tests/datasets/test_summary.csv * clv test_utils.py * remove expected_purchases(future_t=0) * remove monetary_value arg * WIP docstrings * notebooks * docstrings * Revert "notebooks" This reverts commit a3154d9. * gamma-gamma notebook * docstrings
Description
This is the final PR to standardize the CLV API prior to the new release. It also speeds up the
customer_lifetime_value
function quite a bit because it gets rid of a FOR loop initialization that raises a DIV/0 warning.Right now it's still a draft so that I can review the docs prebuild to make any necessary edits to the docstrings. The Quickstart and Gamma-Gamma notebooks will also require revision.
It's worth noting there's an additional Gamma-Gamma model for individual customers that isn't referenced in any of the notebooks.
Related Issue
future_t=0
predictions fromexpected_customer_lifetime_value
function #596Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--758.org.readthedocs.build/en/758/