Skip to content

ENH: Add an out argument to concatenate #9209

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 4 commits into from
Sep 18, 2017
Merged

ENH: Add an out argument to concatenate #9209

merged 4 commits into from
Sep 18, 2017

Conversation

eric-wieser
Copy link
Member

@eric-wieser eric-wieser commented Jun 2, 2017 •
edited
Loading

Useful for:

  • Reusing a single buffer
  • Casting and concatenating in one step

eric-wieser
eric-wieser commented Jun 2, 2017
View reviewed changes
Copy link
Member Author

@eric-wieser eric-wieser left a comment

Choose a reason for hiding this comment

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

Arguably we might now want a casting argument as well

numpy/core/src/multiarray/multiarraymodule.c Outdated
}
Py_INCREF(ret);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Only real change here is this if statement

numpy/core/src/multiarray/multiarraymodule.c
}
Py_INCREF(ret);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

And this if statement

numpy/core/src/multiarray/multiarraymodule.c
PyArray_Concatenate(PyObject *op, int axis)
{
return PyArray_ConcatenateInto(op, axis, NULL);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

And here we just redirect the old API to not break compatibility

@eric-wieser eric-wieser force-pushed the concatenate-out branch 2 times, most recently from 909b411 to b7bee01 Compare June 2, 2017 19:26
@ahaldane
Copy link
Member

ahaldane commented Jun 16, 2017 •
edited
Loading

Seems pretty useful to me. There are definitely times where I've wanted to do such an operation and resorted to doing it by hand in a loop over a statement like out[i:i+L] = arrs[n].

Code LGTM.

@eric-wieser eric-wieser changed the title WIP/ENH: Add an out argument to concatenate ENH: Add an out argument to concatenate Jul 1, 2017
@eric-wieser
Copy link
Member Author

eric-wieser commented Jul 1, 2017 •
edited
Loading

Tests and release note added. I've left out hstack and vstack deliberately, since guessing the output shape for these is confusing, and since we only have them for backwards compatibility, leaving them behind will encourage switching to stack.

I'll leave casting for another time, since it's messy, and isn't useful in most cases anyway

@eric-wieser eric-wieser mentioned this pull request Sep 11, 2017
Merged
@eric-wieser
Copy link
Member Author

eric-wieser commented Sep 11, 2017 •
edited
Loading

Rebased to fix release note clashes. @ahaldane, good to merge?

ahaldane
ahaldane reviewed Sep 11, 2017
View reviewed changes
numpy/core/src/multiarray/multiarraymodule.c
* the memory layout of the input arrays, using ambiguity
* resolution rules matching that of the NpyIter.
*/
PyArray_CreateMultiSortedStridePerm(narrays, arrays, ndim, strideperm);
Copy link
Member

Choose a reason for hiding this comment

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

incidentally, this is the only place in numpy this function is used.

For a second I thought there might be some problem that we ignore strides when out is provided, like in #7633, but it's fine, PyArray_AssignArray below is insensitive to order.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I assume this is an optimization

Copy link
Member

Choose a reason for hiding this comment

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

I remember this function being new in 1.7 or so, somewhat thought it was used with the new iterator, but maybe there was some change there (since I also remember there was a some bug with this or its usage). The idea is simple though, find the cache friendliest iteration order....

@ahaldane
Copy link
Member

The tests need to be reworked, not sure what happened to assertIs.

Code LGTM again.

Also, we might use the new priority function in #9672 too.

Actually, a lot of the the out processing is more or less duplicate in that code too (eg check for dimensionality, shape, computation of subtype and datatype). In the future we might consider splitting it off into a function too.

@eric-wieser
Copy link
Member Author

Fixed to use assert_(x is y) - thanks for saving me from checking the travis logs

@eric-wieser eric-wieser mentioned this pull request Sep 14, 2017
Merged
@ahaldane
Copy link
Member

OK, will merge this soon too.

@ahaldane
Copy link
Member

Merging, thanks Eric

@ahaldane
Copy link
Member

oh, sorry, the page just refreshed and there's a release note conflict you need to fix.

@eric-wieser
Copy link
Member Author

Ok, merged through the web interface, since rebasing causes a mess due to the .gitattributes file.

@ahaldane
Copy link
Member

Thanks for the quick fix, merging for real now.

@ahaldane ahaldane merged commit 2d7352a into numpy:master Sep 18, 2017
@mattip mattip mentioned this pull request Sep 1, 2018
Closed
@ahaldane ahaldane mentioned this pull request Oct 6, 2018
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seberg seberg seberg left review comments

@ahaldane ahaldane ahaldane left review comments

Assignees
No one assigned
Labels
01 - Enhancement component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric-wieser @ahaldane @seberg