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.

Sign up for GitHub

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

Jump to bottom

Make MMM Plots tests run faster #363

Closed
wants to merge 2 commits into from
Closed

Make MMM Plots tests run faster #363

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b0a247
make tests run faster
juanitorduz Aug 18, 2023
64b0ba1
reduce tune
juanitorduz Aug 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions tests/mmm/test_plotting.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
@pytest.fixture(scope="module")
def toy_X() -> pd.DataFrame:
date_data: pd.DatetimeIndex = pd.date_range(
start="2019-06-01", end="2021-12-31", freq="W-MON"
start="2020-06-01", end="2021-12-31", freq="W-MON"
)

n: int = date_data.size
Expand Down Expand Up @@ -62,19 +62,21 @@ class ToyMMM(BaseDelayedSaturatedMMM, MaxAbsScaleTarget):
mmm = ToyMMM(
date_column="date",
channel_columns=["channel_1", "channel_2"],
adstock_max_lag=4,
adstock_max_lag=2,
)
elif control == "with_controls":
mmm = ToyMMM(
date_column="date",
adstock_max_lag=4,
adstock_max_lag=2,
control_columns=["control_1", "control_2"],
channel_columns=["channel_1", "channel_2"],
)
# fit the model
mmm.fit(
X=toy_X,
y=toy_y,
chains=2,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to sample? In many CLV tests we create a dummy idata with prior draws as if they were posterior.

If you really need to sample, maybe also reduce tune?

Copy link
Collaborator Author

@juanitorduz juanitorduz Aug 18, 2023 •
edited by ricardoV94
Loading

Choose a reason for hiding this comment

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

Actually, I think the bottleneck comes from https://github.com/pymc-labs/pymc-marketing/blob/main/pymc_marketing/model_builder.py#L308 as we are not use the samples argument in the plot_prior which is one of the slowest tests. @michaelraczycki can we add **kwargs to be also passed to the sample_prior_predictive?

Copy link
Contributor

@ricardoV94 ricardoV94 Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Is it really wise to call all 3 sampling methods when "fitting" the model?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess this discussion might be beyond this PR as it is more about the fit scope in the ModelBuilder. I suggest we merge these small changes which make the ci time from 33min -> 27 min and then iterate on the prior_predictive_sample arguments. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure that is out of scope.

I still don't think we should be doing mcmc sampling at all just for a plotting test. Definitely don't need prior/posterior and posterior predictive draws?

Copy link
Collaborator Author

@juanitorduz juanitorduz Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I still don't think we should be doing mcmc sampling at all just for a plotting test.

ok! so I then suggest we close this PR and I will open another one where I try to do it with dummy idata as suggested.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll make the additional sampling (prior and posterior predictive) optional, controlled by boolean parameters. That way we can make it more compact by people that actually want to get all samples straight away, and not force anyone to burn computing time if they don't want to. Fine with you? @juanitorduz off, I'll include it in the same PR, I'm also adjusting the docstrings in model_builder, as they need some more attention

draws=20,
)
mmm._prior_predictive = mmm.prior_predictive
mmm._fit_result = mmm.fit_result
Expand Down