Skip to content

Comments

ENH: Add an out argument to concatenate#9209

Merged
ahaldane merged 4 commits intonumpy:masterfrom
eric-wieser:concatenate-out
Sep 18, 2017
Merged

ENH: Add an out argument to concatenate#9209
ahaldane merged 4 commits intonumpy:masterfrom
eric-wieser:concatenate-out

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
Copy link
Member Author

@eric-wieser eric-wieser Jun 2, 2017

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 Outdated
Copy link
Member Author

@eric-wieser eric-wieser Jun 2, 2017

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 Outdated
Copy link
Member Author

@eric-wieser eric-wieser Jun 2, 2017

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 Outdated
Copy link
Member

@ahaldane ahaldane Sep 11, 2017

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

@eric-wieser eric-wieser Sep 11, 2017

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

@seberg seberg Sep 12, 2017

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

ahaldane commented Sep 11, 2017

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

eric-wieser commented Sep 13, 2017

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

ahaldane commented Sep 17, 2017

OK, will merge this soon too.

@ahaldane
Copy link
Member

ahaldane commented Sep 18, 2017

Merging, thanks Eric

@ahaldane
Copy link
Member

ahaldane commented Sep 18, 2017

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

@eric-wieser
Copy link
Member Author

eric-wieser commented Sep 18, 2017

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

@ahaldane
Copy link
Member

ahaldane commented Sep 18, 2017

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

+1 more reviewer

@ahaldane ahaldane ahaldane left review comments

Reviewers whose approvals may not affect merge requirements

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