Skip to content

Commit ed4fe33

Browse files
committed
Fix needs_fit logic for model selection with a fixed model
Signed-off-by: Keith Battocchi <[email protected]>
1 parent 479362a commit ed4fe33

File tree

11 files changed

+42
-129
lines changed

11 files changed

+42
-129
lines changed

econml/_ortho_learner.py

Lines changed: 1 addition & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -180,9 +180,6 @@ def train(self, is_selecting, folds, X, y, W=None):
180180
return self
181181
def predict(self, X, y, W=None):
182182
return self._model.predict(X)
183-
@property
184-
def needs_fit(self):
185-
return False
186183
np.random.seed(123)
187184
X = np.random.normal(size=(5000, 3))
188185
y = X[:, 0] + np.random.normal(size=(5000,))
@@ -245,8 +242,7 @@ def needs_fit(self):
245242
# when there is more than one model, nuisances from previous models
246243
# come first as positional arguments
247244
accumulated_args = accumulated_nuisances + args
248-
if model.needs_fit:
249-
model.train(True, fold_vals if folds is None else folds, *accumulated_args, **kwargs)
245+
model.train(True, fold_vals if folds is None else folds, *accumulated_args, **kwargs)
250246

251247
calculate_scores &= hasattr(model, 'score')
252248

@@ -452,9 +448,6 @@ def train(self, is_selecting, folds, Y, T, W=None):
452448
return self
453449
def predict(self, Y, T, W=None):
454450
return Y - self._model_y.predict(W), T - self._model_t.predict(W)
455-
@property
456-
def needs_fit(self):
457-
return False
458451
class ModelFinal:
459452
def __init__(self):
460453
return
@@ -509,9 +502,6 @@ def train(self, is_selecting, folds, Y, T, W=None):
509502
return self
510503
def predict(self, Y, T, W=None):
511504
return Y - self._model_y.predict(W), T - self._model_t.predict_proba(W)[:, 1:]
512-
@property
513-
def needs_fit(self):
514-
return False
515505
class ModelFinal:
516506
def __init__(self):
517507
return

econml/dml/_rlearner.py

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -72,10 +72,6 @@ def predict(self, Y, T, X=None, W=None, Z=None, sample_weight=None, groups=None)
7272
T_res = T - T_pred.reshape(T.shape)
7373
return Y_res, T_res
7474

75-
@property
76-
def needs_fit(self):
77-
return self._model_y.needs_fit or self._model_t.needs_fit
78-
7975

8076
class _ModelFinal:
8177
"""
@@ -234,9 +230,6 @@ def best_model(self):
234230
@property
235231
def best_score(self):
236232
return 0
237-
@property
238-
def needs_fit(self):
239-
return False
240233
class ModelFinal:
241234
def fit(self, X, T, T_res, Y_res, sample_weight=None, freq_weight=None, sample_var=None):
242235
self.model = LinearRegression(fit_intercept=False).fit(X * T_res.reshape(-1, 1),

econml/dml/dml.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,10 +104,6 @@ def best_model(self):
104104
def best_score(self):
105105
return self._model.best_score
106106

107-
@property
108-
def needs_fit(self):
109-
return self._model.needs_fit
110-
111107

112108
def _make_first_stage_selector(model, is_discrete, random_state):
113109
if model == 'auto':

econml/dr/_drlearner.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,10 +126,6 @@ def predict(self, Y, T, X=None, W=None, *, sample_weight=None, groups=None):
126126
propensities_weight = np.sum(propensities * T_complete, axis=1)
127127
return Y_pred.reshape(Y.shape + (T.shape[1] + 1,)), propensities_weight.reshape((n,))
128128

129-
@property
130-
def needs_fit(self):
131-
return self._model_propensity.needs_fit or self._model_regression.needs_fit
132-
133129

134130
def _make_first_stage_selector(model, is_discrete, random_state):
135131
if model == "auto":

econml/iv/dml/_dml.py

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -119,12 +119,6 @@ def predict(self, Y, T, X=None, W=None, Z=None, sample_weight=None, groups=None)
119119
Z_res = Z - Z_pred.reshape(Z.shape)
120120
return Y_res, T_res, Z_res
121121

122-
@property
123-
def needs_fit(self):
124-
return (self._model_y_xw.needs_fit or self._model_t_xw.needs_fit or
125-
(self._projection and self._model_t_xwz.needs_fit) or
126-
(not self._projection and self._model_z_xw.needs_fit))
127-
128122

129123
class _OrthoIVModelFinal:
130124
def __init__(self, model_final, featurizer, fit_cate_intercept):
@@ -773,10 +767,6 @@ def predict(self, Y, T, X=None, W=None, Z=None, sample_weight=None, groups=None)
773767
T_res = TXZ_pred.reshape(T.shape) - TX_pred.reshape(T.shape)
774768
return Y_res, T_res
775769

776-
@property
777-
def needs_fit(self):
778-
return self._model_y_xw.needs_fit or self._model_t_xw.needs_fit or self._model_t_xwz.needs_fit
779-
780770

781771
class _BaseDMLIVModelFinal(_ModelFinal):
782772
"""

econml/iv/dr/_dr.py

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -163,12 +163,6 @@ def predict(self, Y, T, X=None, W=None, Z=None, sample_weight=None, groups=None)
163163

164164
return prel_theta, Y_res, T_res, Z_res
165165

166-
@property
167-
def needs_fit(self):
168-
return (self._model_y_xw.needs_fit or self._model_t_xw.needs_fit or
169-
(self._projection and self._model_t_xwz.needs_fit) or
170-
(not self._projection and self._model_z_xw.needs_fit))
171-
172166

173167
class _BaseDRIVNuisanceCovarianceSelector(ModelSelector):
174168
def __init__(self, *, model_tz_xw,
@@ -275,10 +269,6 @@ def predict(self, prel_theta, Y_res, T_res, Z_res, Y, T, X=None, W=None, Z=None,
275269

276270
return (cov,)
277271

278-
@property
279-
def needs_fit(self):
280-
return self._model_tz_xw.needs_fit
281-
282272

283273
class _BaseDRIVModelFinal:
284274
def __init__(self, model_final, featurizer, fit_cate_intercept, cov_clip, opt_reweighted):
@@ -2464,10 +2454,6 @@ def predict(self, Y, T, X=None, W=None, Z=None, sample_weight=None, groups=None)
24642454

24652455
return prel_theta, Y_res, T_res, Z_res, beta
24662456

2467-
@property
2468-
def needs_fit(self):
2469-
return self._model_y_xw.needs_fit or self._model_t_xwz.needs_fit or self._dummy_z.needs_fit
2470-
24712457

24722458
class _DummyClassifier:
24732459
"""

econml/panel/dml/_dml.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -171,10 +171,6 @@ def _get_shape_formatter(self, X, W):
171171
def _index_or_None(self, X, filter_idx):
172172
return None if X is None else X[filter_idx]
173173

174-
@property
175-
def needs_fit(self):
176-
return self._model_t.needs_fit or self._model_y.needs_fit
177-
178174

179175
class _DynamicModelFinal:
180176
"""

econml/sklearn_extensions/model_selection.py

Lines changed: 25 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -305,15 +305,6 @@ def score(self, *args, **kwargs):
305305
"""
306306
raise NotImplementedError("Abstract method")
307307

308-
@property
309-
@abc.abstractmethod
310-
def needs_fit(self):
311-
"""
312-
Whether the model selector needs to be fit before it can be used for prediction or scoring;
313-
in many cases this is equivalent to whether the selector is choosing between multiple models
314-
"""
315-
raise NotImplementedError("Abstract method")
316-
317308

318309
class SingleModelSelector(ModelSelector):
319310
"""
@@ -392,24 +383,25 @@ class FixedModelSelector(SingleModelSelector):
392383
Model selection class that always selects the given sklearn-compatible model
393384
"""
394385

395-
def __init__(self, model):
386+
def __init__(self, model, score_during_selection):
396387
self.model = clone(model, safe=False)
388+
self.score_during_selection = score_during_selection
397389

398390
def train(self, is_selecting, folds: Optional[List], X, y, groups=None, **kwargs):
399391
if is_selecting:
400-
# since needs_fit is False, is_selecting will only be true if
401-
# the score needs to be compared to another model's
402-
# so we don't need to fit the model itself, just get the out-of-sample score
403-
assert hasattr(self.model, 'score'), (f"Can't select between a fixed {type(self.model)} model and others "
404-
"because it doesn't have a score method")
405-
scores = []
406-
for train, test in folds:
407-
# use _fit_with_groups instead of just fit to handle nested grouping
408-
_fit_with_groups(self.model, X[train], y[train],
409-
groups=None if groups is None else groups[train],
410-
**{key: val[train] for key, val in kwargs.items()})
411-
scores.append(self.model.score(X[test], y[test]))
412-
self._score = np.mean(scores)
392+
if self.score_during_selection:
393+
# the score needs to be compared to another model's
394+
# so we don't need to fit the model itself on all of the data, just get the out-of-sample score
395+
assert hasattr(self.model, 'score'), (f"Can't select between a fixed {type(self.model)} model "
396+
"and others because it doesn't have a score method")
397+
scores = []
398+
for train, test in folds:
399+
# use _fit_with_groups instead of just fit to handle nested grouping
400+
_fit_with_groups(self.model, X[train], y[train],
401+
groups=None if groups is None else groups[train],
402+
**{key: val[train] for key, val in kwargs.items()})
403+
scores.append(self.model.score(X[test], y[test]))
404+
self._score = np.mean(scores)
413405
else:
414406
# we need to train the model on the data
415407
_fit_with_groups(self.model, X, y, groups=groups, **kwargs)
@@ -422,11 +414,10 @@ def best_model(self):
422414

423415
@property
424416
def best_score(self):
425-
return self._score
426-
427-
@property
428-
def needs_fit(self):
429-
return False # We have only a single model so we can skip the selection process
417+
if hasattr(self, '_score'):
418+
return self._score
419+
else:
420+
raise ValueError("No score was computed during selection")
430421

431422

432423
def _copy_to(m1, m2, attrs, insert_underscore=False):
@@ -579,11 +570,6 @@ def best_model(self):
579570
def best_score(self):
580571
return self._best_score
581572

582-
@property
583-
def needs_fit(self):
584-
return True # strictly speaking, could be false if the hyperparameters are fixed
585-
# but it would be complicated to check that
586-
587573

588574
class ListSelector(SingleModelSelector):
589575
"""
@@ -627,14 +613,8 @@ def best_model(self):
627613
def best_score(self):
628614
return self._best_score
629615

630-
@property
631-
def needs_fit(self):
632-
# technically, if there is just one model and it doesn't need to be fit we don't need to fit it,
633-
# but that complicates the training logic so we don't bother with that case
634-
return True
635-
636616

637-
def get_selector(input, is_discrete, *, random_state=None, cv=None, wrapper=GridSearchCV):
617+
def get_selector(input, is_discrete, *, random_state=None, cv=None, wrapper=GridSearchCV, needs_scoring=False):
638618
named_models = {
639619
'linear': (LogisticRegressionCV(random_state=random_state, cv=cv) if is_discrete
640620
else WeightedLassoCVWrapper(random_state=random_state, cv=cv)),
@@ -657,19 +637,21 @@ def get_selector(input, is_discrete, *, random_state=None, cv=None, wrapper=Grid
657637
return input
658638
elif isinstance(input, list): # we've got a list; call get_selector on each element, then wrap in a ListSelector
659639
models = [get_selector(model, is_discrete,
660-
random_state=random_state, cv=cv, wrapper=wrapper)
640+
random_state=random_state, cv=cv, wrapper=wrapper,
641+
needs_scoring=True) # we need to score to compare outputs to each other
661642
for model in input]
662643
return ListSelector(models)
663644
elif isinstance(input, str): # we've got a string; look it up
664645
if input in named_models:
665646
return get_selector(named_models[input], is_discrete,
666-
random_state=random_state, cv=cv, wrapper=wrapper)
647+
random_state=random_state, cv=cv, wrapper=wrapper,
648+
needs_scoring=needs_scoring)
667649
else:
668650
raise ValueError(f"Unknown model type: {input}, must be one of {named_models.keys()}")
669651
elif SklearnCVSelector.can_wrap(input):
670652
return SklearnCVSelector(input)
671653
else: # assume this is an sklearn-compatible model
672-
return FixedModelSelector(input)
654+
return FixedModelSelector(input, needs_scoring)
673655

674656

675657
class GridSearchCVList(BaseEstimator):

econml/tests/test_missing_values.py

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,6 @@ def train(self, is_selecting, folds, Y, T, W=None):
3535
def predict(self, Y, T, W=None):
3636
return Y - self._model_y.predict(W), T - self._model_t.predict(W)
3737

38-
@property
39-
def needs_fit(self):
40-
return False
41-
4238

4339
class ModelFinal:
4440

econml/tests/test_model_selection.py

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,8 @@
1212
from sklearn.preprocessing import PolynomialFeatures
1313
from econml.dml import LinearDML
1414
from econml.sklearn_extensions.linear_model import WeightedLassoCVWrapper
15+
from econml.utilities import SeparateModel
16+
from econml.dr import LinearDRLearner
1517

1618

1719
class TestModelSelection(unittest.TestCase):
@@ -133,3 +135,17 @@ def test_sklearn_model_selection(self):
133135
discrete_treatment=is_discrete,
134136
model_y=LinearRegression())
135137
est.fit(Y, T2 if use_array else T, X=X, W=W)
138+
139+
def test_fixed_model_scoring(self):
140+
Y, T, X, W = self._simple_dgp(500, 2, 3, True)
141+
142+
# SeparatedModel doesn't support scoring; that should be fine when not compared to other models
143+
mdl = LinearDRLearner(model_regression=SeparateModel(LassoCV(), LassoCV()),
144+
model_propensity=LogisticRegressionCV())
145+
mdl.fit(Y, T, X=X, W=W)
146+
147+
# on the other hand, when we need to compare the score to other models, it should raise an error
148+
with self.assertRaises(Exception):
149+
mdl = LinearDRLearner(model_regression=[SeparateModel(LassoCV(), LassoCV()), Lasso()],
150+
model_propensity=LogisticRegressionCV())
151+
mdl.fit(Y, T, X=X, W=W)

0 commit comments

Comments
 (0)