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

PabloRoque · 2024-09-20T10:28:26Z

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

Description

~~Rename old michaelis_menten as michaelis_menten_function. Made to avoid major changes in mmm.utils.estimate_menten_parameters's L70.~~
~~Wrap previous michaelis_menten_function as pt.TensorVariable inside michaelis_menten~~
~~Minor change in test_michaelis_menten. The TensorVariable now needs to be evaluated.~~
~~Minor changes to type hints~~
Wrap MichaelisMentenSaturation.functionwith pt.as_tensor_variable
Expand test_plotting.mock_mmm fixture with MichaelisMentenSaturation
.eval() in example notebook

Related Issue

Closes MMM.plot_direct_contribution_curves errors when using MichaelisMentenSaturation #1050
Related to #

Checklist

Checked that the pre-commit linting/style checks pass
Included tests that prove the fix is effective or that the new feature works
Added necessary documentation (docstrings and/or example notebooks)
If you are a pro: each commit corresponds to a relevant logical change

Modules affected

MMM
CLV

Type of change

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

review-notebook-app · 2024-09-20T10:42:13Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

wd60622 · 2024-09-20T10:57:04Z

pymc_marketing/mmm/transformers.py

@@ -914,6 +914,31 @@ def michaelis_menten(
    return alpha * x / (lam + x)


+def michaelis_menten(


Do we need two functions?

Can all of the SaturationTransformations be wrapped in as_tensor_variable instead of making these changes?

ie. here:

pymc-marketing/pymc_marketing/mmm/components/base.py

Line 556 in f70db33

return self.function(x, **kwargs)

Do we need two functions?

We don't need them, but made my life easier to avoid the pesky L70

Can all of the SaturationTransformations be wrapped in as_tensor_variable instead of making these changes?
Do you have anything in mind? I can think of 2 ways:

Adding boilerplate to all the classes. Perhaps defining a decorator to reduce boilerplate.

Rely on Transformation._checks() but here we would need to trust the type hints to use something like signature(class_function).return_annotation and wrap pt.as_tensor_variable if needed.

Perhaps you have something less convoluted in mind?

Can you not add it in the one location of the apply method? (where I linked) That is a common method that is not overwritten by the subclasses

I think having it for apply would make sure that other custom saturation transformations will not encounter this found bug as well

It shouldn't hurt to has pt.as_tensor_variable wrapping an already TensorVariable. pytensor will figure out an optimization

Can you not add it in the one location of the apply method? (where I linked) That is a common method that is not overwritten by the subclasses

The linked apply method needs to be called within a model context. Not the case in the plotting function.

The proposed sample_curve might work though, since it opens a model context. Having a look

Do we want to add another pm.Deterministic to the model?
It would be the case if we use _sample_curve

We wouldnt want to use the private methods of the saturation transformation. Does sample_curve with fit_result not get us close to the data that is plotted? (Just not scaled on x and y)

wd60622 · 2024-09-20T11:57:33Z

pymc_marketing/mmm/transformers.py

+    alpha: float,
+    lam: float,


I still don't know the best type hints here. But TensorVariable is also accepted here

I still don't know the best type hints here. But TensorVariable is also accepted here

Reverted changes to type hints

wd60622 · 2024-09-20T11:59:09Z

pymc_marketing/mmm/utils.py

@@ -22,7 +22,7 @@
 import xarray as xr
 from scipy.optimize import curve_fit, minimize_scalar

-from pymc_marketing.mmm.transformers import michaelis_menten
+from pymc_marketing.mmm.transformers import michaelis_menten_function


 def estimate_menten_parameters(


Where is this still used? I thought there was a generalization of this? @cetagostini

Created an issue here: #1055

wd60622

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

wd60622 · 2024-09-20T12:19:22Z

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 · 2024-09-20T14:59:52Z

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 · 2024-09-20T15:25:07Z

SaturationTransformation.sample_curve

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

wd60622 · 2024-09-20T23:04:39Z

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 · 2024-09-20T23:08:31Z

pymc_marketing/mmm/transformers.py

@@ -826,10 +826,10 @@ def tanh_saturation_baselined(
    return gain * x0 * pt.tanh(x * pt.arctanh(r) / x0) / r


-def michaelis_menten(
+def michaelis_menten_function(


This is only used in the functions we will deprecate, right?

This is only used in the functions we will deprecate, right?

Yes, they are only used in the methods linked in #1055.
If we remove those, there is no need to define the extra function.

Mind closing that now?

Hi @wd60622,

having doubts about completely removing functions in #1055.

As an example, take estimate_menten_parameters. It was introduced in #329, and never used outside of the current test. I am guessing the original purpose was to double check that the introduced function gives correct results. If we remove the tests, we would never be checking that again.

As alternative I've simplified the PR to wrap only MichaelisMentenSaturation.function with pt.as_tensor_variable.

Note that the newly introduced fixture now passes the tests

If we need to remove tests, that is fine.

This is only saturation transformation which needs to be defined with two functions (which I find silly). Just cosolidate

wd60622 · 2024-10-08T15:59:54Z

Thanks for all the edits @PabloRoque

I am out atm but @juanitorduz will take a look

juanitorduz · 2024-10-08T16:12:07Z

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

PabloRoque · 2024-10-08T17:15:47Z

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

juanitorduz

This is looking good! Shall we merge this one and continue with other smaller PRs?

wd60622

Looks good. Let's merge and continue elsewhere if needed

PabloRoque added 2 commits September 20, 2024 11:16

Wrap michaelis_menten as TensorVariable

b1d63de

Ammend alpha, lam type hints

d2eb266

PabloRoque changed the title ~~michaelis_menten tranformation as pt.TensorVariable~~ michaelis_menten transformation as pt.TensorVariable Sep 20, 2024

Evaluate TensorVariable in example notebook

0c26c0e

wd60622 reviewed Sep 20, 2024

View reviewed changes

wd60622 requested changes Sep 20, 2024

View reviewed changes

Add saturation fixture

245f771

PabloRoque requested a review from wd60622 September 20, 2024 15:43

wd60622 requested a review from juanitorduz September 20, 2024 22:54

wd60622 reviewed Sep 20, 2024

View reviewed changes

PabloRoque added 2 commits September 23, 2024 11:29

Remove extra definitions and wrap as_tensor_variable

04e6367

Revert type michaelis_menten type changes

11d0288

wd60622 added MMM bug Something isn't working labels Sep 28, 2024

PabloRoque mentioned this pull request Oct 7, 2024

Deprecate several pymc_marketing.mmm.utils functions #1078

Merged

14 tasks

PabloRoque requested a review from wd60622 October 8, 2024 09:50

Merge branch 'main' into menten-as-tensorvariable

6fdcc1d

juanitorduz added this to the 0.10.0 milestone Oct 16, 2024

Merge branch 'main' into menten-as-tensorvariable

5eee159

juanitorduz approved these changes Oct 17, 2024

View reviewed changes

wd60622 approved these changes Oct 17, 2024

View reviewed changes

juanitorduz merged commit 429b955 into pymc-labs:main Oct 17, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

PabloRoque commented Sep 20, 2024 •

edited

Loading

review-notebook-app bot commented Sep 20, 2024

wd60622 Sep 20, 2024

wd60622 Sep 20, 2024 •

edited

Loading

PabloRoque Sep 20, 2024

wd60622 Sep 20, 2024 •

edited

Loading

wd60622 Sep 20, 2024

wd60622 Sep 20, 2024

PabloRoque Sep 20, 2024

PabloRoque Sep 20, 2024 •

edited

Loading

PabloRoque Sep 20, 2024 •

edited

Loading

wd60622 Sep 20, 2024

wd60622 Sep 20, 2024

PabloRoque Sep 23, 2024

wd60622 Sep 20, 2024

wd60622 Sep 20, 2024

wd60622 left a comment

wd60622 commented Sep 20, 2024 •

edited

Loading

PabloRoque commented Sep 20, 2024

PabloRoque commented Sep 20, 2024

wd60622 commented Sep 20, 2024

wd60622 Sep 20, 2024

PabloRoque Sep 21, 2024

wd60622 Sep 21, 2024

PabloRoque Sep 23, 2024

wd60622 Sep 26, 2024

wd60622 commented Oct 8, 2024

juanitorduz commented Oct 8, 2024

PabloRoque commented Oct 8, 2024

juanitorduz left a comment

wd60622 left a comment

		@@ -914,6 +914,31 @@ def michaelis_menten(
		return alpha * x / (lam + x)


		def michaelis_menten(

michaelis_menten transformation as pt.TensorVariable #1054

michaelis_menten transformation as pt.TensorVariable #1054

Conversation

PabloRoque commented Sep 20, 2024 • edited Loading

Description

Related Issue

Checklist

Modules affected

Type of change

review-notebook-app bot commented Sep 20, 2024

Choose a reason for hiding this comment

wd60622 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wd60622 Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PabloRoque Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

PabloRoque Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wd60622 left a comment

Choose a reason for hiding this comment

wd60622 commented Sep 20, 2024 • edited Loading

PabloRoque commented Sep 20, 2024

PabloRoque commented Sep 20, 2024

wd60622 commented Sep 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wd60622 commented Oct 8, 2024

juanitorduz commented Oct 8, 2024

PabloRoque commented Oct 8, 2024

juanitorduz left a comment

Choose a reason for hiding this comment

wd60622 left a comment

Choose a reason for hiding this comment

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

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

PabloRoque commented Sep 20, 2024 •

edited

Loading

wd60622 Sep 20, 2024 •

edited

Loading

wd60622 Sep 20, 2024 •

edited

Loading

PabloRoque Sep 20, 2024 •

edited

Loading

PabloRoque Sep 20, 2024 •

edited

Loading

wd60622 commented Sep 20, 2024 •

edited

Loading