EHN make some meta-estimators lenient towards missing values #17987

venkyyuvy · 2020-07-25T14:13:26Z

Reference Issues/PRs: Fixes: #15319

What does this implement/fix? Explain your changes.

Any other comments?

as false in check array and check_X_y

sklearn/tests/test_multioutput.py

ogrisel · 2020-07-25T15:57:08Z

I marked this PR as [WIP] to let other know that it's still a work in progress as MultiOutputClassifier needs to be changed to make it lenient to missing values. Feel free to update the title again when the test pass to implicitly ask for reviews.

venkyyuvy · 2020-07-25T16:13:22Z

@ogrisel

Do I have to add one more test for MultiOutputRegressor as well?

One more question, Is there any other meta estimator, which is not allowing NaN values to passthrough?

jnothman · 2020-07-26T00:32:51Z

It wouldn't hurt to test MultiOutputRegressor too. I think the point of the issue is that none of the core developers knows what the status of our support and testing of this property is. We would like a contributor to review how well passing missing values (and ideally other things like non-2d or non-array-like data) is supported, and that each case where it is supported, this support is tested. Once that has all been verified, then we will know :)

ogrisel

LGTM! As @jnothman said, it would be great to do the same for MultiOutputRegressor as part of the same PR while you are at it.

…mators

venkyyuvy · 2020-07-30T10:05:48Z

Status before this PR.

estimator-type	model	support_missing_value	test
ensemble	BaggingRegressor	yes	test_bagging_regressor_with_missing_inputs
ensemble	BaggingClassifier	yes	test_bagging_classifier_with_missing_inputs
ensemble	VotingClassifier	yes	test_classifier_for_missing_data
ensemble	VotingRegressor	yes	test_regressor_for_missing_data
ensemble	StackingClassifier	yes	test_clf_support_missing_values
ensemble	StackingRegressor	yes	test_reg_support_missing_values
wrapper	MultiOutputRegressor	no	test_leniency_for_missing_data_regression
wrapper	MultiOutputClassifier	no	test_leniency_for_missing_data_classification
wrapper	OneVsRestClassifier	yes	test_support_missing_values
wrapper	OneVsOneClassifier	no	test_support_missing_values
pipeline	Pipeline	yes	test_missing_values_leniency
search	GridseachCV	yes	test_grid_search_allows_nans
feature_selector	RFE	yes	test_rfe_allow_nan_inf_in_x
feature_selector	RFECV	yes	test_rfe_allow_nan_inf_in_x
feature_selector	SelectFromModel	yes	test_fit_accepts_nan_inf

jnothman · 2020-07-30T10:17:48Z

Excellent. Very informative!

jnothman · 2020-07-30T10:18:04Z

Tests here are failing, however.

jnothman · 2020-07-30T10:19:28Z

There are other meta-estimators, however, such as StackingClassifier, SelectFromModel, RFE, Pipeline, GridSearchCV, etc...

venkyyuvy · 2020-07-30T10:24:21Z

I have not fixed for OneVsOneClassifier yet. Just, I have added test for it .

StackingClassifier, SelectFromModel, RFE, Pipeline, GridSearchCV, etc

Thank you for the pointers, I will check on these as well.

glemaitre · 2020-07-31T08:19:07Z

Out of scope of this PR to not create a monster but I think that we need to introduce common tests for checking nan behaviour.
For transformers it would be quite simple, I did not think yet about the meta-estimator case.

venkyyuvy · 2020-07-31T08:28:53Z

@glemaitre ,
what's your suggestion on going about building a common test for nan behaviour?

Currently I am manually checking through each one of them. I am checking for meta-estimators and wrappers and not covering the transformer

sklearn/ensemble/tests/test_stacking.py

sklearn/ensemble/tests/test_voting.py

sklearn/tests/test_multiclass.py

glemaitre · 2020-07-31T08:36:14Z

what's your suggestion on going about building a common test for nan behaviour?

So reviewing the code, we are repeating the same tests for all meta-estimators. So we could isolate this test and put it in estimator_checks.py.

Then, we need to call this test with the proper estimator. So with meta-estimator, we need to make sure that we pass a base estimator that has a pipeline with a SimpleImputer (this should be possible when building estimator instances).

glemaitre · 2020-07-31T08:38:43Z

And basically we should introduce this #9741 at the same time :)

glemaitre · 2020-08-03T06:51:33Z

We can also change these multiple tests to one, without making an automatic
common test.

Yes we could move those sklearn/ensemble/test_common.py, this is a good idea.

venkyyuvy · 2020-08-04T10:05:49Z

Thanks, I will move the tests of voting and stacking meta estimators to sklearn/ensemble/test_common.py.
Can we keep the MultiOutput and MultiClass wrapper tests there itself?

…mators

sklearn/tests/test_multiclass.py

sklearn/tests/test_multioutput.py

sklearn/ensemble/tests/test_common.py

sklearn/tests/test_multiclass.py

sklearn/ensemble/tests/test_common.py

glemaitre · 2020-08-14T07:25:20Z

Test are failing. Could you also add a comment on the test to indicate that we should remove them when we create a common test

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

venkyyuvy · 2020-08-14T11:13:11Z

Test are failing. Could you also add a comment on the test to indicate that we should remove them when we create a common test

I'm not sure why the test are failing with Attribute error (rng.random). The tests are running fine in my end.
Could you please let me know what is going wrong?

glemaitre · 2020-08-14T14:28:08Z

rng.random does not exist on older version of numpy. Maybe use rng.randn to generate randomly normal generated data

doc/whats_new/v0.24.rst

justmarkham · 2020-08-19T20:50:43Z

doc/whats_new/v0.24.rst

@@ -302,6 +302,22 @@ Changelog
  validity of the input is now delegated to the base estimator.
  :pr:`17233` by :user:`Zolisa Bleki <zoj613>`.

+- |Enhancement| :class:`multiclass.OneVsOneClassifier` now accpets


Typo: accpets -> accepts

justmarkham · 2020-08-19T20:51:01Z

doc/whats_new/v0.24.rst

+..........................
+
+- |Enhancement| :class:`multioutput.MultiOutputClassifier` and 
+  :class:`multioutput.MultiOutputRegressor` now accpets the inputs


Typo: accpets -> accepts

jnothman · 2020-08-21T02:22:57Z

This is much neater. @venkyyuvy could you please resolve those nitpicks from @justmarkham, and resolve conflicts with master, so that we can merge. Thanks again!!

sklearn/tests/test_multiclass.py

glemaitre

Only add 2 additional comments. LGTM otherwise (fixing what was pointed out earlier).

sklearn/tests/test_multioutput.py

glemaitre · 2020-08-26T09:10:43Z

Thanks @venkyyuvy Let's merge this. It is super useful.

…learn#17987) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

venkyyuvy added 2 commits July 25, 2020 19:40

test for missing value leniency

a5e0566

making ensure_finite_values

8db866b

as false in check array and check_X_y

ogrisel reviewed Jul 25, 2020

View reviewed changes

sklearn/tests/test_multioutput.py Outdated Show resolved Hide resolved

ogrisel changed the title ~~Meta estimators leniency towards missing values~~ [WIP] Meta estimators leniency towards missing values Jul 25, 2020

linting fix

b0777a9

venkyyuvy changed the title ~~[WIP] Meta estimators leniency towards missing values~~ Meta estimators leniency towards missing values Jul 25, 2020

venkyyuvy requested a review from ogrisel July 25, 2020 16:44

ogrisel approved these changes Jul 26, 2020

View reviewed changes

venkyyuvy added 3 commits July 29, 2020 09:50

test for multi output regression

a4d027f

Merge remote-tracking branch 'upstream/master' into lenient_meta_esti…

656fac9

…mators

test for voting classifier

ed33bf6

github-actions bot added the module:ensemble label Jul 29, 2020

venkyyuvy added 2 commits July 30, 2020 09:45

test for voting_regressor

3955ae4

test for multi class wrappers

443f334

venkyyuvy added 2 commits July 30, 2020 16:06

test for stacking estimators

696d471

removing finite value checking

f1443d5

test pipeline

173e6e5

glemaitre reviewed Jul 31, 2020

View reviewed changes

cmarmo added the Needs Info label Aug 6, 2020

venkyyuvy added 4 commits August 12, 2020 08:31

moving tests to test_common

b9a9789

Merge remote-tracking branch 'upstream/master' into lenient_meta_esti…

6241b4f

…mators

change log

0b74039

fix

e057c81

glemaitre reviewed Aug 13, 2020

View reviewed changes

sklearn/tests/test_multiclass.py Outdated Show resolved Hide resolved

glemaitre reviewed Aug 13, 2020

View reviewed changes

venkyyuvy added 2 commits August 14, 2020 07:28

reivew changes

2f4ea45

reivew changes

e32103d

glemaitre reviewed Aug 14, 2020

View reviewed changes

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

Update sklearn/ensemble/tests/test_common.py

6977ba7

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

glemaitre reviewed Aug 14, 2020

View reviewed changes

doc/whats_new/v0.24.rst Outdated Show resolved Hide resolved

changing numpy rand generation & whats_new

4456da8

cmarmo removed the Needs Info label Aug 15, 2020

justmarkham reviewed Aug 19, 2020

View reviewed changes

glemaitre reviewed Aug 21, 2020

View reviewed changes

sklearn/tests/test_multiclass.py Show resolved Hide resolved

glemaitre approved these changes Aug 21, 2020

View reviewed changes

sklearn/tests/test_multioutput.py Show resolved Hide resolved

glemaitre changed the title ~~Meta estimators leniency towards missing values~~ EHN make some meta-estimators lenient towards missing values Aug 21, 2020

rebasing and incorporating review comments

04ee366

glemaitre merged commit 8ba49f6 into scikit-learn:master Aug 26, 2020

glemaitre mentioned this pull request Aug 26, 2020

Make sure meta-estimators are lenient towards missing data #15319

Closed

jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020

EHN make some meta-estimators lenient towards missing values (scikit-…

f934496

…learn#17987) Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EHN make some meta-estimators lenient towards missing values #17987

EHN make some meta-estimators lenient towards missing values #17987

venkyyuvy commented Jul 25, 2020

ogrisel commented Jul 25, 2020

venkyyuvy commented Jul 25, 2020

jnothman commented Jul 26, 2020 via email

ogrisel left a comment

venkyyuvy commented Jul 30, 2020 •

edited

Loading

jnothman commented Jul 30, 2020

jnothman commented Jul 30, 2020

jnothman commented Jul 30, 2020

venkyyuvy commented Jul 30, 2020 •

edited

Loading

glemaitre commented Jul 31, 2020

venkyyuvy commented Jul 31, 2020 •

edited

Loading

glemaitre commented Jul 31, 2020

glemaitre commented Jul 31, 2020

glemaitre commented Aug 3, 2020

venkyyuvy commented Aug 4, 2020

glemaitre commented Aug 14, 2020

venkyyuvy commented Aug 14, 2020

glemaitre commented Aug 14, 2020

justmarkham Aug 19, 2020

justmarkham Aug 19, 2020

jnothman commented Aug 21, 2020

glemaitre left a comment

glemaitre commented Aug 26, 2020

EHN make some meta-estimators lenient towards missing values #17987

EHN make some meta-estimators lenient towards missing values #17987

Conversation

venkyyuvy commented Jul 25, 2020

Reference Issues/PRs: Fixes: #15319

What does this implement/fix? Explain your changes.

Any other comments?

ogrisel commented Jul 25, 2020

venkyyuvy commented Jul 25, 2020

jnothman commented Jul 26, 2020 via email

ogrisel left a comment

Choose a reason for hiding this comment

venkyyuvy commented Jul 30, 2020 • edited Loading

jnothman commented Jul 30, 2020

jnothman commented Jul 30, 2020

jnothman commented Jul 30, 2020

venkyyuvy commented Jul 30, 2020 • edited Loading

glemaitre commented Jul 31, 2020

venkyyuvy commented Jul 31, 2020 • edited Loading

glemaitre commented Jul 31, 2020

glemaitre commented Jul 31, 2020

glemaitre commented Aug 3, 2020

venkyyuvy commented Aug 4, 2020

glemaitre commented Aug 14, 2020

venkyyuvy commented Aug 14, 2020

glemaitre commented Aug 14, 2020

justmarkham Aug 19, 2020

Choose a reason for hiding this comment

justmarkham Aug 19, 2020

Choose a reason for hiding this comment

jnothman commented Aug 21, 2020

glemaitre left a comment

Choose a reason for hiding this comment

glemaitre commented Aug 26, 2020

venkyyuvy commented Jul 30, 2020 •

edited

Loading

venkyyuvy commented Jul 30, 2020 •

edited

Loading

venkyyuvy commented Jul 31, 2020 •

edited

Loading