BUG: Fill correct strides for ndmin in array creation #466
Conversation
Closes "Issue numpy#465", strides need to be set according to the requested contiguous flags.
|
Sorry, forgot that with 1D arrays we cannot guess correct order from the array...The PyArray_ISFORTRAN(arr) is necessary for |
|
On Sep 29, 2012, at 11:19 AM, seberg wrote:
I don't understand this statement. Remind me again why the order matters for a 1D array? What do you mean by the order for a 1-D array?
|
|
@teoliphant if the original array is 1D its order does not matter, but the user requests an order and afterwards its not 1D anymore, ie. this case: The ISFORTRAN check is for the case: |
|
@teoliphant: IIUC, what @seberg is saying is that numpy distinguishes between 1D fortran-contiguous and 1D C-contiguous arrays. Of course they have identical memory layout, the actual data is stored in an identical way, but nonetheless, in numpy a 1D array must be one of these or the other, it can't be both. (As measured by its flags values). So therefore we have to pass around extra arguments and stuff to keep track of which sort of 1D array we want. This whole thing seems ridiculous to me, but if fixing it so that 1D arrays can be both C- and F-contiguous will break a bunch of code right before the release, then it's possible we should hold our noses and keep slapping kluges on. OTOH there seem to be more and more kluges needed.... so we need to make a judgement call on which of these two strategies is more risky. So the question is, just how badly does numpy freak out when we fix the C/F flags? Is it a few trivial things or large and complicated things? @seberg, what specifically did you see go wrong? |
|
I guess that's what I don't understand. From my understanding it has never mattered what "order" NumPy arrays were. I believe it defaulted to have C-order flag set but I don't know that any code ever relied on that. I would be surprised if NumPy required a 1D array to be one or the other. It shouldn't matter. I could see that there is some code that inappropriately doesn't want them to be "both" -- but again for 1-d arrays that shouldn't matter. So, I would make the change that contiguous arrays that are 1-D have both flags set and fix any problems that may arise from that. On Sep 29, 2012, at 5:03 PM, njsmith wrote:
|
|
@njsmith not quite? I hope this is clearer. The current situation is this that 1D arrays both C and F-Contiguous. However higher dimensional ones can never be both C/F-Contiguous. Take a 1x3x1 array (1 byte itemsize), if its strides are (3,1,1) its C order if they are (1,1,3) its F order, if they are anything else they are neither, but its all the same array on the same data. So the proposed fix was to just ignore those axes, so that for that 1x3x1 array also strides=(0,1,-10) would be both F and C-Contiguous. From the numpy side, I actually don't see a real problem. You have to fix:
So, these are not complex changes really and not really much code changed (and most of that is messing with UPDATEFLAGS). I got them pretty much done though I won't guarantee there isn't some array creation method that I missed. So if you want to fix the flags like that and this is not needed as a non-invasive bug fix, this change needs not be made at all. I am not sure about is how badly 1. might affect someone. Its easy to fix, but it could be a nasty bug in some C extensions for someone who relies on that equality now or in the future. It may be an option to still set the strides sensibly (ie. always to itemsize) to avoid this though. |
|
Oh, and please forget that comment :), it was really just a stupid comment when I realized that I had to change the commit to cover all cases, if you look at the |
BUG: Fill correct strides for ndmin in array creation
Closes "Issue #465", strides need to be set according to the requested contiguous flags.
I will add tests and maybe another bug fix from my cflags branch which is somewhat related. And just to note in that branch this change would not really matter ;), as these strides do not really matter to contiguous flags.