[MRG] Added variable_name as a parameter for check_array by dalmia · Pull Request #8178 · scikit-learn/scikit-learn

dalmia · 2017-01-09T14:22:30Z

What does this implement/fix? Explain your changes.

This adds variable_name as a parameter for check_array to be used as a placeholder for error messages. Related files using check_array on instances passed as a parameter have also been updated.

Any other comments?

Discussed in #7996

jnothman

Where variables are the result of computation, check_array is used to recast into a particular dtype or order, and/or to ensure finiteness. Perhaps having a variable name in these cases is a distraction in the code, but I'm okay with it.

Note that the alternative is to remove X from error messages (or make the fact that it's merely a placeholder clearer), and expect the user to read the traceback carefully. I personally think this is the right way to go, personally.

Arguably the same issue applies to column_or_1d.

jnothman · 2017-01-10T03:52:43Z

sklearn/covariance/empirical_covariance_.py


        """
-        covariance = check_array(covariance)
+        covariance = check_array(covariance, variable_name='covariance')


This value is not provided by the user, and it's a bit of a mystery to me that check_array is here at all

Removing the variable_name argument from here for now.

jnothman · 2017-01-10T04:30:11Z

sklearn/decomposition/nmf.py


 def _check_init(A, shape, whom):
-    A = check_array(A)
+    A = check_array(A, variable_name='A')


the variable name is actually included in whom (see calls to the function, below). Replace whom with variable_name.

jnothman · 2017-01-10T04:33:56Z

sklearn/linear_model/coordinate_descent.py

            _, _, X_offset, _, X_scale = _preprocess_data(X, y, fit_intercept,
-                                                      normalize,
-                                                      return_mean=True)
+                                                          normalize,


We usually avoid fixing incidental things. It makes reviewing PRs harder.

jnothman · 2017-01-10T04:34:57Z

sklearn/linear_model/coordinate_descent.py

            if check_input:
                precompute = check_array(precompute, dtype=X.dtype.type,
-                                         order='C')
+                                         order='C', variable_name='precompute')


precompute is not user-provided, but this is okay.

jnothman · 2017-01-10T04:39:44Z

sklearn/mixture/gaussian_mixture.py

    """
    weights = check_array(weights, dtype=[np.float64, np.float32],
-                          ensure_2d=False)
+                          ensure_2d=False, variable_name='weights')


The user sees this variable as "weights_init"

jnothman · 2017-01-10T04:39:52Z

sklearn/mixture/gaussian_mixture.py

    """
-    means = check_array(means, dtype=[np.float64, np.float32], ensure_2d=False)
+    means = check_array(means, dtype=[np.float64, np.float32], ensure_2d=False,
+                        variable_name='means')


jnothman · 2017-01-10T04:40:00Z

sklearn/mixture/gaussian_mixture.py

                             ensure_2d=False,
-                             allow_nd=covariance_type == 'full')
+                             allow_nd=covariance_type == 'full',
+                             variable_name='precisions')


precisions_init

dalmia · 2017-01-10T17:18:11Z

I agree on making the error message independent of the variable_name. Also, better for future usages of check_array. I thought maybe something like this for check_array:

Got 1-dimensional input. Reshape your data either using .reshape(-1, 1)
if your data has a single feature or .reshape(1, -1) if it contains a single sample.

And maybe for column_or_1d:

A column-vector was passed when a 1d array was expected. Please 
change the shape of your data to (n_samples, ), for example using ravel().

dalmia · 2017-01-12T13:39:15Z

Should we go ahead with this? @jnothman

dalmia · 2017-02-06T21:09:19Z

ping @jnothman

dalmia · 2017-02-18T07:16:34Z

sklearn/decomposition/dict_learning.py


    dictionary = check_array(dictionary.T, order='F', dtype=np.float64,
-                             copy=False)
+                             copy=False, variable_name='dictionary')


Remove variable_name here as it's not seen by the user.

dalmia · 2017-02-18T07:37:35Z

sklearn/metrics/classification.py

    # Check if dimensions are consistent.
-    transformed_labels = check_array(transformed_labels)
+    transformed_labels = check_array(transformed_labels,
+                                     variable_name='transformed_labels')


The user can't see this.

dalmia · 2017-02-18T07:46:34Z

sklearn/utils/multiclass.py

    if (label_type == "multilabel-indicator" and
-            len(set(check_array(y, ['csr', 'csc', 'coo']).shape[1]
+            len(set(check_array(y, ['csr', 'csc', 'coo'],
+                                variable_name='y').shape[1]


The user doesn't see it as y.

dalmia · 2017-02-18T08:57:39Z

I think that we shouldn't leave the responsibility of figuring out what the error is, to the user and that trying to come up with a general description of the error might not be the right thing to do. On the other hand, variable_name helps to identify the error faster. Not adding variable_name to any function call doesn't cause any harm in any case, but can be used in situations where the name in the error message might be confusing (#7996). Also, as you mentioned, I did the same for column_or_1d as the same argument holds.

codecov · 2017-02-18T10:11:20Z

Codecov Report

Merging #8178 into master will not change coverage.
The diff coverage is 98.95%.

@@           Coverage Diff           @@
##           master    #8178   +/-   ##
=======================================
  Coverage   94.75%   94.75%           
=======================================
  Files         342      342           
  Lines       60813    60813           
=======================================
  Hits        57621    57621           
  Misses       3192     3192

Impacted Files	Coverage Δ
sklearn/metrics/cluster/supervised.py	`99.23% <ø> (ø)`	✅
sklearn/utils/mocking.py	`100% <ø> (ø)`	✅
sklearn/metrics/pairwise.py	`97.21% <ø> (ø)`	✅
sklearn/linear_model/sag.py	`93.84% <ø> (ø)`	✅
sklearn/metrics/ranking.py	`98.75% <100%> (ø)`	✅
sklearn/neural_network/multilayer_perceptron.py	`97.81% <100%> (ø)`	✅
sklearn/decomposition/dict_learning.py	`93.45% <100%> (ø)`	✅
sklearn/linear_model/ridge.py	`93.88% <100%> (ø)`	✅
sklearn/dummy.py	`98.24% <100%> (ø)`	✅
sklearn/base.py	`94.15% <100%> (ø)`	✅
... and 34 more

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 f952e43...57bf914. Read the comment docs.

jnothman · 2017-06-08T05:11:11Z

Thanks for all the work, though I think the proposal in #8178 (comment) that we avoid mentioning the variable by name in error messages might be the better way to go.

jnothman · 2017-06-14T13:55:46Z

Approach of #8178 (comment) adopted in #9068

jnothman reviewed Jan 10, 2017

View reviewed changes

dalmia commented Feb 18, 2017

View reviewed changes

dalmia added 10 commits February 18, 2017 14:17

ENH: added variable_name to check_array

3c7d0d8

ENH: added variable_name as argument

378d0c7

ENH: added variable_name as argument for linear_model

9421307

ENH: added variable_name as argument for metrics

fe610c9

ENH: added variable_name as argument for remaining files

2bbf10d

FIX: fix typo in check_array

21c582b

ENH: added variable_name for instances passed as parameters

9cad6b3

FIX: remove flake8 errors

0cdf3c1

FIX: corrected variable names as per what user sees

5cd932b

FEAT: added variable_name to column_or_1d

57bf914

dalmia force-pushed the add_variable_name branch from a36c57b to 57bf914 Compare February 18, 2017 08:52

jnothman mentioned this pull request Jun 8, 2017

Make error messages in check_array and similar not mention variable name #9047

Closed

lesteve closed this Jun 14, 2017

Uh oh!

Conversation

dalmia commented Jan 9, 2017

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jnothman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalmia Feb 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalmia commented Jan 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dalmia commented Jan 12, 2017

Uh oh!

dalmia commented Feb 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dalmia commented Feb 18, 2017

Uh oh!

codecov bot commented Feb 18, 2017

Codecov Report

Uh oh!

jnothman commented Jun 8, 2017

Uh oh!

jnothman commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dalmia Feb 18, 2017 •

edited

Loading

dalmia commented Jan 10, 2017 •

edited

Loading