BUG: Fix strides of trailing 1s when reshaping #2950
Conversation
|
There is always something you forget... fixed handling of size 1 arrays. Also added a second commit that would just manually fix the 0-sized array special case, just so one does not have to worry about it, since it means that |
|
The travis error looks legit: |
When adding ones to the shape for non contiguous arrays reshapes (i.e. either the first array is not contiguous or the the reshape order does not match it). The strides of trailing ones were not set. For example reshape of (6,) to (6,1,1). Previously this occured rarely because of removed special handleing when only ones were added or removed.
|
OK, failures should be fixed... |
|
Since the comment is invisible here due to rebase... I did add some better tests for the second commit (extending the old regression ones) and checked it locally. It is not checked by Travis because PyArray_NewReshape has a special case for contiguous arrays (skipping the function) and this is always triggered for 0-sized arrays. |
|
I ran all Theano's tests and all passed with this fix. thanks |
|
Now that NumPy 1.7 is released, is there something that prevent the merge of this PR? As it fix a bug introduced in the master, I would be great that is get merged. I read the full function and I'm convinced that this PR is good and fix the bug introduced. thanks |
|
Probably I am just being stupid, but I have read the description and skimmed the code and I'm still a little unclear on what the actual change is. Is the point that we had a bug where in certain cases, parts of the new strides array were left uninitialized? And then a further bug (not fixed here) where we were paying attention to those bits of the stride array that were left uninitialized (even though the value actually shouldn't have mattered), and setting the ALIGNED flag incorrectly? And finally, this PR takes the trouble to preserve the traditional C or F contiguity constraints as necessary? I support the initialization of memory even if we do manage to stop caring about strides for 1-length axes -- it makes tools like valgrind better able to do their job. |
|
That is just how I write descriptions, utterly impossible to understand... To all of your questions: yes. The one other thing I will just mention again, is that this "size <= 1" code block is currently unused, but makes the function work in all cases. I don't particularly like it, but on the other hand I also don't like the trap of having a function that almost always works as expected. So if someone prefers that currently unused code block away, no problem. |
|
I'm not a big fan of duplicate or unused code blocks either, though you're
|
|
OK, I just removed the second commit. Removing the special case in NewShape did not seem useful. If anyone ever wants to use the attempt_reshape_nocopy for something else, it is easy enough to add this again. |
|
Yeah, I took a closer look, and |
BUG: Fix strides of trailing 1s when reshaping
|
Thanks, now Theano work without problem with master. |
When adding ones to the shape for non contiguous arrays reshapes
(i.e. either the first array is not contiguous or the the reshape
order does not match it). The strides of trailing ones were not
set. For example reshape of (6,) to (6,1,1). Previously this occured
rarely because of removed special handleing when only ones were
added or removed.
Fix for gh-2949, this does not attempt to set strides in a beautiful way to conserve the contiguity of the input array, but that is maybe a bigger matter anyway (and here only interesting in corner cases I guess).