-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
MNT simplify xfail check marking logic #16949
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
Conversation
NicolasHug
commented
Apr 17, 2020
- renamed xfail_test tag into xfail_checks because that's the variable name
- simplified and added comments to the marking logic
ping @rth @thomasjpfan |
I also removed the description of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NicolasHug ! Two minor comments otherwise LGTM.
Just to make sure I understand correctly, this is pure refactoring? It does not address issues for #16890 right?
sklearn/base.py
Outdated
@@ -33,7 +33,7 @@ | |||
'stateless': False, | |||
'multilabel': False, | |||
'_skip_test': False, | |||
'_xfail_test': False, | |||
'_xfail_checks': {}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm weary of having mutable object in this default dict. You never know what downstream object can do with it. I think it should not be mutable, hence the False. We also say somewhere in the doc that all estimator tags with a few exceptions default to False.
Unless this is solving some issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it makes the code cleaner and easier to understand because we can just do if check_name not in xfail_checks:
instead of checking if it's a bool first.
I share the concern about not using mutable globals, but right above there is the 'X_types': ['2darray']
tag so I thought we could just do it here as well
Alternatively I can update _safe_tags()
to return {}
when the tag is False?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I share the concern about not using mutable globals, but right above there is the 'X_types': ['2darray'] tag so I thought we could just do it here as well
Indeed, but it one puts wrong value there either all tests are going to be skipped or some would fail. Here it would potentially skip one test which is harder to notice.
Alternatively I can update _safe_tags() to return {} when the tag is False?
+1 for this particular tag.
Co-Authored-By: Roman Yurchak <rth.yurchak@gmail.com>
… into cln_xfail_checks
|
||
xfail_checks = _safe_tags(estimator, '_xfail_checks') or {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @NicolasHug !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Co-Authored-By: Roman Yurchak <rth.yurchak@gmail.com>
Co-Authored-By: Roman Yurchak <rth.yurchak@gmail.com>