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

Fix bootstrap inference #299

Merged
merged 2 commits into from
Nov 6, 2020
Merged

Fix bootstrap inference #299

merged 2 commits into from
Nov 6, 2020

Conversation

kbattocchi
Copy link
Collaborator

No description provided.

@kbattocchi kbattocchi force-pushed the kebatt/bootstrapFixes branch 5 times, most recently from 9c4965c to d27b441 Compare November 4, 2020 06:58
@kbattocchi kbattocchi marked this pull request as ready for review November 4, 2020 07:02
@kbattocchi kbattocchi force-pushed the kebatt/bootstrapFixes branch 2 times, most recently from ef264f3 to 9a153bc Compare November 4, 2020 08:26
econml/bootstrap.py Outdated Show resolved Hide resolved
econml/bootstrap.py Outdated Show resolved Hide resolved
econml/bootstrap.py Outdated Show resolved Hide resolved
if prefix in ['const_marginal_effect', 'marginal_effect', 'effect']:
inf_type = 'effect'
elif prefix == 'coef_':
inf_type = 'coefficient'
if (hasattr(self._instances[0], 'cate_feature_names') and
Copy link
Collaborator

Choose a reason for hiding this comment

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

Currently we have no test that calls coef__inference or summary() but such that the estimator has no cate_feature_names method!

We either need to add such a test and make sure the behavior is as we want:

  1. I see this here being problematic because we would ideally want when I call summary(feat_name=..), even if the method has no cate_feature_names method, that the feat_name list that I give, will appear in the summary table.

Or maybe even better: we make cate_feature_names a mandatory method for any estimator that inherits from LinearModelFinalInference class and the LinearModelFinalInferenceDiscrete class. In this case, summary should perform ok as we will always have cate_feature_names implemented, whenever summary is available.

if (hasattr(self._instances[0], 'cate_feature_names') and
callable(self._instances[0].cate_feature_names)):
def fname_transformer(x):
return self._instances[0].cate_feature_names(x)
elif prefix == 'intercept_':
inf_type = 'intercept'
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should add a test that checks that the attribute error is raised in this loop and nothing else happens.

econml/bootstrap.py Show resolved Hide resolved
econml/bootstrap.py Show resolved Hide resolved
econml/cate_estimator.py Show resolved Hide resolved
econml/inference.py Show resolved Hide resolved
econml/inference.py Outdated Show resolved Hide resolved
@vsyrgkanis
Copy link
Collaborator

vsyrgkanis commented Nov 4, 2020

Here T takes three values {0, 1, 2}

est2 = LinearDRLearner(model_regression=RandomForestRegressor(n_estimators=10),
                       model_propensity=RandomForestClassifier(n_estimators=10, min_samples_leaf=50),
                 featurizer=None, min_propensity=1e-3,
                 n_splits=6)
est2.fit(Y, T, X, W, inference=BootstrapInference(n_bootstrap_samples=8))
est2.coef__inference(T=1).summary_frame()

Should be returning the inference only for the coefficient associated with T=1. Currently it returns inference for all models.

Moreover, est2.summary(T=1) raises an error. Weird since, I think we do have a test for summary with drlearner and bootstrap.

@kbattocchi kbattocchi force-pushed the kebatt/bootstrapFixes branch 5 times, most recently from 54611ac to b8d5030 Compare November 6, 2020 12:26
@kbattocchi kbattocchi force-pushed the kebatt/bootstrapFixes branch from b8d5030 to c33dd87 Compare November 6, 2020 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants