Fix deprecated args and dowhy wrapper #434
Conversation
heimengqi
commented
Mar 18, 2021
- Fix the deprecated args in ForestDRLearner, this will fix the issue ForestDRLearner DoWhy Fit not working #432
- Ignore the "deprecated" args when get the parameters for cate estimator under dowhy wrapper
- Fix the feature name bug, should save the feature names from input data frame before transferring it to numpy arrays.
…n search params from dowhy wrapper
econml/dr/_drlearner.py
Outdated
| @@ -1523,7 +1523,7 @@ def n_crossfit_splits(self, value): | |||
|
|
|||
| @property | |||
| def criterion(self): | |||
| return self.criterion | |||
| return "mse" | |||
There was a problem hiding this comment.
Seems a bit weird that we allow est.criterion = 'deprecated' but not est.criterion = 'mse', and yet we return "mse" here...
If this is identical to sklearn's approach then I guess we can stick with it, but seems unintuitive to me.
There was a problem hiding this comment.
Not exactly the same approach, they have a decorator for deprecated args and the deprecated args are not defined in init. Or can I just remove these deprecated args? since it should be removed for next release anyway?
There was a problem hiding this comment.
@kbattocchi @vsyrgkanis Any final conclusion on this? Then I could make it ready to merge.
There was a problem hiding this comment.
How hard would it be to mimic sklearn's approach? I think it would be best to either do that or remove the deprecated args as you suggested.
There was a problem hiding this comment.
I removed all the deprecated args, also for other classes, especially clean up the n_splits/n_crossfit_splits. It's not so clear to me how to mirror sklearn, they seem just add a warning, might worth create an issue to explore how to handle that systematically.
econml/dr/_drlearner.py
Outdated
| @@ -1533,7 +1533,7 @@ def criterion(self, value): | |||
|
|
|||
| @property | |||
| def max_leaf_nodes(self): | |||
| return self.max_leaf_nodes | |||
| return None | |||
There was a problem hiding this comment.
Parallel comment here.
25c8337 to
5b069f0
Compare