-
Notifications
You must be signed in to change notification settings - Fork 192
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
Media transformation sampling & plotting methods #734
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
==========================================
- Coverage 93.60% 93.48% -0.12%
==========================================
Files 27 27
Lines 2722 2841 +119
==========================================
+ Hits 2548 2656 +108
- Misses 174 185 +11 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments :)
ax: plt.Axes | None = None, | ||
hdi_kwargs: dict | None = None, | ||
plot_kwargs: dict | None = None, | ||
) -> plt.Axes: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually return the figure... shall we keep that convention (no strong opinion)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am leaning toward that as well.
I am adding the support to sample the curves based on parameter distributions that could include other dimensions (i.e. channel) and think support for plotting
i.e. 3 channels in the dims would lead to figure with 3 axes each with their saturation curve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok! No strong preference. Just think about it from a user perspective :)
Still need to add:
|
This should be good to go @juanitorduz The plots adapt to the "Curve" that is used as argument. For instance, unless specified, the priors will have only chain, draw coords as dims wouldn't be known / are all the same transformation = LogisticSaturation()
parameters= transformation.sample_prior()
curve = transformation.sample_curve(parameters)
transformation.plot_curve(curve)
plt.show() coords = {"channel": ["A", "B", "C"]}
parameters= transformation.sample_prior(coords=coords) # This could be from posterior with additional coords for transformation
curve = transformation.sample_curve(parameters)
transformation.plot_curve(curve)
plt.show() |
Two tests are failing: FAILED tests/mmm/test_delayed_saturated_mmm.py::test_plotting_media_transform_workflow[adstock]
FAILED tests/mmm/test_delayed_saturated_mmm.py::test_plotting_media_transform_workflow[saturation] |
Somewhere the |
@wd60622 this looks great! I just left a question :) |
We have these bad guys failing FAILED tests/mmm/components/test_base.py::test_new_transformation_plot_curve[coords0-1]
FAILED tests/mmm/components/test_base.py::test_new_transformation_plot_curve[coords1-3] :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update the branch and merge! Cool work!
* add plotting methods * add tests for new methods * saturation support for additional variable dims * consolidate the logic of sampling * change warning * workflow from a fitted model * change order of tests * suggestion to use names * because of new data --------- Co-authored-by: Juan Orduz <[email protected]>
* add plotting methods * add tests for new methods * saturation support for additional variable dims * consolidate the logic of sampling * change warning * workflow from a fitted model * change order of tests * suggestion to use names * because of new data --------- Co-authored-by: Juan Orduz <[email protected]>
Description
This adds new methods for sampling and plotting the media transformations. They are:
sample_prior
: sample the parameter distributions for the transformationsample_curve
: sample the curve given a set of parameters. These can be prior or posteriorplot_curve
: Plot curve HDI andn
random curve samples. Usesplot_curve_hdi
andplot_curve_samples
methodsThis creates a nice pre-modeling workflow of:
And also model analysis
Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--734.org.readthedocs.build/en/734/