Skip to content

MNT Ignore xfail_checks tag in check_estimator, with warning#17222

Merged
NicolasHug merged 8 commits intoscikit-learn:masterfrom
NicolasHug:xfail_ignore_and_warn_check_estimator
May 15, 2020
Merged

MNT Ignore xfail_checks tag in check_estimator, with warning#17222
NicolasHug merged 8 commits intoscikit-learn:masterfrom
NicolasHug:xfail_ignore_and_warn_check_estimator

Conversation

@NicolasHug
Copy link
Copy Markdown
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 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
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.

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

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
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generators everywhere!

Copy link
Copy Markdown
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
Copy Markdown
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
…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
…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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

xfail_checks tag only works with parametrize_with_checks

3 participants