8000 [MRG + 1] Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() to work with @deprecated in Python 2 by markroth8 · Pull Request #11277 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

[MRG + 1] Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() to work with @deprecated in Python 2 #11277

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 20, 2018
Merged

[MRG + 1] Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() to work with @deprecated in Python 2 #11277

merged 4 commits into from
Jun 20, 2018

Conversation

markroth8
Copy link
Contributor

Reference Issues/PRs

Fixes #9350: Bug in common tests: should use utils.testing._get_args

What does this implement/fix? Explain your changes.

This fix does not use utils.testing._get_args() but rather adds two new unit tests confirm that #9350 is no longer an issue. I walked through this with @amueller during a sprint and he agrees these tests are the right ones to confirm the issue is resolved.

Specifically:

  1. I've added a test in test_validation.py that ensures has_fit_parameter() works for classes with a deprecated fit() method.

  2. This issue was referenced in PR [WIP] Adding classes argument to Gaussian Naive Bayes #9290 where check_fit_score_takes_y() was retur 8000 ning a "list index out of range" when a method in naive_bayes.py was marked as @deprecated, but succeeding when not deprecated. To ensure check_fit_score_takes_y() behaves correctly in this scenario, I've added another test to ensure check_fit_score_takes_y() works on a class that has a deprecated fit() method.

Any other comments?

These tests were discussed with @amueller during Two Sigma's "TS Open" 2018 sprint.

amueller
amueller reviewed Jun 15, 2018
View reviewed changes
sklearn/utils/tests/test_estimator_checks.py Outdated
# a deprecated fit method

class TestEstimatorWithDeprecatedFitMethod(BaseEstimator):
@deprecated("Deprecated for the purpose of testing check_fit_score_takes_y")
Copy link
Member

Choose a reason for hiding this comment

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

we checked that also works with the other decorators.

@amueller
Copy link
Member
amueller commented Jun 15, 2018 •
edited
Loading

ha, actually fails on Python2.7 with the error we saw! I'm not overly paranoid after all (in this case).
(also pep8 errors)

@markroth8
Copy link
Contributor Author

Aha!

Oh, good, that means my tests work! Okay, now on to fix the issue...

@markroth8 markroth8 changed the title [MRG] Fix #9350: Test has_fit_parameter() and fit_score_takes_y() work with @deprecated [MRG] Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() to work with @deprecated in Python 2 Jun 15, 2018
@amueller
Copy link
Member

LGTM, thanks!

@amueller amueller changed the title [MRG] Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() to work with @deprecated in Python 2 [MRG + 1] Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() to work with @deprecated in Python 2 Jun 15, 2018
jnothman
jnothman approved these changes Jun 17, 2018
View reviewed changes
Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I don't see why we are likely to be testing an estimator with a deprecated fit method. But I suppose this is fine.

sklearn/utils/tests/test_validation.py Outdated
def fit(self, X, y, sample_weight=None):
pass

assert_true(has_fit_parameter(TestClassWithDeprecatedFitMethod,
Copy link
Member

Choose a reason for hiding this comment

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

Please just use bare assert. We're phasing out the use of assert_true, assert_equals, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, replaced assert_true with assert.

@amueller
Copy link
Member

@jnothman same goes for if_delegate_has_attr, which we are likely to test. But this actually came up in a PR once.

jnothman
jnothman reviewed Jun 19, 2018
View reviewed changes
Copy link
Member
@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I can't see what codecov is complaining about... what line in the diff is not covered?

Otherwise, I'm happy to merge

@markroth8
Copy link
Contributor Author

I don't understand it, either. @amueller - any ideas?

jnothman
jnothman reviewed Jun 20, 2018
View reviewed changes
sklearn/utils/tests/test_validation.py
class TestClassWithDeprecatedFitMethod:
@deprecated("Deprecated for the purpose of testing has_fit_parameter")
def fit(self, X, y, sample_weight=None):
pass
Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see now. This doesn't have test coverage. That's fine.

@jnothman jnothman merged commit c50dc0e into scikit-learn:master Jun 20, 2018
@jnothman
Copy link
Member

Thanks!

georgipeev pushed a commit to georgipeev/scikit-learn that referenced this pull request Jun 20, 2018
@markroth8
MAINT Fix scikit-learn#9350: Enable has_fit_parameter() and fit_score…
9566738 
…_takes_y() to work with @deprecated in Python 2 (scikit-learn#11277)
@markroth8 markroth8 deleted the decorated-method-args branch June 20, 2018 14:00
@markroth8
Copy link
Contributor Author

Thanks, all!

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

@jnothman jnothman jnothman approved these changes

@amueller amueller amueller approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug in common tests: should use utils.testing._get_args
3 participants
@markroth8 @amueller @jnothman
0