8000 Bug in common tests: should use utils.testing._get_args · Issue #9350 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

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

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
Closed
amueller opened this issue Jul 13, 2017 · 3 comments · Fixed by #11277
Closed

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

amueller opened this issue Jul 13, 2017 · 3 comments · Fixed by #11277
Labels
help wanted Sprint

Comments

@amueller
Copy link
Member

Right now we are using inspect in some common tests (utils.estimator_checks) to get method arguments. But that doesn't work if the method is decorated, for example with a deprecation.

In that case we need to use utils.testing._get_args. So we need to make sure to use that whenever we inspect arguments.
A good test would be to create a model that has all methods deprecated and see if the tests still pass.
We should also check whether the sample_weights are detected correctly.
@amueller amueller mentioned this issue Jul 13, 2017
Open
@tsdlovell
Copy link
Contributor

Is this still an open issue? I don't see any usages of inspect in utils.estimator_checks

git grep inspect -- utils/estimator_checks.py | wc -l
0

@markroth8
Copy link
Contributor

@tsdlovell - After looking at the code a bit, it's possible inspect (or something like it) is being used indirectly.

I'm working on this. My first step is to confirm the issue still exists by writing a test.

@markroth8
Copy link
Contributor

Discussed this with @amueller and we agreed the most direct approach to confirming this issue is to create a unit test for has_fit_parameter in validation.py which checks that it returns True if the parameter is present, even if the fit method is marked as deprecated.

If False, the bug is confirmed. If True, the bug is already fixed but we can still commit the test.

@markroth8 markroth8 mentioned this issue Jun 15, 2018
Merged
jnothman pushed a commit that referenced this issue Jun 20, 2018
@markroth8 @jnothman
MAINT Fix #9350: Enable has_fit_parameter() and fit_score_takes_y() t…
c50dc0e 
…o work with @deprecated in Python 2 (#11277)
georgipeev pushed a commit to georgipeev/scikit-learn that referenced this issue 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
help wanted Sprint
Projects
None yet
Milestone
No milestone
4 participants
@amueller @lesteve @markroth8 @tsdlovell
0