[MRG+1] RFE can raise NotFittedError#9283
Conversation
|
need to squash commits |
no need, GitHub will squash when merging |
TomDLT
left a comment
There was a problem hiding this comment.
RFE doesnt raise NotFittedError, suspect other meta feature selectors have same issue.
I checked, the other classes inheriting SelectorMixin all have already a specific check_is_fitted call.
LGTM
|
There should really be a non-regression test. |
sklearn/feature_selection/rfe.py
Outdated
| return self.estimator_.score(self.transform(X), y) | ||
|
|
||
| def _get_support_mask(self): | ||
| check_is_fitted(self, 'support_') # will raise NotFittedError if not fitted |
There was a problem hiding this comment.
This line is too long (>79 characters), you may remove the comment
jnothman
left a comment
There was a problem hiding this comment.
This looks good!
Though now I wonder if it would be nice for check_is_fitted to return the value of that attribute... Hmm. Not required in this PR.
@TomDLT, this has changed a lot since you gave it +1. Could you please check again?
sklearn/tests/test_metaestimators.py
Outdated
| self._check_fit() | ||
| return 1.0 | ||
|
|
||
|
|
There was a problem hiding this comment.
These two blank lines should not have been added
|
is there an exhaustive list of estimators somewhere, so I can find more that don't raise In terms of a longer-term solution, maybe |
|
This kind of thing is tested by way of
|
|
LGTM 👍 : merging now, more improvements can come later. |
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
* RFE can raise NotFittedError * boom boom * dont change tests * tests pass * remove two extra lines
Reference Issue
Fixes Issue #9276
What does this implement/fix? Explain your changes.
Any other comments?