ENH: np.dot: better "matrices not aligned" message #4985
Conversation
|
thats very good change, it would be even better if it also reports the axis of the mismatch not just the sizes of the mismatch |
|
Oh, this is probably going to conflict with #4969... |
|
I'd give this PR precedence, this is simple and useful we should probably also add it to 1.9 |
| not_aligned(Py_ssize_t m, Py_ssize_t n) | ||
| { | ||
| PyErr_Format(PyExc_ValueError, "matrices are not aligned: %zd != %zd", | ||
| m, n); |
There was a problem hiding this comment.
maybe as message:
matrices are not aligned: shape[%d] (%zd) != shape[%d] (%zd)
it should mention shape, because its not obvious what aligned even means
There was a problem hiding this comment.
Good idea, I'll see if I can hack this up.
|
Definitely a conflict with #4969, as |
|
Ok, put in the improvements suggested by @juliantaylor. |
|
multiarraymodule.c:976 has another instance of the message but with objects instead of matrix |
|
Hm. That makes me want to factor the message out there as well. Should I put the function in |
|
hm I guess one could put it there can you please also run this command on the branch to allow easy merge to 1.9 (assuming origin is remote name main numpy repo): |
|
@charris I saw you moved some of my code there, that's why I asked :) @juliantaylor And then force-push? |
|
yes |
|
It looks like |
|
urg, jsut duplicate the function, not the end of the world. we can clean it up in the reorg PR which does buildsystem changes anyway |
|
putting it in common.h would be an option |
|
Actually, that's where I put my functions... |
|
Ok, it's in |
|
I prefer an orange bikeshed, and also for this error message to just print the full shape of both arguments, because often this makes it obvious what happened (e.g. if I try to dot arrays with shapes |
|
it does now say: additionally printing the full shapes could be useful but its also quite easy to just print them from the input arrays. |
|
It's often not so easy for the user to print the original shapes - this It's not the most important issue ever, but I do think it would be better
|
|
One orange bikeshed for mr. @njsmith: >>> x = np.zeros((10, 2))
>>> np.dot(x, x)
Traceback (most recent call last):
File "<ipython-input-8-7170710dec0d>", line 1, in <module>
np.dot(x, x)
ValueError: matrices of shapes (10, 2) and (10, 2) not aligned: 2 != 10
>>> x = np.zeros((10, 2, 4, 3, 7))
>>> np.dot(x, x)
Traceback (most recent call last):
File "<ipython-input-10-7170710dec0d>", line 1, in <module>
np.dot(x, x)
ValueError: matrices of shapes (10, 2, 4, 3, 7) and (10, 2, 4, 3, 7) not aligned: 7 != 3 |
| } | ||
| else { | ||
| PyErr_Format(PyExc_ValueError, | ||
| "matrices of shapes %s and %s not aligned: %zd != %zd", |
There was a problem hiding this comment.
Shall I change this to "arrays" instead of "matrices"?
There was a problem hiding this comment.
or just drop matrix of
|
I'd still keep the shape index as in the original, that way its clear which axis is the problem in case there a duplicates. |
|
d3c2a74fd43ce8bb9c9c06176473554216fd74d0 reuses >>> x = np.zeros((10, 2))
>>> np.dot(x, x)
Traceback (most recent call last):
File "<ipython-input-5-7170710dec0d>", line 1, in <module>
np.dot(x, x)
ValueError: shapes (10,2) and (10,2) not aligned: 2 (dim 1) != 10 (dim 0)
>>> x = np.zeros((10, 2, 4, 3, 7))
>>> np.dot(x, x)
Traceback (most recent call last):
File "<ipython-input-6-7170710dec0d>", line 1, in <module>
np.dot(x, x)
ValueError: shapes (10,2,4,3,7) and (10,2,4,3,7) not aligned: 7 (dim 4) != 3 (dim 3)Please review, I'm calling it quits: it's midnight over here. |
|
how to turn a 20 line diff into a 200 line diff by adding redundant information ... would possibly be simpler by decoding the pystring to ascii, but this is fine too. |
|
fwiw I would prefer to revert back to the simpler version which I want in 1.9 |
|
@juliantaylor I tried the ASCII conversion at first but it was actually more complicated than this. I suggest that I:
Do you guys all agree with a modified 818398df1e484dd9f9bb9a564466c1ca82bdcdc6 (so with the |
|
I'm for either everything as is (with warnings fixed) or the really simple one we had in the beginning that only updated the error message with no fancy shape printing. |
I had to move convert_shape_to_string to common.h so that _dotblas.c can access it.
|
Ok, I squashed the last two commits to get rid of the intermediate version and included |
ENH: np.dot: better "matrices not aligned" message
|
thanks, pushed to 1.9, I saw you already opened a PR onto charris branch, so we don't need to merge this into master. |
|
Thanks! |
based on numpygh-4985 by Lars Buitinck
This scratches a long-standing itch of mine, which is that
np.dot's "matrices not aligned" message never explains which of the two arguments I forgot to transpose somewhere deep inside an algorithm. This patch makes it report the mismatching pair of dimensions.It might be even prettier to report the full shape of both inputs, but I think this is a big enough improvement for now.