-
Notifications
You must be signed in to change notification settings - Fork 218
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
base: main
Are you sure you want to change the base?
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
pymc_marketing/mmm/lift_test.py
Outdated
[df_lift_test_channel_scaled, df_target_scaled, df_sigma_scaled], | ||
axis=1, | ||
) | ||
if "date" in df_lift_test.columns: ## for time_varying |
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.
What about a shorter version like:
components = [df_lift_test_channel_scaled, df_target_scaled, df_sigma_scaled]
if "date" in df_lift_test.columns:
components.append(df_lift_test[["date"]])
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1316 +/- ##
==========================================
+ Coverage 95.10% 95.34% +0.23%
==========================================
Files 44 47 +3
Lines 4703 4966 +263
==========================================
+ Hits 4473 4735 +262
- Misses 230 231 +1 ☔ View full report in Codecov by Sentry. |
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
tests/mmm/test_lift_test.py
Outdated
@@ -474,9 +474,15 @@ def test_scale_lift_measurements(df_lift_test_with_numerics) -> None: | |||
delta_x=lambda row: row["delta_x"] * 2.0, | |||
delta_y=lambda row: row["delta_y"] / 2, | |||
sigma=lambda row: row["sigma"] / 2, | |||
).loc[:, ["channel", "x", "delta_x", "delta_y", "sigma"]] | |||
).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.
It seems that this will always be true because of the change in the fixture
tests/mmm/test_mmm.py
Outdated
mmm.add_lift_test_measurements( | ||
df_lift_test_with_date, | ||
) | ||
|
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 you make an assert statement. Maybe something with the model graph itself
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Description
Related Issue
Checklist
Modules affected
Type of change
📚 Documentation preview 📚: https://pymc-marketing--1316.org.readthedocs.build/en/1316/