Skip to content
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.

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

MAINT: Refactor differences between cblas_matrixproduct and PyArray_MatrixProduct2 #11432

Merged
merged 1 commit into from
Jun 30, 2018

Conversation

mattip
Copy link
Member

@mattip mattip commented Jun 27, 2018

The two functions do very similar things, cblas_matrixproduct is used in both PyArray_MatrixProduct2 (which is the actual implementation of dot) and in array_matmul if ndim <=2 and the dtype is fitting. Otherwise array_matmul goes through einsum, PyArray_MatrixProduct2 has its own continuation. I worked through the logic to compare the two and found points of code duplication:

stage PyArray_MatrixProduct2 cblas_matrixproduct
detect non-aligned or non-contiguous data doesn't care uses PyArray_NewCopy if non-aligned or non-contiguous
create output array use new_array_for_sum refactored lines of code to new_array_for_sum in this PR
logic to handle vector-matrix, matrix-matrix, scalar-matrix and visa-versa uses PyArray_IterAllButAxis if blocks for each case
chooses function for sum-of-loops uses out->dtype->vdot if blocks for each of the level 2 BLAS cases, use out->dtype->vdot for vector/scalar (which will use level 1 BLAS dot functions)

This PR makes the two functions share the output array logic and uses the same semantics for the level 1 BLAS dot functions (scalar-vector)

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Very nice cleanup! Apart from a total nitpick - the unneeded change in numpy/core/src/multiarray/cblasfuncs.h - this looks all OK to me.

@mhvk
Copy link
Contributor

mhvk commented Jun 29, 2018

Good to squash before merging, though, that way the commit message is more guaranteed to be what you want.

@mattip mattip force-pushed the refactor-new_array_for_sum branch from 0003933 to 8486c8c Compare June 29, 2018 16:59
@mattip mattip force-pushed the refactor-new_array_for_sum branch from 8486c8c to 760daff Compare June 29, 2018 17:05
@mattip
Copy link
Member Author

mattip commented Jun 30, 2018

Squashed, rebased, removed the gratuitous change, and realized that adding a include directive to one file meant I should remove it from the another

*
* If `out` is non-NULL, memory overlap is checked with ap1 and ap2, and an
* updateifcopy temporary array may be returned. If `result` is non-NULL, the
* output array to be returned (`out` if non-NULL and the newly allocated array
Copy link
Contributor

Choose a reason for hiding this comment

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

I must admit I do not understand what is the point of possibly passing on result to store the output in when the output is also returned. Since this PR is just about moving stuff so it can be used in multiple places, I think it is better to leave it as is here, but if you have the energy for a clean-up follow-up PR, that might be an idea...

Copy link
Member Author

Choose a reason for hiding this comment

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

result is the output ndarray to be returned - either out or a freshly allocated ndarray (let's call it retval). retval is the ndarray to be iterated over. If writeback semantics are active then retval is not out rather it is a new ndarray for iterating, and retval->base == out. Then toward the end of the function the writeback semantics are resolved, retval is discared, and the function returns return.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation! If in a future PR you get to touch this file again, maybe add it to the comments...

@mhvk
Copy link
Contributor

mhvk commented Jun 30, 2018

I'll merge now. I guess you can copy your table on top for the next clean-up!!

@mhvk mhvk merged commit e33be77 into numpy:master Jun 30, 2018
@mattip mattip deleted the refactor-new_array_for_sum branch October 9, 2018 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants