8000 FIX/ENH CheckingClassifier support parameters and sparse matrices by glemaitre · Pull Request #17259 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]

Skip to content
8000

FIX/ENH CheckingClassifier support parameters and sparse matrices #17259

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

Merged
merged 11 commits into from
May 18, 2020

Conversation

glemaitre
Copy link
Member
@glemaitre glemaitre commented May 18, 2020

Reference Issues/PRs

Required for #17233

What does this implement/fix? Explain your changes.

  • Solve bug with sparse matrices (usage of len instead of _num_samples)
  • Add the possibility to pass parameters to the check_* functions.
  • Add multiple tests
  • Add predict_proba and decision_function to be compliant with classifier API
  • Add more documentation since we should probably use it internally for some other testing purposes

@zoj613
Copy link
Contributor
zoj613 commented May 18, 2020

Is it possible to add a default decision_function or predict_proba inside CheckingClassifier?

@glemaitre glemaitre changed the title Add test checking classifier FIX/ENH CheckingClassifier support parameters and sparse matrices May 18, 2020
@glemaitre
Copy link
Member Author

Is it possible to add a default decision_function or predict_proba inside CheckingClassifier?

Yes, I was doing it :). I just pushed. Now I have to check what are the related failures.

@glemaitre
Copy link
Member Author

@adrinjalali @NicolasHug @thomasjpfan @jeremiedbb It would be cool if you could have a quick look at this. It will unlock #17233. It does not really change any behaviour and this is just an internal utility for the moment.

8000
Copy link
Member
@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

thanks @glemaitre , only nits, LGTM

y_pred = clf.predict(X)
assert_array_equal(y_pred, np.zeros(y_pred.size, dtype=np.int))

assert clf.score(X) == pytest.approx(0)
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to check for strict equality here

Copy link
Member Author

Choose a reason for hiding this comment

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

Since they are floating-point, why would you make strict equality?

Copy link
Member

Choose a reason for hiding this comment

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

because they're hard-coded ones and zeros. There's not going to be any floating issue there

Copy link
Member Author

Choose a reason for hiding this comment

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

I still consider this a bad pattern to have in the code.


y_proba = clf.predict_proba(X)
assert y_proba.shape == (150, 3)
assert_allclose(y_proba[:, 0], 1)
Copy link
Member

Choose a reason for hiding this comment

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

same here and for most other checks

Copy link
Member Author

Choose a reason for hiding this comment

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

proba are also floating point

glemaitre and others added 4 commits May 18, 2020 16:18
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>
Copy link
Member
@jeremiedbb jeremiedbb left a comment

Choose a reason for hiding this comment

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

lgtm. @NicolasHug feel free to merge if you think all your comments have been addressed.

@NicolasHug NicolasHug merged commit f2e873f into scikit-learn:master May 18, 2020
@NicolasHug
Copy link
Member

Thanks @glemaitre

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants