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

Add BetaGeoBetaBinomModel #1031

Merged
merged 12 commits into from
Sep 13, 2024
Merged

Add BetaGeoBetaBinomModel #1031

merged 12 commits into from
Sep 13, 2024

Conversation

ColtAllen
Copy link
Collaborator

@ColtAllen ColtAllen commented Sep 13, 2024

Description

Reopening #922 due to the force push.

The BG/BB model is for non-contractual purchase opportunities across discrete time periods; a good example would be sporting events.

Sampling is rather slow because NUTS defaults to compound sampling due to the discrete distributions used in this model. I recommend that #707 be merged to speed up performance.

A small addition to CLVBaseModel was required for the input validations, and the dev notebook is rather minimalist right now but will be expanded into a full how-to in a future PR. An inefficiency in distribution_new_customers that is shared by all CLV models was also identified and will be fixed in a separate PR.

Related Issue

Checklist

Modules affected

  • MMM
  • CLV

Type of change

  • New feature / enhancement
  • Bug fix
  • Documentation
  • Maintenance
  • Other (please specify):

📚 Documentation preview 📚: https://pymc-marketing--1031.org.readthedocs.build/en/1031/

@ColtAllen ColtAllen added this to the 0.9.0 milestone Sep 13, 2024
@ColtAllen ColtAllen self-assigned this Sep 13, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@ColtAllen
Copy link
Collaborator Author

Is there a ruff version discrepancy between the CI and the pre-commit config? I'm not getting these errors when working locally and all dependencies are up to date.

@juanitorduz
Copy link
Collaborator

pre-commit.ci autofix

@juanitorduz
Copy link
Collaborator

Fixed by b3abdab using #1031 (comment)

@ColtAllen If you wanna add some changed make sure to pull first ;)

@juanitorduz
Copy link
Collaborator

@ColtAllen regarding

Sampling is rather slow because NUTS defaults to compound sampling due to the discrete distributions used in this model. I recommend that #707 be merged to speed up performance.

Do you see a speed-up with this model or all CLV models?

Copy link
Collaborator

@juanitorduz juanitorduz left a comment

Choose a reason for hiding this comment

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

Fantastic work @ColtAllen !

@juanitorduz
Copy link
Collaborator

@ColtAllen

This test is very slow:

6182.49s call     tests/clv/models/test_beta_geo_beta_binom.py::TestBetaGeoBetaBinomModel::test_model_convergence[mcmc-0.1]

Any ideas if we can make it a bit faster?

@ColtAllen
Copy link
Collaborator Author

Do you see a speed-up with this model or all CLV models?

#707 only impacts BetaGeoBetaBinomModel.

This test is very slow. Any ideas if we can make it a bit faster?

Merging #707 would make it about 3x faster. Only other way to speed it up would be to use a smaller fit dataset. There's another test here that is also rather slow, but addressing the TODOs will fix that.

@juanitorduz
Copy link
Collaborator

Ok! Then from my side we can merge this one and lets work on those to-dos 🙂

@ColtAllen ColtAllen merged commit 660686b into pymc-labs:main Sep 13, 2024
11 checks passed
@ColtAllen ColtAllen deleted the bgbb_model branch September 13, 2024 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants