Skip to content

[MRG] ENH allow to specify which methods should run a check in CheckingClassifier#15230

Merged
glemaitre merged 9 commits intoscikit-learn:masterfrom
BenjaminBossan:tests-for-checkingclassifier
May 26, 2020
Merged

[MRG] ENH allow to specify which methods should run a check in CheckingClassifier#15230
glemaitre merged 9 commits intoscikit-learn:masterfrom
BenjaminBossan:tests-for-checkingclassifier

Conversation

@BenjaminBossan
Copy link
Copy Markdown
Contributor

@BenjaminBossan BenjaminBossan commented Oct 13, 2019

Reference Issues/PRs

Adding tests as discussed in #15218.

What does this implement/fix? Explain your changes.

Add unit tests for CheckingClassifier, improve its docstring.

Any other comments?

I would propose renaming the foo_param argument to something more
useful, e.g. score_param since it affects the score method. Since
CheckingClassifier is located in _mocking.py, such a change should
not break the public API.

@BenjaminBossan BenjaminBossan changed the title Add unit tests for CheckingClassifier, improve docstring [MRG] Add unit tests for CheckingClassifier, improve docstring Oct 13, 2019
thomasjpfan
thomasjpfan reviewed Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Thank you @BenjaminBossan for the PR!

@BenjaminBossan
Address reviewer comments
e661e8a 
* parametrize tests where possible
* add test for classes_ attribute
* use pytest.approx for float comparison
@BenjaminBossan
Copy link
Copy Markdown
Contributor Author

BenjaminBossan commented Oct 15, 2019

@thomasjpfan Thanks for the helpful comments. I changed the code to reflect your suggestions. A few things I did differently, mainly to have each test only check one specific functionality.

thomasjpfan
thomasjpfan approved these changes Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

jnothman
jnothman reviewed Oct 15, 2019
View reviewed changes
Copy link
Copy Markdown
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.

Sorry I've only glanced at this

jnothman
jnothman approved these changes May 20, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Thanks @BenjaminBossan!

jnothman
jnothman reviewed May 20, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Actually this now has substantial conflicts with #17259. Sorry @BenjaminBossan... @glemaitre, does this PR still have something to offer?

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented May 20, 2020

First sorry to have rush without seeing this PR.

I think that there are a couple of tests that should be added because they are making covering a use case that I did not add in the other PR. I am going to solve the conflict and keep the required tests.

@glemaitre glemaitre self-assigned this May 20, 2020
@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented May 20, 2020

OK, I clean-up my mess from master. This would be what I would keep.
I think that the tests should pass.

@BenjaminBossan
Copy link
Copy Markdown
Contributor Author

BenjaminBossan commented May 20, 2020

It seems that some tests are failing, are you taking a look at them @glemaitre? I could also do it but not earlier than the weekend.

@glemaitre glemaitre mentioned this pull request May 21, 2020
Merged
@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented May 25, 2020

@jnothman I added a parameter to be able to make the check specific to a specific method if needed (which was the case).

@glemaitre glemaitre changed the title [MRG] Add unit tests for CheckingClassifier, improve docstring [MRG] ENH allow to specify which methods should run a check in CheckingClassifier May 25, 2020
thomasjpfan
thomasjpfan reviewed May 25, 2020
View reviewed changes
thomasjpfan
thomasjpfan approved these changes May 25, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM

sklearn/utils/tests/test_mocking.py
)

clf.fit(X, y)
if predict_method in methods_to_check:
Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan May 25, 2020

Choose a reason for hiding this comment

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

@glemaitre It's always fun reading how you parametrize things.

@glemaitre glemaitre merged commit 76df39f into scikit-learn:master May 26, 2020
@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented May 26, 2020

Thanks @BenjaminBossan. Sorry for the bumps on the road but it was worth.

@BenjaminBossan
Copy link
Copy Markdown
Contributor Author

BenjaminBossan commented May 26, 2020

Thank you @glemaitre for finishing this.

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@BenjaminBossan @glemaitre @viclafargue
ENH allow to specify which methods should run a check in CheckingClas…
298b0cb 
…sifier (scikit-learn#15230)

Co-authored-by: BenjaminBossan <b.bossan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@BenjaminBossan @glemaitre @jayzed82
ENH allow to specify which methods should run a check in CheckingClas…
738e841 
…sifier (scikit-learn#15230)

Co-authored-by: BenjaminBossan <b.bossan@gmail.com>
Co-authored-by: Guillaume Lemaitre <g.lemaitre58@gmail.com>
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

@glemaitre glemaitre glemaitre left review comments

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees

@glemaitre glemaitre

Labels

module:utils

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@BenjaminBossan @glemaitre @jnothman @thomasjpfan