[MRG+1] Adding multi output checks to common tests by rok · Pull Request #13392 · scikit-learn/scikit-learn

rok · 2019-03-05T15:33:46Z

Reference Issues/PRs

Changes

This implements new classifier and regressor checks to test for multi-output support in common tests.
Some random forest and tree classifier test are removed as they are duplicating this functionality.

sklearn/utils/estimator_checks.py

TomDLT

Not sure about these tag changes (#13392 (comment)). ping @amueller

sklearn/neighbors/regression.py

sklearn/utils/estimator_checks.py

sklearn/base.py

sklearn/linear_model/coordinate_descent.py

sklearn/linear_model/least_angle.py

TomDLT

Thanks for the update !
I like much more your Mixin fix now.

You will also need to add en entry in https://github.com/scikit-learn/scikit-learn/blob/master/doc/whats_new/v0.21.rst#changes-to-estimator-checks, writing the change, this PR number and your GitHub name.

sklearn/utils/tests/test_estimator_checks.py

jnothman · 2019-03-19T11:50:18Z

I wonder if requires_positive_data is implied by accepting pairwise data in all cases.

jnothman · 2019-03-19T11:50:42Z

But there's certainly nothing wrong with conditional tags

rok · 2019-03-19T12:14:04Z

I wonder if requires_positive_data is implied by accepting pairwise data in all cases.

@jnothman - right, is there some other way to know?

TomDLT

LGTM

We just need to wait for a second review.

doc/whats_new/v0.21.rst

sklearn/ensemble/tests/test_forest.py

sklearn/utils/estimator_checks.py

rok · 2019-04-03T20:44:13Z

Ping! :)

rok · 2019-05-10T00:16:19Z

I've rebased and maybe we could get it merged in v0.22? :)

rok · 2019-07-20T22:06:53Z

Rebased and fixed new issues. Since things changed it might be good to do another review.

amueller · 2019-08-12T15:39:37Z

sklearn/neighbors/regression.py

              metric_params=metric_params, n_jobs=n_jobs, **kwargs)
        self.weights = _check_weights(weights)

+    def _more_tags(self):


Shouldn't this be handled by _pairwise?

amueller · 2019-08-12T15:44:27Z

The reason why we need to add the intermediate classes into the hierarchy is because we don't add the mixins to the correct (that is left) side, and so we can't have nice things #14044?

Would it be feasible / sensible to actually change the order of mixins and allow overwriting tags? That would simplify a bunch of this, right?

amueller · 2019-08-12T17:13:28Z

posted #14635 which will probably simplify this

amueller · 2019-09-04T19:35:59Z

ok this has a chance to be merged soon and would make this much easier: #14884

jnothman · 2019-09-18T11:31:42Z

With #14884 merged, and a bunch of nitpicks above, are you going to bring this to conclusion, @rok? Thanks.

rok · 2019-09-20T08:20:10Z

@jnothman - will try to finish this tonight. Sorry for the delay :)

rok · 2019-09-21T13:18:16Z

I'm not sure if I got the general approach right but I have:

moved mixins to the left
replaced intermediate classes by adding _more_tags to classes that inherit multioutput tag as True but should have it set to False. E.g. Lars is a multioutput class but LarsCV that inherits tags from Lars is not. We adjust LarsCV tags.

Please review :).

Thanks for pushing this @glemaitre @jnothman @amueller!

glemaitre

There is a couple of missing assert in the tests and I would add meaningful error message.
It would ease debugging when rolling your own estimator and that check_estimator is failing.

sklearn/utils/estimator_checks.py

glemaitre · 2019-09-24T08:39:16Z

sklearn/utils/estimator_checks.py

+    estimator.fit(X, y)
+    y_pred = estimator.predict(X)
+
+    assert y_pred.dtype == np.dtype('float')


Let's add an error message

"Multi-output predictions by a regressor are expected to be floating-point precision. Got {} instead".format(y_pred.dtype)

We can also have a similar style as before

assert y_pred.dtype.kind == 'f'

Had to set this to 'float64'.

sklearn/utils/estimator_checks.py

glemaitre

So a couple of changes required.

rok · 2019-09-24T09:38:54Z

Thanks @glemaitre. I'll do this tonight.

[MRG] Adding multi output checks to common tests (scikit-learn#13187) Removing redundant tests. Adding tests to check_classifier_multioutput and check_regressor_multioutput.

Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>

rok · 2019-09-24T16:19:13Z

@glemaitre done, please review. :)

rok · 2019-09-30T11:34:09Z

@glemaitre ping :)

glemaitre · 2019-10-01T12:20:21Z

Thanks @rok

rok · 2019-10-01T13:32:25Z

Thanks all! :)

rok mentioned this pull request Mar 5, 2019

Missing multi-output checks in common tests #13187

Closed

TomDLT suggested changes Mar 5, 2019

View reviewed changes

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 5328660 to b720e06 Compare March 7, 2019 14:39

rok changed the title ~~Adding multi output checks to common tests~~ [MRG] Adding multi output checks to common tests Mar 7, 2019

TomDLT reviewed Mar 13, 2019

View reviewed changes

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 33aa2ab to 8618a0e Compare March 17, 2019 00:50

TomDLT suggested changes Mar 18, 2019

View reviewed changes

sklearn/utils/tests/test_estimator_checks.py Outdated Show resolved Hide resolved

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 9450697 to 499fc5b Compare March 18, 2019 22:36

TomDLT approved these changes Mar 22, 2019

View reviewed changes

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

thomasjpfan reviewed Mar 28, 2019

View reviewed changes

sklearn/ensemble/tests/test_forest.py Show resolved Hide resolved

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 85cf3c0 to c53179d Compare March 28, 2019 19:32

TomDLT reviewed Mar 28, 2019

View reviewed changes

sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved

TomDLT approved these changes Apr 3, 2019

View reviewed changes

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 6a222f3 to 9a482b6 Compare April 11, 2019 16:42

rok force-pushed the adding_multi-output_checks_to_common_tests branch 5 times, most recently from c9ceb6e to 3f3671b Compare May 9, 2019 23:43

TomDLT added this to the 0.22 milestone May 10, 2019

rth self-requested a review June 25, 2019 12:10

rok force-pushed the adding_multi-output_checks_to_common_tests branch 3 times, most recently from 14df99b to 86e6baf Compare July 20, 2019 21:46

amueller reviewed Aug 12, 2019

View reviewed changes

rok force-pushed the adding_multi-output_checks_to_common_tests branch 3 times, most recently from 0f23831 to 1969e70 Compare September 21, 2019 12:45

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 1969e70 to fd6c04d Compare September 23, 2019 21:08

glemaitre reviewed Sep 24, 2019

View reviewed changes

glemaitre self-requested a review September 24, 2019 08:43

glemaitre requested changes Sep 24, 2019

View reviewed changes

rok and others added 3 commits September 24, 2019 18:15

Adding multi-output checks for estimators.

1d3c819

[MRG] Adding multi output checks to common tests (scikit-learn#13187) Removing redundant tests. Adding tests to check_classifier_multioutput and check_regressor_multioutput.

Switching to overwriting inherited tags.

b4070db

Update sklearn/utils/estimator_checks.py

2b7dab1

Co-Authored-By: Guillaume Lemaitre <g.lemaitre58@gmail.com>

rok force-pushed the adding_multi-output_checks_to_common_tests branch from c1457ad to 8cef195 Compare September 24, 2019 16:17

rok force-pushed the adding_multi-output_checks_to_common_tests branch 2 times, most recently from bb8f4eb to 78f3938 Compare September 24, 2019 16:55

Review feedback.

8eabe3b

rok force-pushed the adding_multi-output_checks_to_common_tests branch from 78f3938 to 8eabe3b Compare September 24, 2019 17:03

Update v0.22.rst

c12bb6d

glemaitre approved these changes Oct 1, 2019

View reviewed changes

glemaitre merged commit 5e4b275 into scikit-learn:master Oct 1, 2019

Uh oh!

Conversation

rok commented Mar 5, 2019

Reference Issues/PRs

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomDLT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TomDLT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jnothman commented Mar 19, 2019 via email

Uh oh!

jnothman commented Mar 19, 2019 via email

Uh oh!

rok commented Mar 19, 2019

Uh oh!

TomDLT left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rok commented Apr 3, 2019

Uh oh!

rok commented May 10, 2019

Uh oh!

rok commented Jul 20, 2019

Uh oh!

amueller Aug 12, 2019

Choose a reason for hiding this comment

Uh oh!

amueller commented Aug 12, 2019

Uh oh!

amueller commented Aug 12, 2019

Uh oh!

amueller commented Sep 4, 2019

Uh oh!

jnothman commented Sep 18, 2019

Uh oh!

rok commented Sep 20, 2019

Uh oh!

rok commented Sep 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glemaitre Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

glemaitre Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

rok Sep 24, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glemaitre left a comment

Choose a reason for hiding this comment

Uh oh!

rok commented Sep 24, 2019

rok commented Sep 21, 2019 •

edited

Loading