[MRG + 1] Remove redundancy in #9552 #9573

liamge · 2017-08-17T00:01:47Z

Reference Issue

What does this implement/fix? Explain your changes.

Fixes the redundancy in #9552 by removing the second array = np.atleast_2d(array) check.

Any other comments?

All check_array tests pass.

jnothman · 2017-08-17T03:49:14Z

======================================================================
ERROR: sklearn.preprocessing.tests.test_data.test_quantile_transform_bounds
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/miniconda/envs/testenv/lib/python2.7/site-packages/nose/case.py", line 197, in runTest
    self.test(*self.arg)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/preprocessing/tests/test_data.py", line 1160, in test_quantile_transform_bounds
    assert_equal(transformer.transform(-10), transformer.transform(np.min(X)))
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/preprocessing/data.py", line 2423, in transform
    X = self._check_inputs(X)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/preprocessing/data.py", line 2348, in _check_inputs
    dtype=[np.float64, np.float32])
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/utils/validation.py", line 425, in check_array
    n_samples = _num_samples(array)
  File "/home/travis/build/scikit-learn/scikit-learn/sklearn/utils/validation.py", line 118, in _num_samples
    " a valid collection." % x)
TypeError: Singleton array array(-10.0) cannot be considered a valid collection.

Perhaps we need to improve the tests for check_array because clearly the behaviour does change with this line.

Though I think the QuantileTrsansformer tests should not be using scalar inputs to transform, @glemaitre!

liamge · 2017-08-17T04:08:02Z

Any advice for how to build new tests for this? It seems as though the current tests for check_array only raise a ValueError when given a 1d array and not a scalar. Should check_array return a 2d array when given a scalar input?

jnothman · 2017-08-17T04:50:26Z

no, I think we should get an error on scalar input. but apparently we have tests that do otherwise. it's fine if you fix the QuantileTransformer tests in the same PR too, IMO.

…

On 17 Aug 2017 2:08 pm, "Liam Geron" ***@***.***> wrote: Any advice for how to build new tests for this? It seems as though the current tests for check_array only raise a ValueError when given a 1d array and not a scalar. Should check_array return a 2d array when given a scalar input? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#9573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz62OT3rVDV-n20u7MkCtzTtwMwsUhks5sY7ykgaJpZM4O5n8v> .

liamge · 2017-08-17T05:12:58Z

Ok I'll take a look at the QuantileTransformer tests as well, as far as they go how would you suggest I test for the upper/lower bounds without the use of a scalar? Thanks for all the help by the way!

glemaitre · 2017-08-17T08:43:59Z

Though I think the QuantileTrsansformer tests should not be using scalar inputs to transform, @glemaitre!

I take the blame. It should be a 2d array. @jnothman.
Would it make sense to have a common test checking that an error is raised when passing a scalar?

jnothman · 2017-08-17T10:39:40Z

Yes, probably that to

…

On 17 August 2017 at 18:44, Guillaume Lemaitre ***@***.***> wrote: Though I think the QuantileTrsansformer tests should not be using scalar inputs to transform, @glemaitre <https://github.com/glemaitre>! I take the blame. It should be a 2d array. @jnothman <https://github.com/jnothman>. Would it make sense to have a common test checking that an error is raised when passing a scalar? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#9573 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAEz6xiMCfmHyytR2ObCD89C8Fv_Qtdkks5sY_1SgaJpZM4O5n8v> .

jnothman · 2017-08-17T10:39:46Z

too

…

On 17 August 2017 at 20:39, Joel Nothman ***@***.***> wrote: Yes, probably that to On 17 August 2017 at 18:44, Guillaume Lemaitre ***@***.***> wrote: > Though I think the QuantileTrsansformer tests should not be using scalar > inputs to transform, @glemaitre <https://github.com/glemaitre>! > > I take the blame. It should be a 2d array. @jnothman > <https://github.com/jnothman>. > Would it make sense to have a common test checking that an error is > raised when passing a scalar? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#9573 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAEz6xiMCfmHyytR2ObCD89C8Fv_Qtdkks5sY_1SgaJpZM4O5n8v> > . >

liamge · 2017-08-17T17:17:16Z

Ok so I'll change the input to 2D arrays in the quantile tests and write an additional test in test_quantile_transform_check_error to make sure it raises an error on 1D input. Does that work?

lesteve · 2017-09-01T09:38:00Z

sklearn/utils/validation.py

+            if array.ndim == 0:
+                raise ValueError(
+                    "Expected 2D array, got scalar array instead:\narray={}.\n"
+                    "Reshape your data either using array.reshape(-1, 1) if "


Does this apply to a scalar array?

I would say the advice does not apply to scalar arrays.

@liamge do you intend to address this?

Can reshape(1,1)

np.array(10).reshape(-1, 1) actually works (np 1.13.1).

Maybe you can rewrite the code to avoid the copy and paste look and feel:

if array.ndim < 2: array_type = 'scalar' if array.ndim == 0 else '1D' raise ValueError('...got {} array instead ... array={} ...'.format(array_type, array))

Very sorry about the radio silence, would you want me to add an additional commit with the avoidance of the copy/paste look?

Well @amueller reckons this works, do let's just key it.

jnothman

Apart from @lesteve’s comment this LGTM

jnothman · 2017-10-26T07:06:56Z

sklearn/utils/validation.py

+            if array.ndim == 0:
+                raise ValueError(
+                    "Expected 2D array, got scalar array instead:\narray={}.\n"
+                    "Reshape your data either using array.reshape(-1, 1) if "


Can reshape(1,1)

jnothman · 2017-10-26T07:07:25Z

sklearn/utils/validation.py

+                    "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.".format(array))
+            # If input is 1D raise error
            if array.ndim == 1:


Might as well use elif

amueller

LGTM

amueller · 2017-10-26T16:36:35Z

sklearn/utils/validation.py

+            if array.ndim == 0:
+                raise ValueError(
+                    "Expected 2D array, got scalar array instead:\narray={}.\n"
+                    "Reshape your data either using array.reshape(-1, 1) if "


np.array(10).reshape(-1, 1) actually works (np 1.13.1).

jnothman · 2017-10-26T19:57:50Z

Thanks @liamge!

liamge · 2017-10-26T21:45:50Z

No problem! Glad to help where I can

lesteve · 2017-10-27T03:07:21Z

sklearn/utils/validation.py

@@ -402,13 +402,20 @@ def check_array(array, accept_sparse=False, dtype="numeric", order=None,
        array = np.array(array, dtype=dtype, order=order, copy=copy)

        if ensure_2d:
+            # If input is scalar raise error
+            if array.ndim == 0:


Actually I think the error for scalar array should be the same irrespective of ensure_2d, i.e. it should be taken out of the if ensure_2d clause.

The error message when ensure_2d==False is not so great:

from sklearn.utils import check_array check_array(10, ensure_2d=False)

--------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-3-ce2f2639811f> in <module>() ----> 1 check_array(10, ensure_2d=False) ~/dev/scikit-learn/sklearn/utils/validation.py in check_array(array, accept_sparse, dtype, order, copy, force_all_finite, ensure_2d, allow_nd, ensure_min_samples, ensure_min_features, warn_on_dtype, estimator) 488 shape_repr = _shape_repr(array.shape) 489 if ensure_min_samples > 0: --> 490 n_samples = _num_samples(array) 491 if n_samples < ensure_min_samples: 492 raise ValueError("Found array with %d sample(s) (shape=%s) while a" ~/dev/scikit-learn/sklearn/utils/validation.py in _num_samples(x) 118 if len(x.shape) == 0: 119 raise TypeError("Singleton array %r cannot be considered" --> 120 " a valid collection." % x) 121 return x.shape[0] 122 else: TypeError: Singleton array array(10) cannot be considered a valid collection.

…cs/donigian-update-contribution-guidelines * 'master' of github.com:scikit-learn/scikit-learn: (23 commits) fixes scikit-learn#10031: fix attribute name and shape in documentation (scikit-learn#10033) [MRG+1] add changelog entry for fixed and merged PR scikit-learn#10005 issue scikit-learn#9633 (scikit-learn#10025) [MRG] Fix LogisticRegression see also should include LogisticRegressionCV(scikit-learn#9995) (scikit-learn#10022) [MRG + 1] Labels of clustering should start at 0 or -1 if noise (scikit-learn#10015) MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573) [MRG+1] correct comparison in GaussianNB for 'priors' (scikit-learn#10005) [MRG + 1] ENH add check_inverse in FunctionTransformer (scikit-learn#9399) [MRG] FIX bug in nested set_params usage (scikit-learn#9999) [MRG+1] Fix LOF and Isolation benchmarks (scikit-learn#9798) [MRG + 1] Fix negative inputs checking in mean_squared_log_error (scikit-learn#9968) DOC Fix typo (scikit-learn#9996) DOC Fix typo: x axis -> y axis (scikit-learn#9985) improve example plot_forest_iris.py (scikit-learn#9989) [MRG+1] Deprecate pooling_func unused parameter in AgglomerativeClustering (scikit-learn#9875) DOC update news DOC Fix three typos in manifold documentation (scikit-learn#9990) DOC add missing dot in docstring DOC Add what's new for 0.19.1 (scikit-learn#9983) Improve readability of outlier detection example. (scikit-learn#9973) DOC: Fixed typo (scikit-learn#9977) ...

Remove redundancy in #9552

048f433

liamge added 3 commits August 17, 2017 13:30

Fixes errors caused by #9573 and redundancy in #9552

c5faea5

Fix line length from #9573

d4f608b

Fixes line length problems

814b002

lesteve reviewed Sep 1, 2017

View reviewed changes

gxyd mentioned this pull request Oct 26, 2017

redundant code in check_array #9552

Closed

jnothman approved these changes Oct 26, 2017

View reviewed changes

amueller approved these changes Oct 26, 2017

View reviewed changes

amueller changed the title ~~Remove redundancy in #9552~~ [MRG + 1] Remove redundancy in #9552 Oct 26, 2017

jnothman merged commit 202b532 into scikit-learn:master Oct 26, 2017

liamge deleted the bug/9552 branch October 26, 2017 21:45

lesteve reviewed Oct 27, 2017

View reviewed changes

maskani-moh pushed a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573)

95bd5a6

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

MAINT Remove redundancy in scikit-learn#9552 (scikit-learn#9573)

2995bac

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG + 1] Remove redundancy in #9552 #9573

[MRG + 1] Remove redundancy in #9552 #9573

liamge commented Aug 17, 2017 •

edited by lesteve

Loading

jnothman commented Aug 17, 2017

liamge commented Aug 17, 2017

jnothman commented Aug 17, 2017 via email

liamge commented Aug 17, 2017

glemaitre commented Aug 17, 2017

jnothman commented Aug 17, 2017 via email

jnothman commented Aug 17, 2017 via email

liamge commented Aug 17, 2017

lesteve Sep 1, 2017

lesteve Sep 1, 2017

jnothman Oct 26, 2017

jnothman Oct 26, 2017

amueller Oct 26, 2017

lesteve Oct 26, 2017

liamge Oct 26, 2017

jnothman Oct 26, 2017

jnothman left a comment

jnothman Oct 26, 2017

jnothman Oct 26, 2017

amueller left a comment

amueller Oct 26, 2017

jnothman commented Oct 26, 2017

liamge commented Oct 26, 2017

lesteve Oct 27, 2017

[MRG + 1] Remove redundancy in #9552 #9573

[MRG + 1] Remove redundancy in #9552 #9573

Conversation

liamge commented Aug 17, 2017 • edited by lesteve Loading

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

jnothman commented Aug 17, 2017

liamge commented Aug 17, 2017

jnothman commented Aug 17, 2017 via email

liamge commented Aug 17, 2017

glemaitre commented Aug 17, 2017

jnothman commented Aug 17, 2017 via email

jnothman commented Aug 17, 2017 via email

liamge commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnothman commented Oct 26, 2017

liamge commented Oct 26, 2017

Choose a reason for hiding this comment

liamge commented Aug 17, 2017 •

edited by lesteve

Loading