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

Fixed inference results when cate_feature_names is not defined #225

Merged
merged 2 commits into from
Feb 25, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions econml/inference.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,7 @@ def coef__inference(self):
if coef.size == 0: # X is None
raise AttributeError("X is None, please call intercept_inference to learn the constant!")

if callable(self._est.cate_feature_names):
if hasattr(self._est, 'cate_feature_names') and callable(self._est.cate_feature_names):
def fname_transformer(x):
return self._est.cate_feature_names(x)
else:
Expand Down Expand Up @@ -426,7 +426,7 @@ def coef__inference(self, T):
coef_stderr = self.fitted_models_final[ind].coef_stderr_
if coef.size == 0: # X is None
raise AttributeError("X is None, please call intercept_inference to learn the constant!")
if callable(self._est.cate_feature_names):
if hasattr(self._est, 'cate_feature_names') and callable(self._est.cate_feature_names):
def fname_transformer(x):
return self._est.cate_feature_names(x)
else:
Expand Down Expand Up @@ -692,7 +692,7 @@ def summary_frame(self, alpha=0.1, value=0, decimals=3, feat_name=None):
if self.d_y == 1:
res.index = res.index.droplevel(1)
if self.inf_type == 'coefficient':
if feat_name and self.fname_transformer:
if feat_name is not None and self.fname_transformer:
ind = self.fname_transformer(feat_name)
else:
ct = res.shape[0] // self.d_y
Expand Down
53 changes: 53 additions & 0 deletions econml/tests/test_inference.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
# Copyright (c) Microsoft Corporation. All rights reserved.
# Licensed under the MIT License.

import numpy as np
import unittest
from sklearn.base import clone
from sklearn.preprocessing import PolynomialFeatures
from econml.dml import LinearDMLCateEstimator


class TestInference(unittest.TestCase):

@classmethod
def setUpClass(cls):
np.random.seed(123)
# DGP constants
cls.n = 1000
cls.d_w = 30
cls.d_x = 5
# Generate data
cls.X = np.random.uniform(0, 1, size=(cls.n, cls.d_x))
cls.W = np.random.normal(0, 1, size=(cls.n, cls.d_w))
cls.T = np.random.normal(0, 1, size=(cls.n, ))
cls.Y = np.random.normal(0, 1, size=(cls.n, ))

def test_inference_results(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd rename to something like test_inference_no_feature_names or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it this way so we can add more tests under the same method in the future. And this is just one subtest.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay; I generally prefer to have a larger number of fine grained tests than a smaller number of broader tests where possible because it makes it quicker to track down the nature of the error when there's a failure, but I don't feel super strongly about it.

"""Tests the inference results summary."""
# Test inference results when `cate_feature_names` doesn not exist
cate_est = LinearDMLCateEstimator(
featurizer=PolynomialFeatures(degree=1,
include_bias=False)
)
wrapped_est = self._NoFeatNamesEst(cate_est)
wrapped_est.fit(
TestInference.Y,
TestInference.T,
TestInference.X,
TestInference.W,
inference='statsmodels'
)
summary_results = wrapped_est.summary()
coef_rows = np.asarray(summary_results.tables[0].data)[1:, 0]
np.testing.assert_array_equal(coef_rows, ['X{}'.format(i) for i in range(TestInference.d_x)])

class _NoFeatNamesEst:
def __init__(self, cate_est):
self.cate_est = clone(cate_est, safe=False)

def __getattr__(self, name):
if name != 'cate_feature_names':
return getattr(self.cate_est, name)
else:
return self.__getattribute__(name)