Skip to content

Updated error message to remove deprecated n_values#13454

Merged
jnothman merged 5 commits intoscikit-learn:masterfrom
maftabali:updateerrormsg
Mar 21, 2019
Merged

Updated error message to remove deprecated n_values#13454
jnothman merged 5 commits intoscikit-learn:masterfrom
maftabali:updateerrormsg

Conversation

@maftabali
Copy link
Copy Markdown
Contributor

@maftabali maftabali commented Mar 16, 2019

Reference Issues/PRs

Fixes #13446

What does this implement/fix? Explain your changes.

Updated error message to use categories in place of the deprecated n_values

Any other comments?

if self._categories != 'auto':
if len(self._categories) != n_features:
raise ValueError("Shape mismatch: if n_values is an array,"
raise ValueError("Shape mismatch: if categories is an array,"
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.

seems like there's no test for this one, could you please add a test for it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@adrinjalali Added. Please check.

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.

pep8 issues.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

could you please clarify? are you referring to the same point as made by jnothman or anything specific like naming conventions on the test..

% type(self._n_values))
if n_values.ndim < 1 or n_values.shape[0] != X.shape[1]:
raise ValueError("Shape mismatch: if n_values is an array,"
raise ValueError("Shape mismatch: if categories is an array,"
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.

I don't think the change is applicable here. This condition applies only when the user is working with the deprecated parameter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

will revert the change for the _legacy_fit_transform.

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Mar 19, 2019 via email

@maftabali
Copy link
Copy Markdown
Contributor Author

See log at https://circleci.com/gh/scikit-learn/scikit-learn/51959 for pep8 errors

Thanks. Will make the changes and resubmit tonight.

@maftabali
Copy link
Copy Markdown
Contributor Author

Fixed pep8 issues.

cats = ['Low', 'Medium', 'High']
enc = OrdinalEncoder(categories=cats)
msg = ("Shape mismatch: if categories is an array,")
# "Call 'fit' with appropriate arguments before using this method.")
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.

What is the reason for this comment? (I don't really understand it)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

My bad.. that was unrelated. Have removed it now.

@jorisvandenbossche
Copy link
Copy Markdown
Member

@maftabali you still have one remaining linting error:

sklearn/preprocessing/tests/test_encoders.py:702:1: W293 blank line contains whitespace

For the rest looks good to me!

@maftabali
Copy link
Copy Markdown
Contributor Author

@maftabali you still have one remaining linting error:

sklearn/preprocessing/tests/test_encoders.py:702:1: W293 blank line contains whitespace

For the rest looks good to me!

All done @jorisvandenbossche

@jnothman jnothman merged commit c3731f5 into scikit-learn:master Mar 21, 2019
@jnothman
Copy link
Copy Markdown
Member

Thanks @maftabali

@maftabali
Copy link
Copy Markdown
Contributor Author

Thanks @maftabali

Pleasure @jnothman

xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
xhluca pushed a commit to xhluca/scikit-learn that referenced this pull request Apr 28, 2019
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Confusing error message in OrdinalEncoder when passing single list of categories

4 participants