Skip to content

[MRG+2] check_array Error message#9068

Merged
jnothman merged 4 commits intoscikit-learn:masterfrom
massich:is/9047
Jun 14, 2017
Merged

[MRG+2] check_array Error message#9068
jnothman merged 4 commits intoscikit-learn:masterfrom
massich:is/9047

Conversation

@massich
Copy link
Copy Markdown
Contributor

@massich massich commented Jun 8, 2017

Reference Issue

Fixes #9047

What does this implement/fix? Explain your changes.

Any other comments?

@massich
Copy link
Copy Markdown
Contributor Author

massich commented Jun 8, 2017

@jnothman is this what you had in mind?

RFC before propagating the changes to the tests

@massich massich changed the title [WIP] check_array Error message [WIP][RFC] check_array Error message Jun 8, 2017
jnothman
jnothman reviewed Jun 8, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

I suppose that's right

@GaelVaroquaux GaelVaroquaux changed the title [WIP][RFC] check_array Error message [MRG+1][RFC] check_array Error message Jun 9, 2017
@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jun 9, 2017

LGTM. Thanks

@GaelVaroquaux
Copy link
Copy Markdown
Member

GaelVaroquaux commented Jun 9, 2017

Actually: not LGTM: you need to change all the error messages in the tests. The tests currently fail

@massich
Copy link
Copy Markdown
Contributor Author

massich commented Jun 9, 2017 •
edited
Loading

@GaelVaroquaux, yep I wanted confirmation before changing ALL the testing. I let this for the monkey time, which I think it would come earlier than I would like.

@vene vene mentioned this pull request Jun 9, 2017
Merged
jorisvandenbossche
jorisvandenbossche reviewed Jun 9, 2017
View reviewed changes
sklearn/utils/validation.py Outdated
"Got X with X.ndim=1. Reshape your data either using "
"X.reshape(-1, 1) if your data has a single feature or "
"X.reshape(1, -1) if it contains a single sample.")
"Expected 2D array, got 1D array instead: array={}. "
Copy link
Copy Markdown
Member

@jorisvandenbossche jorisvandenbossche Jun 9, 2017

Choose a reason for hiding this comment

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

I would put a newline after the array printing

Copy link
Copy Markdown
Member

@lesteve lesteve Jun 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Even maybe "got 1D array instead:\n{}\n". Otherwise the first row of the array will not be vertically aligned with the other rows

@massich massich changed the title [MRG+1][RFC] check_array Error message [MRG+1] check_array Error message Jun 14, 2017
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jun 14, 2017

flake8 error:


--------------------------------------------------------------------------------
./sklearn/utils/validation.py:409:80: E501 line too long (80 > 79 characters)
                    "if your data has a single feature or array.reshape(1, -1) "

@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jun 14, 2017

LGTM

@jnothman jnothman changed the title [MRG+1] check_array Error message [MRG+2] check_array Error message Jun 14, 2017
@jnothman jnothman added this to the 0.19 milestone Jun 14, 2017
@massich
Copy link
Copy Markdown
Contributor Author

massich commented Jun 14, 2017

ping @jnothman

@jnothman jnothman merged commit 90cfb48 into scikit-learn:master Jun 14, 2017
@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jun 14, 2017

a few seconds too late to comment on this one ...

This was referenced Jun 14, 2017
Closed
Closed
@jnothman
Copy link
Copy Markdown
Member

jnothman commented Jun 14, 2017

Sorry, was there a comment to be heard?

@lesteve
Copy link
Copy Markdown
Member

lesteve commented Jun 14, 2017 •
edited
Loading 

import numpy as np
from sklearn.utils.validation import check_array
check_array(np.array([1., 2., 3.]), ensure_2d=True)
ValueError: Expected 2D array, got 1D array instead: 
array=[ 1.  2.  3.]
 Reshape your data either using array.reshape(-1, 1) if your data has a single feature or array.reshape(1, -1) if it contains a single sample.

The message would be better formatted like this:

ValueError: Expected 2D array, got 1D array instead: array=[ 1.  2.  3.].
Reshape your data either using array.reshape(-1, 1) if your data has a single feature or array.reshape(1, -1) if it contains a single sample.

and it would be nice to add a test in sklearn/utils/tests/test_validation.py.

@massich
Copy link
Copy Markdown
Contributor Author

massich commented Jun 14, 2017 •
edited
Loading

I've added @lesteve comments. How should I reopen the PR?

@jorisvandenbossche
Copy link
Copy Markdown
Member

jorisvandenbossche commented Jun 14, 2017

You need to open a new PR

@massich massich mentioned this pull request Jun 14, 2017
Merged
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@massich @dmohns
[MRG+2] Make check_array Error message independent of variable name (…
2dc98d8 
…scikit-learn#9068)
dmohns pushed a commit to dmohns/scikit-learn that referenced this pull request Aug 7, 2017
@massich @dmohns
[MRG+2] Make check_array Error message independent of variable name (…
25b4428 
…scikit-learn#9068)
NelleV pushed a commit to NelleV/scikit-learn that referenced this pull request Aug 11, 2017
@massich @NelleV
[MRG+2] Make check_array Error message independent of variable name (…
678d302 
…scikit-learn#9068)
paulha pushed a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017
@massich @paulha
[MRG+2] Make check_array Error message independent of variable name (…
9c4d58c 
…scikit-learn#9068)
AishwaryaRK pushed a commit to AishwaryaRK/scikit-learn that referenced this pull request Aug 29, 2017
@massich @AishwaryaRK
[MRG+2] Make check_array Error message independent of variable name (…
086c648 
…scikit-learn#9068)
maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017
@massich @maskani-moh
[MRG+2] Make check_array Error message independent of variable name (…
52e1036 
…scikit-learn#9068)
jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017
@massich
[MRG+2] Make check_array Error message independent of variable name (…
7bd2369 
…scikit-learn#9068)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman left review comments

@lesteve lesteve lesteve left review comments

+1 more reviewer

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.19

Development

Successfully merging this pull request may close these issues.

Make error messages in check_array and similar not mention variable name

5 participants

@massich @GaelVaroquaux @jnothman @lesteve @jorisvandenbossche