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

Improved Default Priors for BetaGeoModel #1264

Merged
merged 10 commits into from
Dec 16, 2024

Conversation

ColtAllen
Copy link
Collaborator

@ColtAllen ColtAllen commented Dec 12, 2024

Description

This PR modifies the default model config of BetaGeoModel with a prior specification for the dropout process similar to that in BetaGeoBetaBinomModel. This improved parameter estimates and reduced fit times by 40-50% in testing.

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

@ColtAllen ColtAllen added docs Improvements or additions to documentation enhancement New feature or request CLV priority: medium model config labels Dec 12, 2024
@ColtAllen ColtAllen self-assigned this Dec 12, 2024
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 tests label Dec 12, 2024
Copy link

codecov bot commented Dec 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.06%. Comparing base (66a3f3c) to head (1613149).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1264   +/-   ##
=======================================
  Coverage   95.05%   95.06%           
=======================================
  Files          42       42           
  Lines        4451     4456    +5     
=======================================
+ Hits         4231     4236    +5     
  Misses        220      220           

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

@ColtAllen ColtAllen marked this pull request as ready for review December 13, 2024 00:06
@ColtAllen ColtAllen added this to the 0.11.0 milestone Dec 13, 2024
@github-actions github-actions bot added good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package priority: low priors labels Dec 14, 2024
@wd60622
Copy link
Contributor

wd60622 commented Dec 14, 2024

can you add graphviz method instead of importing to close out #1138

Copy link

review-notebook-app bot commented Dec 14, 2024

View / edit / reply to this conversation on ReviewNB

wd60622 commented on 2024-12-14T05:29:20Z
----------------------------------------------------------------

How about just model? This makes me think that there is a different way to initialize a non-mcmc model.

But couldn't I use same model for multiple fits? that is:

python

model = clv.BetaGeoModel(data=data)

idata_mcmc = model.fit(method="mcmc")

idata_map = model.fit(method="map")


Copy link

review-notebook-app bot commented Dec 14, 2024

View / edit / reply to this conversation on ReviewNB

wd60622 commented on 2024-12-14T05:29:21Z
----------------------------------------------------------------

Can you explicitly say by adding "a" and "b" priors to the model_config


@ColtAllen
Copy link
Collaborator Author

can you add graphviz method instead of importing to close out #1138

I'll create another PR for this prior to merging this one. If it can be added to ModelBuilder I'd prefer to remove the existing MMM method.

@juanitorduz
Copy link
Collaborator

After addressing @wd60622 's comments we can merge this one :)

@ColtAllen
Copy link
Collaborator Author

can you add graphviz method instead of importing to close out #1138

I'll create another PR for this prior to merging this one. If it can be added to ModelBuilder I'd prefer to remove the existing MMM method.

#1284

@ColtAllen ColtAllen merged commit 95db52b into pymc-labs:main Dec 16, 2024
19 of 20 checks passed
@ColtAllen ColtAllen deleted the bg_default_priors branch December 16, 2024 06:25
aseyboldt pushed a commit to aseyboldt/pymc-marketing that referenced this pull request Dec 20, 2024
* BG docstrings

* doctrings and default_model_config

* readd bg_nbd nb and revise config

* clv dev nb cleanup

* beta_geo.ipynb

* quickstart

* notebooks
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 first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package model config priority: low priority: medium priors tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update default_model_config for BetaGeoModel
3 participants