Skip to content

BUG: Fix bug with size 1-dims in CreateSortedStridePerm #2696

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

Merged
merged 1 commit into from
Nov 16, 2012

Conversation

seberg
Copy link
Member

@seberg seberg commented Oct 23, 2012

This changes CreateSortedStridedPerm to not use the shape for special casing 1-dim axis. The cleanup does not seem to be useful in most cases and the current way is buggy. Also insert stride so that reduce with keepdims=1 will keep contiguous arrays contiguous. "Closes Issue #434"

Sorry if a PR against 1.7. is the wrong thing at this point. But to me this seems the easiest way to avoid any headaches.

This changes CreateSortedStridedPerm to not use the shape
for special casing 1-dim axis. The cleanup does not seem
to be useful in most cases and the current way is buggy.
Also insert stride so that reduce with keepdims=1 will
keep contiguous arrays contiguous. "Closes Issue numpy#434"
@seberg
Copy link
Member Author

seberg commented Oct 23, 2012

I don't know if this needs to up the C-ABI/API versions or not.

@njsmith
Copy link
Member

njsmith commented Oct 23, 2012

Looks fine to me.

@certik: The background here is that #2694 is going to change the call signature for CreateSortedStridePerm, which is a new public API function in 1.7. But, #694 is too large to backport to 1.7. So this PR makes a minimal matching change to 1.7 so that we won't end up with API breakage or having to make uglier workarounds later.

@certik
Copy link
Contributor

certik commented Nov 14, 2012

I set this to milestone 1.7. This looks ok to me too, but given the change, @charris, @rgommers, would you mind giving this an OK as well? After that it can be merged.

@rgommers
Copy link
Member

@seberg you don't need to update the API/ABI versions. That should be done by the release manager just before a release. For master or unreleased branches you can get a CAPIMismatchWarning during building, but that's not a big deal.

@rgommers
Copy link
Member

@certik motivation for this PR sounds good. I didn't look at this code in detail, if @njsmith reviewed it that should be enough.

@certik
Copy link
Contributor

certik commented Nov 14, 2012

Ok. Where is the API/ABI version? I am only aware of these lines in setup.py:

MAJOR               = 1
MINOR               = 8
MICRO               = 0

I'll wait a little more with the merge, to give others chance to comment. If there are no more objections, I'll merge it tomorrow.

@seberg
Copy link
Member Author

seberg commented Nov 15, 2012

First of, what we are talking about here is ie. a 3x1x3 Fortran ordered array, that is put into CreateSortedStridePerm will not be Fortran order any more unless the shape information is used. This does somewhat matter for array creation purpose (I thought that would change when I wrote this, but it probably won't) but not when sorting for speed reasons. So if the array creation is supposed to be all clean, either the shape information needs to stay passed in and probably also added to npy_stride_sort_item (if that is possible) or maybe a new function to clean the strides of 1-dimensional axes created.

I am tending toward just doing it like this PR does and then maybe implementing that extra function later. But if you prefer that CreateSortedStridePerm should sort cleanly right away, I could do a PR for that.

@certik
Copy link
Contributor

certik commented Nov 16, 2012

That's fine with me. Since nobody is against, I am merging it now.

certik added a commit that referenced this pull request Nov 16, 2012
BUG: Fix bug with size 1-dims in CreateSortedStridePerm
@certik certik merged commit 3ecbac5 into numpy:maintenance/1.7.x Nov 16, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants