Skip to content

Implement vectorized adstock transformations #221

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

Merged
merged 1 commit into from
Apr 6, 2023

Conversation

ricardoV94
Copy link
Contributor

@ricardoV94 ricardoV94 commented Mar 30, 2023

Follow up to #114

Unlike #114 it doesn't try to respect broadcastable flags as those are in a limbo in PyTensor at the moment (see pymc-devs/pytensor#149) and are not respected anywhere else.

Closes #22
Closes #196

@cluhmann
Copy link
Contributor

Ok. Can verify this works as (I) expected in #196. Will review now!

@cluhmann
Copy link
Contributor

cluhmann commented Apr 3, 2023

This looks good to me, though the details of batched_convolution are pretty opaque. @lucianopaz should definitely review that as it was lifted from his PR.

Copy link
Contributor

@lucianopaz lucianopaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest adding a bunch of comments to make the convolution code a bit easier to read

@ricardoV94
Copy link
Contributor Author

Thanks @lucianopaz!

@codecov
Copy link

codecov bot commented Apr 6, 2023

Codecov Report

Merging #221 (b341199) into main (edf0530) will decrease coverage by 0.57%.
The diff coverage is 93.54%.

❗ Current head b341199 differs from pull request most recent head 93f2ac8. Consider uploading reports for the commit 93f2ac8 to get more accurate results

@@            Coverage Diff             @@
##             main     #221      +/-   ##
==========================================
- Coverage   94.83%   94.27%   -0.57%     
==========================================
  Files          18       18              
  Lines         968      978      +10     
==========================================
+ Hits          918      922       +4     
- Misses         50       56       +6     
Impacted Files Coverage Δ
pymc_marketing/mmm/transformers.py 94.44% <93.33%> (-5.56%) ⬇️
pymc_marketing/mmm/delayed_saturated_mmm.py 91.66% <100.00%> (-8.34%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ricardoV94 ricardoV94 force-pushed the vectorized_conv_follow_up branch from b341199 to 93f2ac8 Compare April 6, 2023 08:38
@ricardoV94 ricardoV94 requested a review from lucianopaz April 6, 2023 08:39
@ricardoV94 ricardoV94 added this to the v0.1.0 milestone Apr 6, 2023
@ricardoV94 ricardoV94 merged commit 13949fe into pymc-labs:main Apr 6, 2023
@aseyboldt
Copy link
Contributor

@ricardoV94 Didn't this PR change the runtime of the geometric adstock to something slower (from $O(n)$ to $O(n n_\text{lag})$)?
I didn't perform any testing, and maybe on a GPU it doesn't make as much difference, but at least for longer lag times this should be worse?

@ricardoV94
Copy link
Contributor Author

@ricardoV94 Didn't this PR change the runtime of the geometric adstock to something slower (from O(n) to $O(n n_\text{lag})$)? I didn't perform any testing, and maybe on a GPU it doesn't make as much difference, but at least for longer lag times this should be worse?

Possibly, the concern was vectorization not speed. I didn't write the code, it was actually @lucianopaz, this was a small change from #114

@aseyboldt
Copy link
Contributor

Makes sense. I guess we can just try to remember in case we run into performance issues :-)

@williambdean
Copy link
Contributor

This came up recently #670

@twiecki
Copy link
Contributor

twiecki commented May 23, 2024

Could we keep both implementations?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request MMM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MMM transformer(s) fail when parameters are not scalars? Adstock transformation without for loop
6 participants