BUG: fix the error msg of empty hstack input by zixu-zhao · Pull Request #8801 · numpy/numpy

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

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

The other error message uses array not element.

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

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

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

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

 ===================

+
+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

Comment thread numpy/core/tests/test_shape_base.py Outdated

eric-wieser Mar 20, 2017

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.

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

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

Conversation

zixu-zhao commented Mar 20, 2017

Uh oh!

eric-wieser Mar 20, 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!

zixu-zhao commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MSeifert04 commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eric-wieser commented Mar 20, 2017

Uh oh!

zixu-zhao commented Mar 20, 2017

Uh oh!

eric-wieser commented Mar 20, 2017

Uh oh!

zixu-zhao commented Mar 20, 2017

Uh oh!

eric-wieser commented Mar 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zixu-zhao commented Mar 20, 2017

Uh oh!

eric-wieser commented Mar 20, 2017

Uh oh!

zixu-zhao commented Mar 21, 2017

Uh oh!

eric-wieser left a comment

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!

zixu-zhao commented Mar 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charris commented Mar 25, 2017

Uh oh!

charris commented Mar 25, 2017

Uh oh!

zixu-zhao commented Mar 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

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