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

Allow plot_expected_purchases_pcc in BetaGeoModel and ModifiedBetaGeoModel #1470

Merged

Conversation

PabloRoque
Copy link
Contributor

@PabloRoque PabloRoque commented Feb 5, 2025

Description

  • Remove Exception in plot_expected_purchases_pcc.
  • Update notebooks to now supported plots
  • FIX: patch ModifiedBetaGeoNBDRV internal sim_data

Related Issue

Checklist


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

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 docs Improvements or additions to documentation CLV enhancement New feature or request good second issue Bit more involved but still doable for newcomers priority: high labels Feb 5, 2025
@github-actions github-actions bot added the tests label Feb 5, 2025
Copy link

codecov bot commented Feb 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.58%. Comparing base (596e7d4) to head (d4793ab).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1470      +/-   ##
==========================================
- Coverage   92.58%   92.58%   -0.01%     
==========================================
  Files          52       52              
  Lines        6045     6043       -2     
==========================================
- Hits         5597     5595       -2     
  Misses        448      448              

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

@PabloRoque PabloRoque mentioned this pull request Feb 6, 2025
6 tasks
Comment on lines +169 to +170
"alpha_prior": Prior("Weibull", alpha=2, beta=10),
"r_prior": Prior("Weibull", alpha=2, beta=1),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why this change? (I'm not opposed, just curious.)

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.

Want to get your opinion on the default prior change in BetaGeoModel, but otherwise this is good to merge!

@PabloRoque
Copy link
Contributor Author

Want to get your opinion on the default prior change in BetaGeoModel, but otherwise this is good to merge!

I) If we use :

"phi_dropout_prior": Prior("HalfFlat"),
"kappa_dropout_prior": Prior("HalfFlat"),

Then model.fit(fit_method="map") errors: Logp initial evaluation results: {'alpha': 0.0, 'r': 0.0, 'phi_dropout': 0.0, 'kappa_dropout': 0.0, 'recency_frequency': -inf} You can call model.debug() for more details.

II) If we just modify those 2 but leave "alpha_prior": Prior("HalfFlat") or "r_prior": Prior("HalfFlat") then: clv.plot_expected_purchases_ppc(model, ppc="prior") fails:

@classmethod
def rng_fn(cls, rng, size):
    raise NotImplementedError("Cannot sample from half_flat variable")

Also, we should add an Issue to experiment with the default priors.
I've noticed in #1430 that the agreement with the actuals in plot_expected_purchases_over_time is heavily dependent on the choice of priors for small datasets. We should have some sensible defaults that minimize the bias.

@PabloRoque PabloRoque enabled auto-merge (squash) February 11, 2025 11:44
@PabloRoque PabloRoque merged commit cf3eab2 into pymc-labs:main Feb 11, 2025
17 of 18 checks passed
@PabloRoque PabloRoque deleted the beta-geo-plot-expected-purchases-ppc branch February 11, 2025 11:45
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 good second issue Bit more involved but still doable for newcomers priority: high tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add plot_expected_purchases_pcc support to BG/NBD and MBG/NBD Models
2 participants