ENH: Add 'out' in tensordot#15186
ENH: Add 'out' in tensordot#15186mproszewska wants to merge 6 commits intonumpy:mainfrom mproszewska:tensordot-out
Conversation
| else: | ||
| out = out.reshape((newshape_a[0], newshape_b[1])) | ||
| else: | ||
| correct = False |
There was a problem hiding this comment.
This should be a TypeError, I think
There was a problem hiding this comment.
Does it actually have to be a C order array? I think we can just keep those error details to the np.dot function.
There was a problem hiding this comment.
I think that checking only shape should be enough. np.dot will check other details.
There was a problem hiding this comment.
The danger is that reshape may create a copy, so F contiguous arrays may be accepted by dot, but not necessarily work here (at least not without a copy back into out). In any case, just make sure we have tests for these things, then it is easy to try around to see how many checks are needed.
numpy/core/numeric.py
Outdated
| return res.reshape(olda + oldb) | ||
|
|
||
| out = dot(at, bt, out) | ||
| return out.reshape(olda + oldb) |
There was a problem hiding this comment.
This will end up returning a view of out when out is specified (since it is a reshaped version of the reshaped version). I am not sure I mind that though.
There was a problem hiding this comment.
I think we promise that return(func, out=out) is out for functions that have an out kwarg, no?
There was a problem hiding this comment.
@mattip: Indeed, although that promise is hard to keep unless transpose does not return a view (something subclasses can change, and I think to a certain extent ma.array does). I think the logic would be:
- (somehow) check if transpose returns a view
- If it does, then return the original out array
- If it does not, then do a copy from the transposed array back to the original array.
There was a problem hiding this comment.
Can we rewrite this function to use matmul instead of dot, using the new axes argument to matmul? Then the out would be handled for us without any reshaping.
There was a problem hiding this comment.
One small issue with such rewriting is that it gives a different speed tradeoff currently. I would not actually mind the lazy way of doing type(out) is np.ndarray saying that subclasses may be allowed in the future.
The whole __array_wrap__ dance is not fun, also the function currently uses np.asarray on the inputs in any case.
|
I still have some small issues with this, sorry that this is tricker than it seems on first sight...
EDIT: In point 2, actually, you could also do that before checking that the shape is correct really... That was unnecessary... |
|
I'm not sure how to force creation of copies in tests. |
|
I rebased this off main to see if tests still pass. @mproszewska would you want to continue with this? Did you have questions how to progress? |
|
whoops, I meant to push to #18053, sorry. Fixing. |
|
Closing, I don't think there is enough interest in this to make it happen. |
Closes #15103