8000 MNT Ignore xfail_checks tag in check_estimator, with warning by NicolasHug · Pull Request #17222 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

MNT Ignore xfail_checks tag in check_estimator, with warning #17222

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 8 commits into from
May 15, 2020
Merged

MNT Ignore xfail_checks tag in check_estimator, with warning #17222

merged 8 commits into from
May 15, 2020

Conversation

NicolasHug
Copy link
Member

Fixes #16958

Alternative to and Closes #16963
Alternative to and Closes #17219

Same as #17219 , but raises a warning when a check is skipped.

CC @rth @thomasjpfan

@NicolasHug NicolasHug mentioned this pull request May 14, 2020
Closed
@NicolasHug NicolasHug changed the title MNT Ignore xfail_checks in check_estimator, with warning [MRG] MNT Ignore xfail_checks in check_estimator, with warning May 14, 2020
thomasjpfan
thomasjpfan approved these changes May 14, 2020
View reviewed changes
Copy link
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.

In this case, xfail_checks skips the whole test and raises a warning. I think I am happy with this.

sklearn/utils/estimator_checks.py
checks_generator = chain.from_iterable(
check_estimator(estimator, generate_only=True)
for estimator in estimators)
names = (type(estimator).__name__ for estimator in estimators)
Copy link
Member

Choose a reason for hiding this comment

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

Generators everywhere!

rth
rth reviewed May 15, 2020
View reviewed changes
rth
rth approved these changes May 15, 2020
View reviewed changes
Copy link
Member
@rth rth left a comment

Choose a reason for hiding this comment

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

Minor improvement otherwise LGTM!

I'll add Thomas as co-author when merging given all the work he did on this.

@NicolasHug NicolasHug changed the title [MRG] MNT Ignore xfail_checks in check_estimator, with warning MNT Ignore xfail_checks tag in check_estimator, with warning May 15, 2020
@NicolasHug NicolasHug merged commit b0cd595 into scikit-learn:master May 15, 2020
@NicolasHug NicolasHug deleted the xfail_ignore_and_warn_check_estimator branch May 15, 2020 17:30
@NicolasHug
Copy link
Member Author

Merging, thanks for the reviews, I put you both as co-authors

viclafargue pushed a commit to viclafargue/scikit-learn that referenced this pull request Jun 26, 2020
@NicolasHug @thomasjpfan
@rth @viclafargue
MNT Ignore xfail_checks tag in check_estimator, with warning (scikit-…
dac8288 
…learn#17222)

Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
jayzed82 pushed a commit to jayzed82/scikit-learn that referenced this pull request Oct 22, 2020
@NicolasHug @thomasjpfan
@rth @jayzed82
MNT Ignore xfail_checks tag in check_estimator, with warning (scikit-…
7f665bc 
…learn#17222)

Co-authored-by: Thomas J Fan <thomasjpfan@gmail.com>
Co-authored-by: Roman Yurchak <rth.yurchak@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rth rth rth approved these changes

@thomasjpfan thomasjpfan thomasjpfan approved these changes

Assignees
No one assigned
Labels
module:utils
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

xfail_checks tag only works with parametrize_with_checks
3 participants
@NicolasHug @rth @thomasjpfan
0