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

Basic support for multivalue categorical treatments #750

Conversation

EgorKraevTransferwise
Copy link
Contributor

Support multivalue categorical treatments in econml.py; add a utility method to CausalEstimator that returns the effect of the treatment that was actually used, and works with the multivalue changes

@EgorKraevTransferwise
Copy link
Contributor Author

"Imports are incorrectly sorted and/or formatted." - why and how should I sort/format them? :)

@emrekiciman
Copy link
Member

Hi @EgorKraevTransferwise

There are instructions at https://github.com/py-why/dowhy/blob/main/docs/source/contributing/contributing-code.rst for running the auto-formatters and other checks during PR prep. The key command is:

poetry run poe format

Thanks!

Copy link
Member

@amit-sharma amit-sharma left a comment

Choose a reason for hiding this comment

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

thanks for this functionality @EgorKraevTransferwise .

I did not understand the utility of the effect_tt since it is not used anywhere.
Can you add some tests for this functionality? And perhaps also update the conditional effects notebook to showcase this functionality?

dowhy/causal_estimator.py Outdated Show resolved Hide resolved
dowhy/causal_estimator.py Outdated Show resolved Hide resolved
dowhy/causal_estimator.py Outdated Show resolved Hide resolved
@amit-sharma amit-sharma added the enhancement New feature or request label Nov 10, 2022
@EgorKraevTransferwise
Copy link
Contributor Author

Added a unit test for the multivalue usecase, which also illustrates the use of effect_tt. Moved the latter to econml.py

@EgorKraevTransferwise
Copy link
Contributor Author

EgorKraevTransferwise commented Nov 16, 2022

Very strange fail in example_notebooks/sensitivity_analysis_nonparametric_estimators.ipynb.

Basically, in the code snippet below the model._graph.get_effect_modifiers(model._treatment, model._outcome) returns [] (leading to exceptions in downstream code which expects non-empty list of effect modifiers), which is strange for two reasons: firstly, is that really the desired behavior for that graph? And second, do we really want to call EconML estimators with an empty list of effect modifiers? @amit-sharma

import re
import numpy as np
import dowhy
from dowhy import CausalModel
import dowhy.datasets
from dowhy.utils.regression import create_polynomial_function

np.random.seed(101)
data = dowhy.datasets.partially_linear_dataset(
    beta=10,
    num_common_causes=7,
    num_unobserved_common_causes=1,
    strength_unobserved_confounding=10,
    num_samples=1000,
    num_treatments=1,
    stddev_treatment_noise=10,
    stddev_outcome_noise=5,
)

# Observed data
dropped_cols = ["W0"]
user_data = data["df"].drop(dropped_cols, axis=1)
# assumed graph
user_graph = data["gml_graph"]
for col in dropped_cols:
    user_graph = user_graph.replace('node[ id "{0}" label "{0}"]'.format(col), "")
    user_graph = re.sub(
        'edge\[ source "{}" target "[vy][0]*"\]'.format(col), "", user_graph
    )

model = CausalModel(
    data=user_data,
    treatment=data["treatment_name"],
    outcome=data["outcome_name"],
    graph=user_graph,
    test_significance=None,
)

model._graph.get_effect_modifiers(model._treatment, model._outcome)

@amit-sharma
Copy link
Member

Very strange fail in example_notebooks/sensitivity_analysis_nonparametric_estimators.ipynb.

Basically, in the code snippet below the model._graph.get_effect_modifiers(model._treatment, model._outcome) returns [] (leading to exceptions in downstream code which expects non-empty list of effect modifiers), which is strange for two reasons: firstly, is that really the desired behavior for that graph? And second, do we really want to call EconML estimators with an empty list of effect modifiers? @amit-sharma

Sometimes, one may simply want to compute the ATE using DML since it offers ML-based first stage. We should support such calls where X=None.
The way to resolve your issue will be to use X_test variable instead of df[self._effect_modifier_names]. This is also preferred since X_test includes the filtering by target_units too.

@EgorKraevTransferwise
Copy link
Contributor Author

Ah, got it thanks! Fixed that bit and added a test for it.

@EgorKraevTransferwise
Copy link
Contributor Author

EgorKraevTransferwise commented Nov 18, 2022

This DCO thing is really a hassle, any chance one of the repository owners could hit the override button? :)
I believe I already certified something along those lines when making earlier contributions.

@emrekiciman
Copy link
Member

Hi @EgorKraevTransferwise , yes, until one sets up git to automatically sign, DCO can be a pain. There are instructions on how to fix the DCO for the PR in the details of the failure: https://github.com/py-why/dowhy/pull/750/checks?check_run_id=9571908626

@EgorKraevTransferwise
Copy link
Contributor Author

Hi @EgorKraevTransferwise , yes, until one sets up git to automatically sign, DCO can be a pain. There are instructions on how to fix the DCO for the PR in the details of the failure: https://github.com/py-why/dowhy/pull/750/checks?check_run_id=9571908626

Why didn't you use a CLA instead that each contributor has to sign only once, like FLAML does?

I will jump through the hoops in this case, no problem, but surely this hassle will also discourage other would-be contributors?

@EgorKraevTransferwise
Copy link
Contributor Author

Tried creating a new PR with all the changes (it's just 3 files) and the correct commit message, but DCO still fails for some reason :(

@amit-sharma
Copy link
Member

Closing this PR in favor of #768

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

Successfully merging this pull request may close these issues.

None yet

3 participants