TST check error consistency when calling get_feature_names_out on unfitted estimator #25223
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s called before fit
|
I think this is a good start. I would now create a whitelist such that we can correct one by one (or more than one if it comes with inheritance) the failing estimators. The patch to do so would be: diff --git a/sklearn/tests/test_common.py b/sklearn/tests/test_common.py
index b61e2e2a93..d9352f97fe 100644
--- a/sklearn/tests/test_common.py
+++ b/sklearn/tests/test_common.py
@@ -461,16 +461,60 @@ def test_transformers_get_feature_names_out(transformer):
ESTIMATORS_WITH_GET_FEATURE_NAMES_OUT = [
est for est in _tested_estimators() if hasattr(est, "get_feature_names_out")
]
+WHITELISTED_FAILING_ESTIMATORS = [
+ "AdditiveChi2Sampler",
+ "Binarizer",
+ "DictVectorizer",
+ "GaussianRandomProjection",
+ "GenericUnivariateSelect",
+ "IterativeImputer",
+ "IsotonicRegression",
+ "KBinsDiscretizer",
+ "KNNImputer",
+ "MaxAbsScaler",
+ "MinMaxScaler",
+ "MissingIndicator",
+ "Normalizer",
+ "OrdinalEncoder",
+ "PowerTransformer",
+ "QuantileTransformer",
+ "RFE",
+ "RFECV",
+ "RobustScaler",
+ "SelectFdr",
+ "SelectFpr",
+ "SelectFromModel",
+ "SelectFwe",
+ "SelectKBest",
+ "SelectPercentile",
+ "SequentialFeatureSelector",
+ "SimpleImputer",
+ "SparseRandomProjection",
+ "SplineTransformer",
+ "StackingClassifier",
+ "StackingRegressor",
+ "StandardScaler",
+ "TfidfTransformer",
+ "VarianceThreshold",
+ "VotingClassifier",
+ "VotingRegressor",
+]
@pytest.mark.parametrize(
"estimator", ESTIMATORS_WITH_GET_FEATURE_NAMES_OUT, ids=_get_check_estimator_ids
)
def test_estimators_get_feature_names_out_error(estimator):
+ estimator_name = estimator.__class__.__name__
+ if estimator_name in WHITELISTED_FAILING_ESTIMATORS:
+ return pytest.xfail(
+ reason=f"{estimator_name} is not failing with a consistent NotFittedError"
+ )
+
_set_checking_parameters(estimator)
with ignore_warnings(category=(FutureWarning)):
- check_get_feature_names_out_error(estimator.__class__.__name__, estimator)
+ check_get_feature_names_out_error(estimator_name, estimator)
@pytest.mark.parametrize( |
|
@glemaitre , Just included the patch in the most recent commit. |
|
Thanks @glemaitre , should we wait for the PR to be merged or others can begin working on the failing estimators? |
|
You can always start to work by branching from this branch. You will need to merge |
|
Thanks @jeremiedbb to be so quick ;) |
|
Thanks @glemaitre and @jeremiedbb for approving this quickly. I will now mention the estimators that need to be worked on in issue #24916 so that other contributors can join in. |
36 tasks
jjerphan
pushed a commit
to jjerphan/scikit-learn
that referenced
this pull request
Jan 3, 2023
jjerphan
pushed a commit
to jjerphan/scikit-learn
that referenced
this pull request
Jan 20, 2023
jjerphan
pushed a commit
to jjerphan/scikit-learn
that referenced
this pull request
Jan 20, 2023
jjerphan
pushed a commit
to jjerphan/scikit-learn
that referenced
this pull request
Jan 23, 2023
adrinjalali
pushed a commit
that referenced
this pull request
Jan 24, 2023
…itted estimator (#25223)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issues/PRs
In issue #24916 , we want to make error messages uniform when calling
get_feature_names_outbeforefit. To adhere to the uniformity, it was agreed that all estimators should raise aNotFittedErrorif they are unfitted.What does this implement/fix? Explain your changes.
To solve the issue, we first needed to identify the estimators that don't raise a
NotFittedError. Therefore, this PR proposes tests that check if aNotFittedErroris raised in estimators withget_feature_names_out.Any other comments?
For a particular estimator, the test will pass if a
NotFittedErroris raised byget_feature_names_outand will fail if any other type of error/exception is raised.In case the test fails, it will show the estimator, the error and which parts of the code led to the error being raised.
The command below can be used to run the tests that check errors generated when
get_feature_names_outis called beforefitin all estimators:pytest -vsl sklearn/tests/test_common.py -k estimators_get_feature_names_out_error