BUG: fix the error msg of empty hstack input #8801

zixu-zhao · 2017-03-20T20:30:02Z

Raise the meaningful error msg for np.hstack() under empty input.

eric-wieser · 2017-03-20T21:46:16Z

numpy/core/shape_base.py

+
+    if len(arrs) == 0:
+        raise ValueError('need at least one element to concatenate')
+
    # As a special case, dimension 0 of 1-dimensional arrays is "horizontal"
    if arrs[0].ndim == 1:


An alternative fix would be to simply change this line to arrs and arrs[0].ndim == 1, which would then allow concatenate to produce an appropriate error message

Thanks, that's really helpful!

eric-wieser · 2017-03-20T21:46:20Z

numpy/core/shape_base.py

@@ -282,6 +282,10 @@ def hstack(tup):

    """
    arrs = [atleast_1d(_m) for _m in tup]
+
+    if len(arrs) == 0:
+        raise ValueError('need at least one element to concatenate')


The other error message uses array not element.

eric-wieser · 2017-03-20T21:46:20Z

numpy/core/shape_base.py

@@ -282,6 +282,10 @@ def hstack(tup):

    """
    arrs = [atleast_1d(_m) for _m in tup]
+
+    if len(arrs) == 0:


if not arrs would be more pythonic IMO

zixu-zhao · 2017-03-20T21:52:19Z

@eric-wieser I have updated the implementation and squashed the previous commit. Thanks for the help!

MSeifert04 · 2017-03-20T21:52:30Z

I'm not actually opposed but this will break backwards-compatibility. Code that checked for IndexError before needs to check for ValueError now. Not sure if anyone ever used this in a try: ... except IndexError: ... but these will get a nasty surprise.

Maybe safer to go with some sort of FutureWarning first or is it too unlikely?

In any case, shouldn't the change be tested?

eric-wieser · 2017-03-20T22:01:56Z

@MSeifert04: I agree, changing the exception API is perhaps risky. But a futurewarning wouldn't help because:

People couldn't change their code to catch ValueError until the release where things are changed in numpy, so there's no way to prepare
- To fix this, we could raise class FutureValueButCurrentlyIndexError(IndexError, ValueError): pass
Even then, we provide no way for the user to squash the warning, so they have no incentive to try and fix it

From a somewhat mean and not terribly valid point of view, if someone is relying on this behaviour and did not think to report the (relatively obvious) bug here, then perhaps they deserve the breakage?

At any rate, hstack can throw ValueError already for a different set of reasons, so the type of code with a try catch is likely to catch both

Yes, this needs tests. And if they're not there already, vstack and concatenate should test for this too.

eric-wieser · 2017-03-20T22:04:46Z

Also, there's a typo in the commit message

zixu-zhao · 2017-03-20T22:08:36Z

@eric-wieser Fix the typo.Sorry about that.

eric-wieser · 2017-03-20T22:09:22Z

I would have saved that fixup till you add the tests, but whatever

zixu-zhao · 2017-03-20T23:05:33Z

@eric-wieser Hi, I added the test. I haven't done testing addition before, so please let me know if I did something wrong. Thanks!

eric-wieser · 2017-03-20T23:39:38Z

Test looks good - but let's add one for vstack as well, since that also doesn't test this. No need to mention the test in the commit message - it's usually implied.

eric-wieser · 2017-03-20T23:40:11Z

numpy/core/tests/test_shape_base.py

@@ -123,6 +123,9 @@ class TestHstack(TestCase):
    def test_non_iterable(self):
        assert_raises(TypeError, hstack, 1)

+    def test_empty_input(self):
+        assert_raises(ValueError, hstack, [])


Nit-pick: the test for concatenate uses () not []

zixu-zhao · 2017-03-20T23:53:16Z

Updated the commit msg and tests.

eric-wieser · 2017-03-20T23:56:06Z

Looks good to me. Regarding @MSeifert04's comment, at the very least there should be something in the compatibility section of the 1.13 release notes

zixu-zhao · 2017-03-21T00:14:36Z

Updated the 1.13 release notes.

eric-wieser

Generally looks good to me, but someone else should probably weigh in on @MSeifert04's point

eric-wieser · 2017-03-21T21:10:32Z

doc/release/1.13.0-notes.rst

@@ -29,6 +29,9 @@ Future Changes
 Compatibility notes
 ===================

+``numpy.hstack()`` now throws ValueError instead of IndexError when input is empty.
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Bit unusual to have a heading with no content here

I will change it. I didn't realize that is a heading line. I just thought that was a dividing line.

zixu-zhao · 2017-03-21T22:13:57Z

Updated the header and content of 1.13 release notes.

eric-wieser · 2017-03-22T12:19:06Z

doc/release/1.13.0-notes.rst

@@ -29,6 +29,12 @@ Future Changes
 Compatibility notes
 ===================

+
+Error type changes


You've reminded me that I should add another entry under this heading regarding #8584, so thanks for that

My pleasure :)

charris · 2017-03-25T00:00:16Z

In theory, we probably shouldn't be changing error types, in practice it isn't uncomon and doesn't seem to cause problems. I don't really see how we can avoid it if we refactor python code as things are likely to fail in different places. i suppose we could do all checks up front but we generally rely on ducktyping failing one place or another. The errors are rarely part of the documentation...

charris · 2017-03-25T00:04:20Z

Thanks @ZZhaoTireless .

zixu-zhao · 2017-03-25T00:08:16Z

Thanks for reviewing and merging my code :D

eric-wieser reviewed Mar 20, 2017

View reviewed changes

zixu-zhao force-pushed the fix/hstack-error-msg branch from 68603f1 to 7523ac5 Compare March 20, 2017 21:51

zixu-zhao force-pushed the fix/hstack-error-msg branch from 7523ac5 to d5bbd85 Compare March 20, 2017 22:07

zixu-zhao changed the title ~~BUG: fix the error msg of empty hastack input~~ BUG: fix the error msg of empty hstack input Mar 20, 2017

zixu-zhao force-pushed the fix/hstack-error-msg branch from d5bbd85 to 9d225b9 Compare March 20, 2017 23:03

eric-wieser reviewed Mar 20, 2017

View reviewed changes

zixu-zhao force-pushed the fix/hstack-error-msg branch from 9d225b9 to 235ce08 Compare March 20, 2017 23:51

zixu-zhao force-pushed the fix/hstack-error-msg branch from 235ce08 to b1bc302 Compare March 21, 2017 00:12

eric-wieser approved these changes Mar 21, 2017

View reviewed changes

BUG: fix the error msg of empty hstack input

1273caf

zixu-zhao force-pushed the fix/hstack-error-msg branch from b1bc302 to 1273caf Compare March 21, 2017 22:13

eric-wieser reviewed Mar 22, 2017

View reviewed changes

charris added 00 - Bug component: numpy._core labels Mar 25, 2017

charris merged commit a2c4d32 into numpy:master Mar 25, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG: fix the error msg of empty hstack input #8801

BUG: fix the error msg of empty hstack input #8801

zixu-zhao commented Mar 20, 2017

eric-wieser Mar 20, 2017 •

edited

Loading

zixu-zhao Mar 20, 2017

eric-wieser Mar 20, 2017

eric-wieser Mar 20, 2017

zixu-zhao commented Mar 20, 2017 •

edited

Loading

MSeifert04 commented Mar 20, 2017 •

edited

Loading

eric-wieser commented Mar 20, 2017 •

edited

Loading

eric-wieser commented Mar 20, 2017

zixu-zhao commented Mar 20, 2017

eric-wieser commented Mar 20, 2017

zixu-zhao commented Mar 20, 2017

eric-wieser commented Mar 20, 2017 •

edited

Loading

eric-wieser Mar 20, 2017

zixu-zhao commented Mar 20, 2017

eric-wieser commented Mar 20, 2017

zixu-zhao commented Mar 21, 2017

eric-wieser left a comment

eric-wieser Mar 21, 2017

zixu-zhao Mar 21, 2017

zixu-zhao commented Mar 21, 2017

eric-wieser Mar 22, 2017

zixu-zhao Mar 22, 2017

charris commented Mar 25, 2017

charris commented Mar 25, 2017

zixu-zhao commented Mar 25, 2017

BUG: fix the error msg of empty hstack input #8801

BUG: fix the error msg of empty hstack input #8801

Conversation

zixu-zhao commented Mar 20, 2017

eric-wieser Mar 20, 2017 • edited Loading

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

zixu-zhao commented Mar 20, 2017 • edited Loading

MSeifert04 commented Mar 20, 2017 • edited Loading

eric-wieser commented Mar 20, 2017 • edited Loading

eric-wieser commented Mar 20, 2017

zixu-zhao commented Mar 20, 2017

eric-wieser commented Mar 20, 2017

zixu-zhao commented Mar 20, 2017

eric-wieser commented Mar 20, 2017 • edited Loading

Choose a reason for hiding this comment

zixu-zhao commented Mar 20, 2017

eric-wieser commented Mar 20, 2017

zixu-zhao commented Mar 21, 2017

eric-wieser left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zixu-zhao commented Mar 21, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charris commented Mar 25, 2017

charris commented Mar 25, 2017

zixu-zhao commented Mar 25, 2017

eric-wieser Mar 20, 2017 •

edited

Loading

zixu-zhao commented Mar 20, 2017 •

edited

Loading

MSeifert04 commented Mar 20, 2017 •

edited

Loading

eric-wieser commented Mar 20, 2017 •

edited

Loading

eric-wieser commented Mar 20, 2017 •

edited

Loading