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

Enforce domain checks for media transformations #918

Open
1 of 3 tasks
daniel-saunders-phil opened this issue Aug 9, 2024 · 3 comments
Open
1 of 3 tasks

Enforce domain checks for media transformations #918

daniel-saunders-phil opened this issue Aug 9, 2024 · 3 comments
Labels
good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package help wanted Extra attention is needed MMM

Comments

@daniel-saunders-phil
Copy link

daniel-saunders-phil commented Aug 9, 2024

Checklist:

Adstock Transformations:

Saturation Transformations

  • ...

Any other parameters that would have a restricted domain?

Original message

The docstring says alpha must be between 0 and 1. But you can pass negative and larger alpha values and it will produce strange results, silently. Negative alpha subtracts impressions from each day. Adstock greater than 1 causes exponential accumulation of impressions over time. I can't see users ever finding this behaviour helpful.

import numpy as np
from pymc_marketing.mmm.transformers import geometric_adstock

geometric_adstock(np.array([100,200,50]),alpha = 1.1).eval()
geometric_adstock(np.array([100,200,50]),alpha = -0.1).eval()

Proposed solution: Adding a check would make it easier to spot mistakes, especially in large hierarchical models with complex parameterizations.

    """
    if np.any(alpha >= 1) or np.any(alpha < 0):
            raise ValueError("alpha must be between greater than or equal to 0 and less than 1.")

    w = pt.power(pt.as_tensor(alpha)[..., None], pt.arange(l_max, dtype=x.dtype))
    w = w / pt.sum(w, axis=-1, keepdims=True) if normalize else w
    return batched_convolution(x, w, axis=axis, mode=mode)
@wd60622 wd60622 added the MMM label Aug 9, 2024
@wd60622
Copy link
Contributor

wd60622 commented Aug 10, 2024

Think this is a good idea. Probably want to use pymc / pytensor tooling though for the checks instead of numpy. The pymc.logprob.utils.CheckParameterValue can be used.

Example:
https://github.com/pymc-devs/pymc-experimental/blob/af91b423095eb37017eb84ded91c14f0eae08494/pymc_experimental/distributions/continuous.py#L331

@wd60622 wd60622 added the good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package label Aug 10, 2024
@wd60622
Copy link
Contributor

wd60622 commented Aug 10, 2024

Thoughts @juanitorduz ?

@juanitorduz
Copy link
Collaborator

Yes! We should use pytensor.

@wd60622 wd60622 added the help wanted Extra attention is needed label Nov 14, 2024
@wd60622 wd60622 changed the title Geometric ad stock accepts alpha values outside its domain Enforce domain checks for saturation transformations Nov 14, 2024
@wd60622 wd60622 changed the title Enforce domain checks for saturation transformations Enforce domain checks for media transformations Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers . Doesn't require extensive knowledge of the repo and package help wanted Extra attention is needed MMM
Projects
None yet
Development

No branches or pull requests

3 participants