Skip to content

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

Merged
merged 1 commit into from Nov 16, 2012

4 participants

@seberg
NumPy member
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.

@seberg seberg BUG: Fix bug with size 1-dims in CreateSortedStridePerm
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"
e565afb
@seberg
NumPy member
seberg commented Oct 23, 2012

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

@njsmith
NumPy 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
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
NumPy 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
NumPy 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
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
NumPy member
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
certik commented Nov 16, 2012

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

@certik certik merged commit 3ecbac5 into numpy:maintenance/1.7.x Nov 16, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.