FIX Add common test to check for unfitted behaviour in classifiers and fix RadiusNeighborsClassifier and ClassifierChain correspondingly #27051

OmarManzoor · 2023-08-11T09:53:20Z

Reference Issues/PRs

Relates to #26828

What does this implement/fix? Explain your changes.

Adds a common test to check for unfitted issues in classifiers.
Adds the check_is_fitted check at the beginning of predict_proba in RadiusNeighborsClassifier.
Adds the check_is_fitted check in predict_proba and decision_function in ClassifierChain.

Any other comments?

CC: @jjerphan

github-actions · 2023-08-11T09:55:10Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 2398ba7. Link to the linter CI: here}

jjerphan

Thanks!

We might want to change the title of this PR for including FIX, I think. How about:

FIX Check that `RadiusNeighborsClassifier` is fit in `predict{,_proba}`

@glemaitre: I am ignorant here, but aren there common tests checking that estimators are fit before predict and predict_proba are called? 🙂

jjerphan · 2023-08-11T09:57:25Z

sklearn/neighbors/tests/test_neighbors.py

+    """Check that :class:`RadiusNeighborsClassifier` is fitted before
+    calling the :meth:`predict` and :meth:`predict_proba`.


check_is_fitted also must be called in RadiusNeighborsClassifier.predict (for consistency with RadiusNeighborsRegressor.predict) and test this change accordingly.

I don't think we need to do that here because the first thing that predict calls is predict_proba.

I think that we need at least to test predict on unfit estimator.

glemaitre · 2023-08-11T20:34:26Z

I am ignorant here, but aren there common tests checking that estimators are fit before predict and predict_proba are called?

Indeed we have common tests that check that we raise a NotFittedError if the estimator was not fitted.

jjerphan

LGTM. Thank you, @OmarManzoor.

jjerphan · 2023-08-13T18:09:18Z

sklearn/neighbors/tests/test_neighbors.py

+    """Check that :class:`RadiusNeighborsClassifier` is fitted before
+    calling the :meth:`predict` and :meth:`predict_proba`.


I think that we need at least to test predict on unfit estimator.

OmarManzoor · 2023-08-13T18:32:28Z

@jjerphan I wonder if we should remove the test considering the common test and maybe add this classifier there if it is missing.

jjerphan · 2023-08-13T18:35:09Z

If this is doable, it's preferable, yes.

* Add a common test to check for unfitted classifiers * Fix ClassifierChain by adding check_is_fitted inside predict_proba and decision_function methods

OmarManzoor · 2023-08-15T08:13:53Z

@jjerphan Could you kindly have a look?

jjerphan

LGTM.

I would wait other maintainers with more authority on common tests to complement my comment and to approve this PR.

jjerphan · 2023-08-15T08:32:41Z

sklearn/tests/test_common.py

+@pytest.mark.parametrize(
+    "classifier",
+    _tested_estimators(type_filter="classifier"),
+    ids=_get_check_estimator_ids,
+)
+def test_classifiers_unfitted(classifier):
+    classifier_name = classifier.__class__.__name__
+    _set_checking_parameters(classifier)
+    check_estimators_unfitted(classifier_name, classifier)


I am surprised that a test like this one is not already existing.

jjerphan · 2023-08-15T08:33:27Z

sklearn/multioutput.py

Changes made to this file enlarge the scope of this PR.

Could you change the title of this PR accordingly?

Done! Feel free to modify if it looks too long.

glemaitre · 2023-08-15T09:42:09Z

This is already the job of the check_fit_check_is_fitted check that is used in _yield_all_checks.

What should be checked instead is the reason why we would skip this test for the model here.
Usually, one possibility is that we face a meta-estimator that requires parameter and we did not modify the code to construct the instance.

glemaitre · 2023-08-15T09:48:31Z

OK so the reason here is that ClassifierChain has a tag "_skip_test": True meaning we will not run most of the test. We should probably revisit what is the reason for not running the checks.

OmarManzoor · 2023-08-15T10:11:51Z

OK so the reason here is that ClassifierChain has a tag "_skip_test": True meaning we will not run most of the test. We should probably revisit what is the reason for not running the checks.

What is the reason that RadiusNeighborsClassifier did not get picked up in this test if it applicable? The main purpose of this PR initially was to check for is fitted in RadiusNeighborsClassifier predict_proba so that it means this was not being checked by the test you mentioned.

OmarManzoor · 2023-08-15T10:44:44Z

Thanks @glemaitre! I explored a bit and found that the check check_estimators_unfitted is already being applied on RadiusNeighborsClassifier. The reason it passes is that we are already checking for check_is_fitted inside the method radius_neighbors.

scikit-learn/sklearn/neighbors/_base.py

Lines 1140 to 1145 in ae01cec

    
                   indices.  In general, multiple points can be queried at the same time. 
        
                   """ 
        
                   check_is_fitted(self) 
        
                   if sort_results and not return_distance: 
        
                       raise ValueError("return_distance must be True if sort_results is True.")

@jjerphan I don't think we need a dedicated PR for this as all we are doing in #26828 is shifting this check a bit earlier as required.

jjerphan · 2023-08-18T07:25:28Z

OK, feel free to close this PR.

glemaitre · 2023-08-21T09:37:55Z

Thanks @OmarManzoor for investigating.

MAINT call in RadiusNeighborsClassifer predict_proba

ae84243

OmarManzoor added the Quick Review For PRs that are quick to review label Aug 11, 2023

github-actions bot added the module:neighbors label Aug 11, 2023

OmarManzoor added the No Changelog Needed label Aug 11, 2023

jjerphan reviewed Aug 11, 2023

View reviewed changes

OmarManzoor changed the title ~~MAINT call in RadiusNeighborsClassifer predict_proba~~ FIX Check that RadiusNeighborsClassifier is fit in predict{,_proba} Aug 11, 2023

jjerphan approved these changes Aug 13, 2023

View reviewed changes

* Remove test from test_neighbors.py

2398ba7

* Add a common test to check for unfitted classifiers * Fix ClassifierChain by adding check_is_fitted inside predict_proba and decision_function methods

jjerphan approved these changes Aug 15, 2023

View reviewed changes

OmarManzoor changed the title ~~FIX Check that RadiusNeighborsClassifier is fit in predict{,_proba}~~ FIX Add common test to check for unfitted behaviour in classifiers and fix RadiusNeighborsClassifier and ClassifierChain correspondingly Aug 15, 2023

OmarManzoor closed this Aug 18, 2023

OmarManzoor deleted the radius_neighbors_classifier_is_fitted branch August 18, 2023 10:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Add common test to check for unfitted behaviour in classifiers and fix RadiusNeighborsClassifier and ClassifierChain correspondingly #27051

FIX Add common test to check for unfitted behaviour in classifiers and fix RadiusNeighborsClassifier and ClassifierChain correspondingly #27051

		"""Check that :class:`RadiusNeighborsClassifier` is fitted before
		calling the :meth:`predict` and :meth:`predict_proba`.

FIX Add common test to check for unfitted behaviour in classifiers and fix RadiusNeighborsClassifier and ClassifierChain correspondingly #27051

FIX Add common test to check for unfitted behaviour in classifiers and fix RadiusNeighborsClassifier and ClassifierChain correspondingly #27051

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

✔️ Linting Passed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment