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 hdi_list kwarg to plot_posterior_predictive #1096

Merged
merged 16 commits into from
Oct 24, 2024
Merged

Conversation

drbenvincent
Copy link
Contributor

@drbenvincent drbenvincent commented Oct 21, 2024

Description

Adds an optional kwarg to the plot_posterior_predictive function. Allows user to provide a list of hdi levels, e.g. [0.94]. Default is the same as before, [0.94, 0.5].

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

@drbenvincent drbenvincent changed the title add hdi_list kwarg to plot_posterior_predictive add hdi_list kwarg to plot_posterior_predictive Oct 21, 2024
Copy link

codecov bot commented Oct 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.56%. Comparing base (e46f690) to head (f4e02dd).
Report is 126 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1096   +/-   ##
=======================================
  Coverage   95.56%   95.56%           
=======================================
  Files          39       39           
  Lines        4015     4018    +3     
=======================================
+ Hits         3837     3840    +3     
  Misses        178      178           

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

@juanitorduz
Copy link
Collaborator

juanitorduz commented Oct 21, 2024

There is one notebook that needs to be updated

 mmm.plot_posterior_predictive(add_hdi=False, add_mean=False, add_gradient=True);   # <-- Here

See the changes in #1072

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@drbenvincent
Copy link
Contributor Author

Should probably do the equivalent changes for sample_prior_predictive and plot_errors? What do you think @juanitorduz / @wd60622 ?

@wd60622
Copy link
Contributor

wd60622 commented Oct 22, 2024

Yeah, that is a good idea. Feel free to do here or in a separate PR.

hdi_list = [0.94]

if hdi_list:
# skipped if hdi_list is empty
Copy link
Contributor

Choose a reason for hiding this comment

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

No comment needed here

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# skipped if hdi_list is empty

if add_hdi:
for hdi_prob, alpha in zip((0.94, 0.50), (0.2, 0.4), strict=True):
if hdi_list is None:
hdi_list = [0.94]
Copy link
Contributor

Choose a reason for hiding this comment

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

What happened with the 0.5 hdi? Are we removing that?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep it :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I maybe misinterpreted a previous reply as being in favour of copying arviz defaults of just the 0.94. But fair enough, I'll change the defaults back to [0.94, 0.5]

@juanitorduz juanitorduz added this to the 0.11.0 milestone Oct 22, 2024
@github-actions github-actions bot added docs Improvements or additions to documentation tests labels Oct 23, 2024
@drbenvincent
Copy link
Contributor Author

I think this should be ready to merge now. I'll add an issue for similar changes to related functions.

@drbenvincent
Copy link
Contributor Author

"Pull Request Labeler" not passing. No idea what that is.

@juanitorduz
Copy link
Collaborator

"Pull Request Labeler" not passing. No idea what that is.

Yeah, this is a new thing we are working on (not a blocker, once is green we can merge)

@github-actions github-actions bot added the enhancement New feature or request label Oct 24, 2024
@juanitorduz
Copy link
Collaborator

@wd60622 the labeling is 🟢 🚀 !

@drbenvincent
Copy link
Contributor Author

I'll let you merge this when you are happy with everything @juanitorduz

Copy link
Contributor

@wd60622 wd60622 left a comment

Choose a reason for hiding this comment

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

LGTM

@juanitorduz juanitorduz merged commit bf86f2c into main Oct 24, 2024
14 checks passed
@juanitorduz juanitorduz deleted the posterior-hdi branch October 24, 2024 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation enhancement New feature or request MMM plots tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow user to specify list of HDI's in plot_posterior_predictive
3 participants