pymc-labs / pymc-marketing

Bayesian marketing toolbox in PyMC. Media Mix (MMM), customer lifetime value (CLV), buy-till-you-die (BTYD) models and more.
https://www.pymc-marketing.io/
Apache License 2.0
705 stars 198 forks source link

`michaelis_menten` transformation as `pt.TensorVariable` #1054

Closed PabloRoque closed 4 weeks ago

PabloRoque commented 1 month ago

Make michaelis_meten consistent with the rest of SaturationTransformation by wrapping it as pt.TensorVariable

Description

Related Issue

Checklist

Modules affected

Type of change


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

review-notebook-app[bot] commented 1 month ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

wd60622 commented 1 month ago

I am looking over the _plot_response_curve_fit internals. I think it would be better to rely on the SaturationTransformation.sample_curve method so that the solution doesn't directly call the function method.

There needs to be a solution that works for others custom saturation transformations

I will create an issue for this. We can address in the future #1056

PabloRoque commented 1 month ago

Can you write a test for the case that discovered this bug

Added saturation fixture. Now test_mmm_plots includes MichaelisMentenSaturation. Would fail without TensorVariable in plot_direct_contribution_curves fixture, but does not with the fix in this PR (pending suggested changes)

PabloRoque commented 1 month ago

SaturationTransformation.sample_curve

This will add 3 extra pm.Deterministic to the model. Are we happy we that behavior?

wd60622 commented 1 month ago

This will add 3 extra pm.Deterministic to the model.

Are we happy we that behavior?

Lets skip if that is required. Dont think that would be good

wd60622 commented 1 month ago

Thanks for all the edits @PabloRoque

I am out atm but @juanitorduz will take a look

juanitorduz commented 1 month ago

I'll review on the weekend 🙏💪 (busy week ahead 😄)

PabloRoque commented 1 month ago

No rush! I'll be taking some time off myself until the 28th.