MNT Checking function `_estimator_has` also raises `AttributeError` #28167

StefanieSenger · 2024-01-18T13:30:51Z

Reference Issues/PRs

What does this implement/fix? Explain your changes.

This PR aims to display a more understandable error message in the case when sub-estimators don't implement a method, the meta-estimator that they are being used in DOES implement. See the issue for an example.

pushes the sub-estimator's AttributeError to be raised during available_if, to prevent the too generic error message from _AvailableIfDescriptor from being raised
I have checked for code aimed to raise the error locally in the corresponding meta-estimator, that is not used anymore: except for in OneVsRestClassifier, nothing was to be found

github-actions · 2024-01-18T13:32:11Z

✔️ Linting Passed

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

_{Generated for commit: 2d24051. Link to the linter CI: here}

glemaitre · 2024-01-18T14:51:28Z

The failure is not associated with this PR. I open #28168 trying to solve the issue.

glemaitre

In terms of source code, it looks good. We need to add non-regression case for each case or if the test already exist, we need to match the class name to be sure that we have the proper error message.

I gave an example for the stacking case.

sklearn/ensemble/_stacking.py

sklearn/feature_selection/_from_model.py

sklearn/feature_selection/_rfe.py

sklearn/semi_supervised/_self_training.py

sklearn/multiclass.py

glemaitre · 2024-01-22T14:51:49Z

I just realized that I commented in the issue and not the PR. @StefanieSenger you can have a look at the following comment regarding the unit tests: #28108 (comment)

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

StefanieSenger · 2024-02-02T15:13:07Z

I've updated the tests as we have talked about, @glemaitre. They all pass now.

But there are new issues popping up in sklearn.inspection around partial_dependence, that are also unrelated, I believe.

glemaitre

We will need to have an entry in the changelog since we fix the error message.

glemaitre · 2024-02-05T14:14:46Z

Uhm I don't see the error with the partial_dependence. Where did you see the traceback @StefanieSenger?

glemaitre

Otherwise LGTM. Thanks @StefanieSenger

sklearn/ensemble/tests/test_stacking.py

sklearn/semi_supervised/tests/test_self_training.py

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

StefanieSenger · 2024-02-05T17:57:51Z

I've made those little requested changes, @glemaitre. Thanks for reviewing and suggesting.

This is it for this PR.

On the unrelated test failure: Locally, when I run pytest sklearn/inspection I get 84 times: ValueError: Buffer dtype mismatch, expected 'int' but got 'long'. It all traces back to sklearn/tree/_tree.pyx and other .pyx files.

I also get this on my main branch, but not a branch where I had last pulled 4 weeks ago . My compilers are up to date with make in, this is still a valid way to re-build, correct?

I think it's connected to #27546 and somehow the re-build didn't work out for me.

adrinjalali

Nits, otherwise LGTM.

sklearn/ensemble/_stacking.py

adrinjalali · 2024-02-06T13:26:36Z

sklearn/ensemble/tests/test_stacking.py

+    X, y = load_breast_cancer(return_X_y=True)
+    X_train, X_test, y_train, _ = train_test_split(
+        scale(X), y, stratify=y, random_state=42
+    )


we can probably use a smaller dataset, and avoid calling scale to make the test faster.

The scale would avoid a ConvergenceWarning of the LogisticRegression certainly. To be checked if we remove it.

we can do make_classification instead then. Would be faster. The data doesn't matter here.

I now put make_classifiction for data creation.

glemaitre · 2024-02-06T14:19:37Z

I also get this on my main branch, but not a branch where I had last pulled 4 weeks ago . My compilers are up to date with make in, this is still a valid way to re-build, correct?

@StefanieSenger Indeed, the error look like the type in the Cython file changed. So a good make clean && make in could make the trick (you also try to build with meson (https://scikit-learn.org/dev/developers/advanced_installation.html#building-with-meson) It will then detect automatically if something change and make the rebuild for you.

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

StefanieSenger · 2024-02-12T11:20:54Z

@glemaitre
Thank you for the guidance with the build. My build hadn't worked properly (and I didn't realise that). Now it's resolved. :)
I will try meson on another occasion.

…cikit-learn#28167) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

…28167) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

checking function _estimator_has also raises AttributeError

ee7c926

github-actions bot added module:ensemble module:feature_selection module:semi_supervised labels Jan 18, 2024

glemaitre self-requested a review January 18, 2024 14:40

glemaitre mentioned this pull request Jan 18, 2024

DOC use list for the ridge_regression docstring #28168

Merged

glemaitre reviewed Jan 18, 2024

View reviewed changes

StefanieSenger and others added 4 commits February 1, 2024 15:16

Merge branch 'main' into available_if

15803d8

Apply suggestions from code review

e9c276d

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

implemented tests, but 2/5 indicate further improvements necessary

d8870b0

use different tests

e894f5c

glemaitre reviewed Feb 5, 2024

View reviewed changes

glemaitre approved these changes Feb 5, 2024

View reviewed changes

sklearn/ensemble/tests/test_stacking.py Outdated Show resolved Hide resolved

sklearn/ensemble/tests/test_stacking.py Outdated Show resolved Hide resolved

sklearn/semi_supervised/tests/test_self_training.py Outdated Show resolved Hide resolved

StefanieSenger and others added 2 commits February 5, 2024 18:36

changes after review

f41cee9

Update sklearn/semi_supervised/tests/test_self_training.py

9710acb

Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Merge branch 'main' into available_if

31afac5

adrinjalali reviewed Feb 6, 2024

View reviewed changes

StefanieSenger and others added 2 commits February 12, 2024 11:58

Update sklearn/ensemble/_stacking.py

4a6d7ba

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

suggestions from review

2d24051

adrinjalali approved these changes Feb 13, 2024

View reviewed changes

adrinjalali merged commit 9a6e6dd into scikit-learn:main Feb 13, 2024

StefanieSenger deleted the available_if branch February 13, 2024 14:14

glemaitre added a commit that referenced this pull request Feb 13, 2024

MNT Checking function _estimator_has also raises AttributeError (#…

3564f20

…28167) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com> Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

glemaitre mentioned this pull request May 18, 2024

MAINT define a single time _estimator_has and refactor code #29046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MNT Checking function `_estimator_has` also raises `AttributeError` #28167

MNT Checking function `_estimator_has` also raises `AttributeError` #28167

StefanieSenger commented Jan 18, 2024

github-actions bot commented Jan 18, 2024 •

edited

Loading

glemaitre commented Jan 18, 2024

glemaitre left a comment

glemaitre commented Jan 22, 2024

StefanieSenger commented Feb 2, 2024

glemaitre left a comment

glemaitre commented Feb 5, 2024

glemaitre left a comment

StefanieSenger commented Feb 5, 2024 •

edited

Loading

adrinjalali left a comment

adrinjalali Feb 6, 2024

glemaitre Feb 6, 2024

adrinjalali Feb 6, 2024

StefanieSenger Feb 12, 2024

glemaitre commented Feb 6, 2024

StefanieSenger commented Feb 12, 2024 •

edited

Loading

MNT Checking function _estimator_has also raises AttributeError #28167

MNT Checking function _estimator_has also raises AttributeError #28167

Conversation

StefanieSenger commented Jan 18, 2024

Reference Issues/PRs

What does this implement/fix? Explain your changes.

github-actions bot commented Jan 18, 2024 • edited Loading

✔️ Linting Passed

glemaitre commented Jan 18, 2024

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented Jan 22, 2024

StefanieSenger commented Feb 2, 2024

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented Feb 5, 2024

glemaitre left a comment

Choose a reason for hiding this comment

StefanieSenger commented Feb 5, 2024 • edited Loading

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali Feb 6, 2024

Choose a reason for hiding this comment

glemaitre Feb 6, 2024

Choose a reason for hiding this comment

adrinjalali Feb 6, 2024

Choose a reason for hiding this comment

StefanieSenger Feb 12, 2024

Choose a reason for hiding this comment

glemaitre commented Feb 6, 2024

StefanieSenger commented Feb 12, 2024 • edited Loading

MNT Checking function `_estimator_has` also raises `AttributeError` #28167

MNT Checking function `_estimator_has` also raises `AttributeError` #28167

github-actions bot commented Jan 18, 2024 •

edited

Loading

StefanieSenger commented Feb 5, 2024 •

edited

Loading

StefanieSenger commented Feb 12, 2024 •

edited

Loading