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.

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

Add example of multiple geo lift test analysis #338

Merged
merged 24 commits into from
Jun 21, 2024
Merged

Conversation

drbenvincent
Copy link
Collaborator

@drbenvincent drbenvincent commented May 7, 2024

Create new classes?

The PR currently does not add any additional code or classes. All the new functionality is embedded within the example notebook. It is relatively simple, we are just creating an aggregate treated region or iterating through each treated geo. So at the moment this uses an approach of transparency so that users can see what is being done etc.

However, part of the point of CausalPy is to provide a simple to use API, not requiring the user to do that much manual python coding. So the question is whether we should create new classes / an API. It would be something along the lines of:

For the pooled approach:

result = multi_cell_geo_test_aggregate(
    df, 
    agg_func=median,
    experiment=cp.pymc_experiments.SyntheticControl,
    expt_kwargs={"treatment_time": treatment_time,
                 "formula": formula},
    model=cp.pymc_models.WeightedSumFitter,
    model_kwargs={sample_kwargs: {"target_accept": 0.95, "random_seed": seed}},
)

For the unpooled approach:

results = multi_cell_geo_test_unpooled(
    df,
    treated_geos=treated,
    untreated_geos=untreated,
    experiment=cp.pymc_experiments.SyntheticControl,
    expt_kwargs={"treatment_time": treatment_time,
                 "formula": formula},
    model=cp.pymc_models.WeightedSumFitter,
    model_kwargs={sample_kwargs: {"target_accept": 0.95, "random_seed": seed}},
)

So the trade-off would be having a relatively clean API, but at the expense of making the operation a little more opaque. The manual python code in the notebook (as it stands right now) is not that complex. So I think it's not overwhelmingly obvious which we should go with.

TODO's based on feedback so far

  • Check the pre-commit checks are applying ruff formatting to notebooks. We'll do this in that in Check pre-commit ruff formatting for notebooks is set up correctly #340 and apply before the next release.
  • We're currently getting negative sales in the synthetic dataset. So need to check the synthetic data generation code and potentially consider a weighted sum model operating on log outcome data.
  • Fix the legends overlapping with plot content. We'll deal with that in Fix legend overlapping with lines on causal impact plots #341 and run it on this notebook (and others) before the next release.
  • Add a section which compares the results from the pooled and unpooled approaches. The similarity or difference will be dependent on the nature of the synthetic data of course - are we simulating with identical causal impacts in all test geos, or heterogeneous causal impacts in the test geos.
  • Think about using fixed effects approaches https://matheusfacure.github.io/python-causality-handbook/14-Panel-Data-and-Fixed-Effects.html. And/or thinking about "What if you de-mean the data (in time and in unit) as in the book and apply the synthetic control model instead of the classic linear regression" This is a great suggestion, but I think we are holding off and waiting until @juanitorduz investigates. We can certainly update this notebook in the future.

@drbenvincent drbenvincent added documentation Improvements or additions to documentation enhancement New feature or request labels May 7, 2024
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@drbenvincent drbenvincent marked this pull request as draft May 7, 2024 17:19
Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.60%. Comparing base (9330a9c) to head (f89c53b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   83.10%   85.60%   +2.49%     
==========================================
  Files          21       22       +1     
  Lines        1687     1716      +29     
==========================================
+ Hits         1402     1469      +67     
+ Misses        285      247      -38     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@drbenvincent drbenvincent changed the title [WIP] Add example to demonstrate analysis if multi-cell geo lift tests [WIP] Add example analysis of multiple geo lift test analysis May 7, 2024
Copy link

review-notebook-app bot commented May 8, 2024

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-08T12:57:25Z
----------------------------------------------------------------

nip: use " or ' for strings (not both) ... btw: do we have ruff for notebooks? :)


drbenvincent commented on 2024-05-08T19:57:06Z
----------------------------------------------------------------

Fixed. Created #340 to double check ruff for notebooks.

Copy link

review-notebook-app bot commented May 8, 2024

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-08T12:57:26Z
----------------------------------------------------------------

shall we use tab20 palette so that we do not reepear colors https://matplotlib.org/stable/users/explain/colors/colormaps.html?

I see (from the plot) negative sales? Is this expected?


drbenvincent commented on 2024-05-08T20:03:07Z
----------------------------------------------------------------

Applied the tab20 colormap.

Good spot with the negative sales. This is not expected. I'll add this to the todo list - I'll go back to the synthetic data generation and ponder whether we need to operate on log sales etc.

Copy link

review-notebook-app bot commented May 8, 2024

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-08T12:57:27Z
----------------------------------------------------------------

Canw e take the legend outside the plots? they are hard to read


drbenvincent commented on 2024-05-08T20:06:20Z
----------------------------------------------------------------

Good point. I'll actually create an issue about this because this is an important plot type in CausalPy and we want a decent solution which will be useful in many contexts.

Copy link

review-notebook-app bot commented May 8, 2024

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-05-08T12:57:28Z
----------------------------------------------------------------

i think is worth adding a section where we compare the results of the two methods.


drbenvincent commented on 2024-05-08T20:12:51Z
----------------------------------------------------------------

Agreed. Will add this to the todo list and update very soon

@juanitorduz
Copy link
Collaborator

@drbenvincent this is very interesting!

In practice, I have used a fixed effect model as described in https://matheusfacure.github.io/python-causality-handbook/14-Panel-Data-and-Fixed-Effects.html. In this case, we can use all the info from both groups without aggregating. It would be super interesting to compare the results of these approaches (I think the fixed effects model is very popular)

@juanitorduz
Copy link
Collaborator

Actually! What if you de-mean the data (in time and in unit) as in the book and apply the synthetic control model instead of the classic linear regression💡

This could be the better way 🤔

Copy link
Collaborator Author

Fixed. Created #340 to double check ruff for notebooks.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Applied the tab20 colormap.

Good spot with the negative sales. This is not expected. I'll add this to the todo list - I'll go back to the synthetic data generation and ponder whether we need to operate on log sales etc.


View entire conversation on ReviewNB

Copy link
Collaborator Author

Agreed. Will add this to the todo list and update very soon


View entire conversation on ReviewNB

@drbenvincent
Copy link
Collaborator Author

Actually! What if you de-mean the data (in time and in unit) as in the book and apply the synthetic control model instead of the classic linear regression💡

This could be the better way 🤔

So this is interesting @juanitorduz. But I think it will have implications in terms of interpolation/extrapolation and kind of change the nature of the model we are using - in that now we might need to use a ZeroSumNormal prior for the weights, rather than a Dirichlet?

In the situation where we have some large geos (with many sales) and some small geos (few sales), this scaling would also be scaling up/down the observation noise. I can't quite think through the implications of this at the moment, but is there a reason why this isn't done in situations where extrapolation seems to be required? Eg when a target geo is outside the convex hull.

@juanitorduz
Copy link
Collaborator

Maybe we can leave this out from this PR and I can try to test it myself? 😄

@drbenvincent drbenvincent self-assigned this May 8, 2024
@drbenvincent
Copy link
Collaborator Author

Maybe we can leave this out from this PR and I can try to test it myself? 😄

Sounds good. We can update the example at a later date with more content.

Also saw this...

Kim, S., Lee, C., & Gupta, S. (2020). Bayesian Synthetic Control Methods. Journal of Marketing Research, 57(5), 831-852. https://doi.org/10.1177/0022243720936230

@drbenvincent
Copy link
Collaborator Author

@juanitorduz... So I addressed many of the comments. Today I fixed the negative sales (whoops) and added a section comparing the approaches. The comments I've not yet addressed, I felt were appropriate to bundle up into separate issues (see the PR description up top).

Let me know what you think.

@drbenvincent drbenvincent marked this pull request as ready for review May 17, 2024 15:41
@drbenvincent drbenvincent changed the title [WIP] Add example analysis of multiple geo lift test analysis Add example analysis of multiple geo lift test analysis May 17, 2024
Copy link

review-notebook-app bot commented Jun 19, 2024

View / edit / reply to this conversation on ReviewNB

juanitorduz commented on 2024-06-19T16:42:37Z
----------------------------------------------------------------

The legend is redundate as all of them are gray ;) So maybe use different colors or remove the legend?


drbenvincent commented on 2024-06-19T18:44:55Z
----------------------------------------------------------------

Good point. I've changed the colours

@juanitorduz
Copy link
Collaborator

I left a small comment regarding a plot. We can merge this one an iterate. I think the killer feature would be to add a hierarchical model. This will close the gap between the pooled and unpooled models :D

Copy link
Collaborator Author

Good point. I've changed the colours


View entire conversation on ReviewNB

Copy link
Collaborator Author

I agree. I'll add an issue for that.

@drbenvincent
Copy link
Collaborator Author

Replaced manual multiple forest plots with the nice feature of plot_forest that allows you to compare multiple models. Eg.
Screenshot 2024-06-19 at 20 38 17

@juanitorduz this should be good for approval + merging now?

@drbenvincent drbenvincent merged commit 67181c6 into main Jun 21, 2024
7 checks passed
@drbenvincent drbenvincent deleted the multi-cell-geolift branch June 21, 2024 09:48
@drbenvincent drbenvincent changed the title Add example analysis of multiple geo lift test analysis Add example of multiple geo lift test analysis Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
2 participants