-
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
keep date column in test_scale_lift_measurements #1316
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1316 +/- ##
=======================================
Coverage 95.35% 95.35%
=======================================
Files 47 47
Lines 4995 4997 +2
=======================================
+ Hits 4763 4765 +2
Misses 232 232 ☔ View full report in Codecov by Sentry. |
pymc_marketing/mmm/lift_test.py
Outdated
else: | ||
components = [df_lift_test_channel_scaled, df_target_scaled, df_sigma_scaled] | ||
|
||
return pd.concat(components, axis=1) |
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 would be great if you could add a test. Maybe @wd60622 can help you with this (I do not know how easy is to add it into the current test-mocking flow :) )
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.
Thanks for making the change @malitsadok1 !
For a test, I think it would be suffice to show that the model with tvp_media
can be built and add a lift test. If you are unfamiliar with pytest, let me know and I can help out.
Also make sure that any current tests will pass
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Hi @malitsadok1 |
).loc[ | ||
:, | ||
["channel", "x", "delta_x", "delta_y", "sigma"] | ||
+ (["date"] if "date" in df_lift_test_with_numerics.columns else []), |
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 this ever not true?
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.
Hi @malitsadok1 |
no it's only one line of code I thought I can skip it but I try to add it
*Mali Tsadok, Statistician*
Bell Statistics LTD
0546206144/ ***@***.*** ***@***.***>
…On Fri, Jan 10, 2025 at 9:02 AM Will Dean ***@***.***> wrote:
Hi @malitsadok1 <https://github.com/malitsadok1>
Do you have precommit installed in your dev setup?
—
Reply to this email directly, view it on GitHub
<#1316 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A6BIIAW4CC4PGGLE4SHOWBT2J5V7LAVCNFSM6AAAAABUGIODNWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKOBRHEZDGOJTGI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Yeah, it is easy to install and run. It should be easy to resolve this locally. If not, try the # type: ignore |
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.
Thanks @malitsadok1
Description
Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1316.org.readthedocs.build/en/1316/