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

Modified Beta Geometric model #1301

Merged
merged 25 commits into from
Dec 24, 2024
Merged

Conversation

PabloRoque
Copy link
Contributor

@PabloRoque PabloRoque commented Dec 19, 2024

Description

Add Modified beta geometric model.
Credit to @ColtAllen and @CamDavidsonPilon, this is basically a copy and paste of their methods

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--1301.org.readthedocs.build/en/1301/

@github-actions github-actions bot added CLV tests enhancement New feature or request wontfix This will not be worked on labels Dec 19, 2024
Copy link

codecov bot commented Dec 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.13%. Comparing base (85f9016) to head (d494fff).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1301      +/-   ##
==========================================
+ Coverage   95.03%   95.13%   +0.10%     
==========================================
  Files          43       44       +1     
  Lines        4530     4629      +99     
==========================================
+ Hits         4305     4404      +99     
  Misses        225      225              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

),
)

def expected_num_purchases(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this method just to deprecate?

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, this should be deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is to be deleted, then we should as well delete the associated BetaGeoModel.expected_num_purchases.

Notice that we are inheriting, and a user may by mistake call the method.

Copy link
Collaborator

@ColtAllen ColtAllen Dec 20, 2024

Choose a reason for hiding this comment

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

Best to do this in a separate PR prior to merging this one. I don't think it should be added to the release notes because it's not deprecation in the truest sense of the term, but rather renaming a method.

tests/clv/models/test_modified_beta_geo.py Outdated Show resolved Hide resolved
tests/clv/models/test_modified_beta_geo.py Show resolved Hide resolved
Copy link
Collaborator

@ColtAllen ColtAllen left a comment

Choose a reason for hiding this comment

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

Thanks for opening this PR @PabloRoque! Can you also add a dev notebook for this model to docs/source/notebooks/clv/dev? Nothing fancy - just need to visually inspect model.idata and some fit diagnostics. You can use the first 21 cells in the BetaGeoNBD Notebook as a template.

pymc_marketing/clv/models/modified_beta_geo.py Outdated Show resolved Hide resolved
pymc_marketing/clv/models/modified_beta_geo.py Outdated Show resolved Hide resolved
),
)

def expected_num_purchases(
Copy link
Collaborator

Choose a reason for hiding this comment

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

No, this should be deleted.

pymc_marketing/clv/models/modified_beta_geo.py Outdated Show resolved Hide resolved
pymc_marketing/clv/models/modified_beta_geo.py Outdated Show resolved Hide resolved
pymc_marketing/clv/models/modified_beta_geo.py Outdated Show resolved Hide resolved
tests/clv/models/test_modified_beta_geo.py Outdated Show resolved Hide resolved
tests/clv/models/test_modified_beta_geo.py Outdated Show resolved Hide resolved
@PabloRoque
Copy link
Contributor Author

Thanks for the quick review. Let me address your suggestions during the day.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the docs Improvements or additions to documentation label Dec 20, 2024
@PabloRoque
Copy link
Contributor Author

Can you also add a dev notebook for this model to docs/source/notebooks/clv/dev?

Added.

The watermark states:
pymc : 5.19.1
pytensor: 2.26.4

This is a bit ahead of the current main and related to #1269, but shouldn't affect the PR itself

Copy link
Collaborator

@ColtAllen ColtAllen left a comment

Choose a reason for hiding this comment

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

expected_probability_no_purchase is being inherited from BetaGeoModel. It was derived by @DylanZammit and does not appear in research:

[https://github.com//issues/1093]

We should override that method in this model with a Exception message that it is unsupported and delete the corresponding notebook section until we can verify the math derivations. Please feel free to create a separate issue for the derivations.

@PabloRoque
Copy link
Contributor Author

expected_probability_no_purchase

Added raise NotImplementedError. Removed the part in the notebook.

@PabloRoque PabloRoque requested a review from ColtAllen December 20, 2024 13:57
@ColtAllen
Copy link
Collaborator

ColtAllen commented Dec 20, 2024

expected_probability_no_purchase

Added raise NotImplementedError. Removed the part in the notebook.

Let's remove the corresponding tests as well and add one that catches the NotImplementedError.

Copy link
Contributor Author

Added Weibull prior section


View entire conversation on ReviewNB

Copy link
Contributor Author

Added raise NotImplementedError on this method


View entire conversation on ReviewNB

…iated tests. Add raise NotImplementedError test
@PabloRoque
Copy link
Contributor Author

Let's remove the corresponding tests as well and add one that catches the NotImplementedError.

Done

@ColtAllen
Copy link
Collaborator

Only comment left to be addressed is the removal of expected_num_purchases.

If this is to be deleted, then we should as well delete the associated BetaGeoModel.expected_num_purchases. Notice that we are inheriting, and a user may by mistake call the method.

Let's leave it as-is for now and we can remove expected_num_purchases from both models in a follow-up PR.

@ColtAllen
Copy link
Collaborator

Nice work so far! It'd be a lot easier to review the docstrings if the readthedocs preview was working properly, but lately that CI job has been crashing: #1286

Copy link
Collaborator

@ColtAllen ColtAllen left a comment

Choose a reason for hiding this comment

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

We'll also need to add the notebook to./docs/source/notebooks/index.md so that it shows up in the docs, and add links to the README, Index, and top of the CLV Quickstart. Again, all stuff that's easier to check if the CI task is fixed.

Copy link
Collaborator

Let's remove the Weibull comparisons; thanks for checking them.


View entire conversation on ReviewNB

Copy link

review-notebook-app bot commented Dec 21, 2024

View / edit / reply to this conversation on ReviewNB

ColtAllen commented on 2024-12-21T11:33:08Z
----------------------------------------------------------------

Clear the outputs from this cell.


Copy link

review-notebook-app bot commented Dec 21, 2024

View / edit / reply to this conversation on ReviewNB

ColtAllen commented on 2024-12-21T11:33:09Z
----------------------------------------------------------------

Thanks for running this experiment. Looks like Weibull priors aren't ideal for the default config. We can delete this section and just compare MCMC & MAP fits.


PabloRoque commented on 2024-12-23T08:40:19Z
----------------------------------------------------------------

Deleted

Copy link

review-notebook-app bot commented Dec 21, 2024

View / edit / reply to this conversation on ReviewNB

ColtAllen commented on 2024-12-21T11:33:09Z
----------------------------------------------------------------

This comment will need to be updated for ModifiedBetaGeoModel.


PabloRoque commented on 2024-12-23T08:40:52Z
----------------------------------------------------------------

Do you have a suggestion for this comment?

ColtAllen commented on 2024-12-24T14:01:00Z
----------------------------------------------------------------

Just remove it for now

Copy link
Contributor Author

Deleted


View entire conversation on ReviewNB

Copy link
Contributor Author

Do you have a suggestion for this comment?


View entire conversation on ReviewNB

Copy link
Collaborator

Just remove it for now


View entire conversation on ReviewNB

@ColtAllen ColtAllen merged commit 3a98c8a into pymc-labs:main Dec 24, 2024
20 checks passed
@ColtAllen
Copy link
Collaborator

Thanks for adding this @PabloRoque! I'll review the pre-release docs and make some minor edits in charts, docstrings, etc. before the next release.

@PabloRoque
Copy link
Contributor Author

Nice Xmas gift!

Thanks for all the feedback and the time you took to dig into the details

@PabloRoque PabloRoque deleted the modified-beta-geo branch December 24, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLV docs Improvements or additions to documentation enhancement New feature or request tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Modified BetaGeo Model
4 participants