-
Notifications
You must be signed in to change notification settings - Fork 252
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
HSGP as component #1246
HSGP as component #1246
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1246 +/- ##
==========================================
- Coverage 95.27% 95.02% -0.26%
==========================================
Files 40 42 +2
Lines 4234 4439 +205
==========================================
+ Hits 4034 4218 +184
- Misses 200 221 +21 ☔ View full report in Codecov by Sentry. |
Checkout the documentation section here: Thoughts on using the One aspect that is tough is that the new parametrization is different than the Would people want to do something like this? from pymc_marketing.prior import Prior
eta = Prior("HalfNormal", sigma=Prior("HalfNormal"), dims="channel")
ell = Prior("HalfNormal", dims="channel")
hsgp = HSGP(dims=("time", "channel"), eta=eta, ell=ell) TODO:
@cetagostini I was mistaken when I talked out it being hierachical, it is not. pymc-marketing/pymc_marketing/hsgp_kwargs.py Lines 537 to 543 in f8975a9
|
Question for @bwengals : |
This looks very nice! In the long run, this seems like a nicer approach. As we have not reached a v1.0.0 version I think it's fine that the API changes from time to time |
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.
This looks great @wd60622 ! I really like the proposal! A couple of questions and comments:
- Do we need more tests for the
hsgp_kwargs
? What is missing in this PR? - It would be great to update the GP MMM notebooks (maybe different PR) to showcase this component point of view.
- Can we also do HSGPPeriodic? I think modeling the media time-varying coefficients as a periodic GP is something we have been asked for.
pymc_marketing/hsgp_kwargs.py
Outdated
|
||
|
||
def pc_prior_1d(alpha: float = 0.1, lower: float = 1.0) -> Prior: | ||
R"""Create a one-dimensional PC prior for GP lengthscale. |
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.
Can we describe what "PC" stands for?
Do we have any references we can add on this?
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.
Penalized complexity. Here's the reference: https://arxiv.org/abs/1503.00256
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.
It'd be nice to have the PC prior available, but IMO a better default prior for PyMC marketing would be InverseGamma, and then have the user give a lower and upper bound, as described here: https://betanalpha.github.io/assets/case_studies/gaussian_processes.html
I think ideally:
- user specifies a lower bound and prior mass -> PC prior
- user specifies a lower, upper bound and mass -> InverseGamma
pymc_marketing/hsgp_kwargs.py
Outdated
) | ||
|
||
|
||
def approx_hsgp_hyperparams( |
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 thought this function was already available in pymc
(gp.hsgp_approx.py
)
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.
Doesn't seem to be in pymc 5.13.0: https://github.com/pymc-devs/pymc/blob/v5.13.0/pymc/gp/hsgp_approx.py#L603
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.
To do above
Part of todo for the components notebook. Need to think about how this integrates into MMM and also MMM notebook
Do you know if the heuristic still works for that one as well? |
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
View / edit / reply to this conversation on ReviewNB cetagostini commented on 2024-12-02T17:40:49Z Line #9. media_tvp_config = { This should change to something like:
|
Indeed, maybe something related to latent makes more sense to me, or time. If this is a component already, should be under component folders, and the name should be related to what are we trying to model. We want to model saturation, then we have the saturation component, if we want to model adstock, then we have the adstock component, same thing if we want to model time/latent effects. |
Yes! |
Sorry i missed this. Yes sometimes you might want to use the centered parameterization, though non-centered is probably the better default. |
) | ||
|
||
|
||
def create_m_and_L_recommendations( |
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.
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.
Can we add some references on where these heuristic come from (just the paper reference, ideally the section)
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 think we can use the same ref as above)
).constrain(lower=lower, upper=upper, mass=mass) | ||
|
||
|
||
def approx_hsgp_hyperparams( |
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.
Is it the same as in https://github.com/pymc-devs/pymc/blob/main/pymc/gp/hsgp_approx.py#L97 ?
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.
Not in the min pymc version that we support. How should we handle?
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 actually think this is great for a first iteration and I think we can improve this over time :)
I just left some comments on the references and some apparent dupplicated code from pymc
Need to double check which I am using as the default. I will set non-centered as default |
What is the status on |
They will probably go after we support the latest versions (some important fixes were made) |
@wd60622 what is the status on this one :) ? |
Ready to merge. Will add to example notebooks later |
Later like in another PR |
Merged ! Cool stuff @wd60622 ! |
Description
Adding
HSGP
component which acts a bit more likePrior
class than MMMcomponents but with some
plot_curve
logic as well.Bringing back the PR that @bwengals started for better priors for the 1d HSGP
EDIT: I've implemented this with the
HSGP.parameterize_from_data
class method:We might want to refactor this to pull out the defaults instead of having them
as
property
of the class. Maybe like:Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1246.org.readthedocs.build/en/1246/