Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[MRG+1] Adding multi output checks to common tests #13392

Merged

Conversation

rok
Copy link
Contributor

@rok rok commented Mar 5, 2019

Reference Issues/PRs

Fixes #13187.

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 Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 5328660 to b720e06 Compare Mar 7, 2019
@rok rok changed the title Adding multi output checks to common tests [MRG] Adding multi output checks to common tests Mar 7, 2019
Copy link
Member

@TomDLT TomDLT left a comment

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

sklearn/neighbors/regression.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/base.py Outdated Show resolved Hide resolved
sklearn/linear_model/coordinate_descent.py Outdated Show resolved Hide resolved
sklearn/linear_model/least_angle.py Outdated Show resolved Hide resolved
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 33aa2ab to 8618a0e Compare Mar 17, 2019
Copy link
Member

@TomDLT TomDLT left a comment

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 Outdated Show resolved Hide resolved
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 9450697 to 499fc5b Compare Mar 18, 2019
@jnothman
Copy link
Member

@jnothman jnothman commented Mar 19, 2019

@jnothman
Copy link
Member

@jnothman jnothman commented Mar 19, 2019

@rok
Copy link
Contributor Author

@rok rok commented Mar 19, 2019

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
TomDLT approved these changes Mar 22, 2019
Copy link
Member

@TomDLT TomDLT left a comment

LGTM

We just need to wait for a second review.

doc/whats_new/v0.21.rst Outdated Show resolved Hide resolved
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 85cf3c0 to c53179d Compare Mar 28, 2019
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
@rok
Copy link
Contributor Author

@rok rok commented Apr 3, 2019

Ping! :)

TomDLT
TomDLT approved these changes Apr 3, 2019
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 6a222f3 to 9a482b6 Compare Apr 11, 2019
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch 5 times, most recently from c9ceb6e to 3f3671b Compare May 9, 2019
@rok
Copy link
Contributor Author

@rok rok commented May 10, 2019

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

@TomDLT TomDLT added this to the 0.22 milestone May 10, 2019
@rth rth self-requested a review Jun 25, 2019
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch 3 times, most recently from 14df99b to 86e6baf Compare Jul 20, 2019
@rok
Copy link
Contributor Author

@rok rok commented Jul 20, 2019

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

@amueller
Copy link
Member

@amueller amueller commented Sep 4, 2019

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

@glemaitre glemaitre added this to GETTING STALLED in Guillaume's pet Sep 10, 2019
@jnothman
Copy link
Member

@jnothman jnothman commented Sep 18, 2019

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

@rok
Copy link
Contributor Author

@rok rok commented Sep 20, 2019

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

@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch 3 times, most recently from 0f23831 to 1969e70 Compare Sep 21, 2019
@rok
Copy link
Contributor Author

@rok rok commented Sep 21, 2019

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!

@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 1969e70 to fd6c04d Compare Sep 23, 2019
Copy link
Contributor

@glemaitre glemaitre left a comment

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 Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
estimator.fit(X, y)
y_pred = estimator.predict(X)

assert y_pred.dtype == np.dtype('float')
Copy link
Contributor

@glemaitre glemaitre Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

@glemaitre glemaitre Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also have a similar style as before

assert y_pred.dtype.kind == 'f'

Copy link
Contributor Author

@rok rok Sep 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to set this to 'float64'.

sklearn/utils/estimator_checks.py Outdated Show resolved Hide resolved
@glemaitre glemaitre self-requested a review Sep 24, 2019
Copy link
Contributor

@glemaitre glemaitre left a comment

So a couple of changes required.

@glemaitre glemaitre moved this from GETTING STALLED to REVIEWED AND WAITING FOR CHANGES in Guillaume's pet Sep 24, 2019
@rok
Copy link
Contributor Author

@rok rok commented Sep 24, 2019

Thanks @glemaitre. I'll do this tonight.

rok and others added 3 commits Sep 24, 2019
[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 rok force-pushed the adding_multi-output_checks_to_common_tests branch from c1457ad to 8cef195 Compare Sep 24, 2019
@rok
Copy link
Contributor Author

@rok rok commented Sep 24, 2019

@glemaitre done, please review. :)

@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch 2 times, most recently from bb8f4eb to 78f3938 Compare Sep 24, 2019
@rok rok force-pushed the adding_multi-output_checks_to_common_tests branch from 78f3938 to 8eabe3b Compare Sep 24, 2019
@rok
Copy link
Contributor Author

@rok rok commented Sep 30, 2019

@glemaitre ping :)

@glemaitre glemaitre merged commit 5e4b275 into scikit-learn:master Oct 1, 2019
13 of 15 checks passed
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Oct 1, 2019

Thanks @rok

@glemaitre glemaitre moved this from REVIEWED AND WAITING FOR CHANGES to MERGED in Guillaume's pet Oct 1, 2019
@rok
Copy link
Contributor Author

@rok rok commented Oct 1, 2019

Thanks all! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

6 participants