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
668 stars 184 forks source link

Avoid PyTensor evals whenever possible #383

Open cluhmann opened 1 year ago

cluhmann commented 1 year ago

In BaseDelayedSaturatedMMM, the channel_contributions_forward_pass() method calls eval() on the pytensor value to be returned (here). This causes problems if you expect the tensor returned by the various transformation methods to be passed along as-is. For example, if you try to compile a pytensor function that incorporates a call to channel_contributions_forward_pass(), pytensor will complain and break.

Can we avoid calls to eval() whenever possible? Obviously, handing back tensors may be a bit confusing to some users, but having the eval() calls scattered throughout the codebase in unsystematic ways is going to lead to problems.

cluhmann commented 1 year ago

Thinking about this more, it probably makes sense to make any "internal" functions propagate results as tensors and any "user-facing" functions call eval(). Because the eval() mentioned above is in BaseDelayedSaturatedMMM, it should be considered "internal" and thus not call eval(). Seem fair?

cluhmann commented 1 year ago

We may also want to change the names of the base class methods so that they are not overridden by the child class methods. The super.channel_contributions_forward_pass() method may be useful even when you have a BaseDelayedSaturatedMMM object (i.e., the contribution forward pass without the target variable transformation). It would be easy enough to rename the base method to _channel_contributions_forward_pass() or something.

cluhmann commented 1 year ago

This is also relevant to #314