Open vasilismsr opened 5 years ago
I looked into this. It seems a little complicated. Here are some thoughts, in case they're useful.
The short version: for some attributes you could inherit or imitate Sklearn's methods with some slight changes, but for other attributes I think it will take more work.
The long version:
Sklearn implements these methods and the helper _get_param_names
in its BaseEstimator
so that all of its estimators inherit the methods. For get_params
, parameter names are parsed out of the signature of the class's __init__
, and the values are assumed to be object attributes with the same name.
You could try to use BaseEstimator
as a super class for BaseCateEstimator
If you do that, you also get a nice repr
output. For example, one of the DML examples changes from
<econml.dml.LinearDMLCateEstimator at 0x7f85c8d86450>
to
LinearDMLCateEstimator(discrete_treatment=None,
featurizer=PolynomialFeatures(degree=1,
include_bias=True,
interaction_only=False,
order='C'),
linear_first_stages=None, model_t=None, model_y=None,
n_splits=None, random_state=None)
However, it doesn't quite work with get_params
(and in fact, the repr above didn't quite work). For the same DML example, calling est.get_params()
results in
{'discrete_treatment': None,
'featurizer__degree': 1,
'featurizer__include_bias': True,
'featurizer__interaction_only': False,
'featurizer__order': 'C',
'featurizer': PolynomialFeatures(degree=1, include_bias=True, interaction_only=False,
order='C'),
'linear_first_stages': None,
'model_t': None,
'model_y': None,
'n_splits': None,
'random_state': None}
It recurses, to give the parameters of the featurizer
, which is nice, but it incorrectly sets some attributes to None
. It doesn't find model_t
and model_y
, for example, because they're stored in _model_t
and _model_y
. Reconciling that is a quick fix, but there are other issues.
In DMLCateEstimator
, the internal models are wrapped in FirstStageWrapper
and FinalWrapper
classes, so getting set_params
to update model_y
correctly will probably take extra work. Whatever the solution is, it's likely that classes will need individual solutions. Also note that in this example Sklearn's get_params
would return something the user doesn't recognize for model_y
, model_t
, and model_final
.
I agree with all these points. It seems a hard task to do. Especially the wrapper point seems a daunting one to resolve. Not sure it’s worth given how many changes it will need. Basically it seems that if we wanted this it should have gone into the design much earlier so that many parts of the code would have been written in a more appropriate manner. I would indeed put this issue on hold for now.
We should implement get_params and set_params across all our cate estimators so that they can be used many times interchangeably with base estimators form sklearn. This will also help prettier copying and printing of the estimators.