Skip to content

FIX wrong usage and occurrence of string tag #14043

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 4 commits into from
Jun 13, 2019
Merged

FIX wrong usage and occurrence of string tag #14043

merged 4 commits into from
Jun 13, 2019

Conversation

glemaitre
Copy link
Member

The right tag to use should be string and not str. However, there are 2 occurrences within the code base.

I am unsure if we should backport this in 0.21.3

rth
rth approved these changes Jun 8, 2019
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.

Thanks for catching this!

@rth rth added this to the 0.21.3 milestone Jun 8, 2019
@jnothman
Copy link
Member

.... can we add some tag validation in check_estimator???

@rth
Copy link
Member

rth commented Jun 11, 2019

.... can we add some tag validation in check_estimator???

I thought about that, but then it means that if some contrib packages add their own tags check_estimator would fail (at least until #13969 is implemented).

@thomasjpfan
Copy link
Member

In check_estimators, we can add an optional keyword valid_tags, which check_estimators can use to validate tags.

@amueller
Copy link
Member

What we actually should be doing is using these tags to generate data and run the tests with this generated data.

@qinhanmin2014
Copy link
Member

I thought about that, but then it means that if some contrib packages add their own tags check_estimator would fail (at least until #13969 is implemented).

+1

qinhanmin2014
qinhanmin2014 approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@qinhanmin2014 qinhanmin2014 left a comment

Choose a reason for hiding this comment

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

let's merge this one first?

sklearn/utils/estimator_checks.py Outdated
@@ -663,7 +663,8 @@ def check_dtype_object(name, estimator_orig):
if "Unknown label type" not in str(e):
raise

if 'str' not in tags['X_types']:
tags = _safe_tags(estimator)
Copy link
Member

Choose a reason for hiding this comment

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

this seems redundant?

@qinhanmin2014
Copy link
Member

We already have tags = _safe_tags(estimator_orig) above?

@codecov
Copy link

codecov bot commented Jun 12, 2019 •
edited
Loading

Codecov Report

Merging #14043 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #14043   +/-   ##
=======================================
  Coverage   96.81%   96.81%           
=======================================
  Files         393      393           
  Lines       71911    71911           
  Branches     7887     7887           
=======================================
  Hits        69623    69623           
  Misses       2265     2265           
  Partials       23       23
Impacted Files Coverage Δ
sklearn/impute/_base.py 98.32% <ø> (ø) ⬆️
sklearn/utils/estimator_checks.py 94.02% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 227ebc4...c74e110. Read the comment docs.

@glemaitre
Copy link
Member Author

We already have tags = _safe_tags(estimator_orig) above?

Oh I see. Sorry about that. I did not see it.

@qinhanmin2014 qinhanmin2014 merged commit 8fe89ea into scikit-learn:master Jun 13, 2019
@qinhanmin2014 qinhanmin2014 mentioned this pull request Jun 13, 2019
Open
jnothman pushed a commit to jnothman/scikit-learn that referenced this pull request Jun 24, 2019
@glemaitre @jnothman
FIX wrong usage and occurrence of string tag (scikit-learn#14043)
7e874b4
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
@glemaitre @koenvandevelde
FIX wrong usage and occurrence of string tag (scikit-learn#14043)
93ebfe3
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

@rth rth rth approved these changes

@qinhanmin2014 qinhanmin2014 qinhanmin2014 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.21.3
Development

Successfully merging this pull request may close these issues.
6 participants
@glemaitre @jnothman @rth @thomasjpfan @amueller @qinhanmin2014