Skip to content

TST, MAINT: Make sure exceptions of inner and dot match for different cases #6987

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 15, 2016
Merged

TST, MAINT: Make sure exceptions of inner and dot match for different cases #6987

merged 2 commits into from
Jan 15, 2016

Conversation

jakirkham
Copy link
Contributor

Related: #6988

Fixes an issue with dot where it was ValueError or a TypeError depending on the order of its arguments. This differs from inner, which always raises a TypeError. Below is an example of the current behavior, which seems odd. This PR makes all cases raise a TypeError.

>>> import numpy
>>> import numpy as np
>>> np.inner(1., np.array((1,1), dtype='i,i'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: invalid type promotion
>>> np.inner(np.array((1,1), dtype='i,i'), 1.)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Cannot cast array data from dtype([('f0', '<i4'), ('f1', '<i4')]) to dtype('float64') according to the rule 'safe'
>>> np.dot(1., np.array((1,1), dtype='i,i'))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: Cannot find a common data type.
>>> np.dot(np.array((1,1), dtype='i,i'), 1.)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: Cannot cast array data from dtype([('f0', '<i4'), ('f1', '<i4')]) to dtype('float64') according to the rule 'safe'

@jakirkham jakirkham changed the title TST, MAINT: Test and fix up dot and inner type failures WIP, TST, MAINT: Test and fix up dot and inner type failures Jan 9, 2016
@jakirkham jakirkham changed the title WIP, TST, MAINT: Test and fix up dot and inner type failures TST, MAINT: Test and fix up dot and inner type failures Jan 9, 2016
@jakirkham jakirkham mentioned this pull request Jan 9, 2016
Merged
@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch from 8996992 to 724c915 Compare January 9, 2016 21:55
charris
charris reviewed Jan 9, 2016
View reviewed changes
numpy/core/tests/test_multiarray.py
c = 1.
A = np.array((1,1), dtype='i,i')

try:
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to use assert_raises here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None, I wasn't sure the right way to do this in numpy's test suite and took test_mul as an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would you prefer it changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was changed to assert_raises.

@charris
Copy link
Member

charris commented Jan 9, 2016

IIRC, there was a suggestion that TypeError would be correct in both cases. Not sure what our policy on changing error types is, we probably shouldn't, but we do on occasion...

@jakirkham
Copy link
Contributor Author

I feel like one of these is a bug, but I am just not sure which one. I agree that TypeError sounds reasonable. The only benefit to keeping one as ValueError would be to handle the too cases separately, which I am not sure is so valuable.

@jakirkham
Copy link
Contributor Author

FWIW, if we decided changing the exception is just not ok, it would be trivial to preserve the current behavior in PR ( #6968 ), but I just wanted to raise this oddity separately so it could be discussed more directly.

@jakirkham
Copy link
Contributor Author

Another nice consequence of changing everything to TypeError is if someone was already using dot like so...

try:
    np.dot(a, b)
except TypeError, ValueError:
    pass

It will still work.

@jakirkham
Copy link
Contributor Author

Interesting, seeing a failure on USE_DEBUG=1 build. Not sure if this is related ( http://bugs.python.org/issue20500 ). Maybe related to this ( #6741 ).

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch 3 times, most recently from e94fbb3 to 9fb0d0c Compare January 9, 2016 23:29
@charris
Copy link
Member

charris commented Jan 10, 2016

I can reproduce this here with 3.4.4. A bit more info is in the comment in the cpython code.

#ifdef Py_DEBUG
    /* PyObject_Repr() must not be called with an exception set,
       because it may clear it (directly or indirectly) and so the
       caller loses its exception */
    assert(!PyErr_Occurred());
#endif

@jakirkham
Copy link
Contributor Author

After looking a bit closer and seeing what you said here @charris, I think this is the issue ( #6990 ).

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch 2 times, most recently from f29c32a to 85ce899 Compare January 11, 2016 05:31
@jakirkham jakirkham changed the title TST, MAINT: Test and fix up dot and inner type failures TST, MAINT: Make sure exceptions of inner and dot match for different cases Jan 11, 2016
@jakirkham
Copy link
Contributor Author

Failures on AppVeyor ( #6991 ) had not previously occurred for this PR with exception of the segmentation fault issue (also happened on Travis), which has since been resolved. I believe these to be unrelated to the content of this PR.

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch 2 times, most recently from fabb617 to 3b47987 Compare January 11, 2016 15:17
@jakirkham
Copy link
Contributor Author

As AppVeyor is merging with master, which is broken, it is currently failing the tests there. So, I ran my own AppVeyor build without this merge (this is rebased on a commit on master before the bad commit) and it looks like it checks out ( https://ci.appveyor.com/project/jakirkham/numpy/build/1.0.5 ).

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch from 3b47987 to b898b60 Compare January 12, 2016 00:43
@charris
Copy link
Member

charris commented Jan 12, 2016

@jakirkham Needs a rebase.

@homu
Copy link
Contributor

homu commented Jan 12, 2016

☔ The latest upstream changes (presumably #6968) made this pull request unmergeable. Please resolve the merge conflicts.

@jakirkham
Copy link
Contributor Author

Sure, @charris, can do. Are we actually happy with this change though or should we switch everything to TypeError? Should we get some more people to weigh in?

@charris
Copy link
Member

charris commented Jan 12, 2016

I certainly wouldn't mind more feedback. @njsmith @seberg Thoughts?

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch from b898b60 to b9cdc50 Compare January 12, 2016 19:40
@jakirkham
Copy link
Contributor Author

I rebased it so hopefully the proposed change is clearer to see.

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch 3 times, most recently from 37bda68 to 8ef338c Compare January 15, 2016 16:54
@jakirkham
Copy link
Contributor Author

Honestly, I kind of think they all of them should be TypeErrors.

@charris
Copy link
Member

charris commented Jan 15, 2016

Yeah, lets go with TypeError. That should be mentioned in the 1.11.0 release notes under compatibility.

@jakirkham
Copy link
Contributor Author

Sure, I'll update it.

There are a couple other cases in the same file where it raises a ValueError when it probably should be a TypeError, but we can look at those later.

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch from 8ef338c to 5fc07a2 Compare January 15, 2016 19:55
njsmith
njsmith reviewed Jan 15, 2016
View reviewed changes
doc/release/1.11.0-notes.rst
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This behaviour mimics that of other functions such as ``np.inner``. If the two
arguments cannot be cast to a common type, ``np.dot`` will now always raise a
``TypeError``. Before it had raise ``ValueError`` in some cases.
Copy link
Member

Choose a reason for hiding this comment

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

had -> could

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I realized after rereading this there were many issues and so rewrote it. Also, is this in the right place? I wasn't sure where this sort of change went so just stuck it here.

@njsmith
Copy link
Member

njsmith commented Jan 15, 2016

I certainly wouldn't mind more feedback. @njsmith @seberg Thoughts?

Sorry, missed this. I don't have a strong opinion either way :-).

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch from 51d8764 to 6ccba3c Compare January 15, 2016 20:11
@jakirkham
Copy link
Contributor Author

No worries. It was a pretty minor one. Thanks for the feedback.

@seberg
Copy link
Member

seberg commented Jan 15, 2016

Well, I would not worry about changing this at all. Changing it the other way around (dot) maybe a little, but also not too much, if you feel strongly that the current error type is bad, I think I won't stop you.

@charris
Copy link
Member

charris commented Jan 15, 2016

I'd put iot under compatibility notes rather than changes.

@charris
Copy link
Member

charris commented Jan 15, 2016

Or maybe both ;)

@jakirkham jakirkham force-pushed the test_dot_inner_type_failures branch from 6ccba3c to ef09a84 Compare January 15, 2016 20:43
@jakirkham
Copy link
Contributor Author

@charris, it is now in both places. :)

@seberg, yeah, I think having two different types of exceptions for the same thing is weird. Then again, maybe I'm just picky.

charris added a commit that referenced this pull request Jan 15, 2016
@charris
Merge pull request #6987 from jakirkham/test_dot_inner_type_failures
35790f6 
TST, MAINT: Make sure exceptions of `inner` and `dot` match for different cases
@charris charris merged commit 35790f6 into numpy:master Jan 15, 2016
@charris
Copy link
Member

charris commented Jan 15, 2016

Thanks @jakirkham .

@jakirkham
Copy link
Contributor Author

Sure. Thanks everyone.

@jakirkham jakirkham deleted the test_dot_inner_type_failures branch January 15, 2016 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
03 - Maintenance 05 - Testing component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jakirkham @charris @homu @njsmith @seberg