8000 ``sum`` method returns array with invalid C/F-contiguous flags · Issue #387 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

sum method returns array with invalid C/F-contiguous flags #387

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.

8000 Sign up for GitHub

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

Jump to bottom
Closed
stefanv opened this issue Aug 14, 2012 · 7 comments
Closed

sum method returns array with invalid C/F-contiguous flags #387

stefanv opened this issue Aug 14, 2012 · 7 comments

Comments

@stefanv
Copy link
Contributor
stefanv commented Aug 14, 2012 
>>> x = np.random.random((5, 3))
>>> y = x.sum(0)
>>> print y.flags
  C_CONTIGUOUS : False
  F_CONTIGUOUS : False
  OWNDATA : True
  WRITEABLE : True
  ALIGNED : True
  UPDATEIFCOPY : False
>>> print y.strides, y.dtype
(8,) float64

This still works in 1.5.1. Broken on the latest Git HEAD.

It affects all reductions of ndarray objects.
@tomspur
Copy link
tomspur commented Aug 25, 2012

The first bad commit over here is aed9925, but don't have enough knowledge about the code base to tell more (Just took this issue as an opportunity to start digging in...).

@njsmith njsmith mentioned this issue Aug 31, 2012
Closed
@certik
Copy link
Contributor
certik commented Sep 2, 2012

@stefanv, would you know how to fix this?

@seberg
Copy link
Member
seberg commented Sep 2, 2012

Hey, I think this might all come down to 0 stride arrays (it seems to me the code added in the commit tomspurs linked uses 0-strides). Maybe it is an idea to fix it by flagsobject.c, something along the lines of (same for Fortranconigouos of course)
EDIT: sorry thought this kind of thing fixes something... but actually it seems like its not enough to fix both flags.

--- a/numpy/core/src/multiarray/flagsobject.c
+++ b/numpy/core/src/multiarray/flagsobject.c
@@ -134,10 +134,12 @@ _IsContiguous(PyArrayObject *ap)
         if (dim == 0) {
             return 1;
         }
-        if (PyArray_STRIDES(ap)[i] != sd) {
-            return 0;
+        if (dim != 1) {
+            if (PyArray_STRIDES(ap)[i] != sd) {
+                return 0;
+            }
+            sd *= dim;
         }
-        sd *= dim;
     }
     return 1;
 }

However I wonder if this may create troubles at some other places. The thing is that size 1 dimensions do not really matter when it comes to C-Contiguousity. Alternatively, maybe these strides in size 1-dimensions should maybe be silently replaced with something other then 0? I remember seeing a 0-stride reshape bug too in the Tracker, comeing down to a similar issue (though at another place I guess).

@seberg
Copy link
Member
seberg commented Sep 2, 2012

Actually, the thing above is what it comes down too. However, UpdateFlags assumes that for multi dimensional arrays it cannot be true that both C and F-contiguous is true. So either PyArray_RemoveAxesInPlace (shape.c) must update the flags, or here as said above 1 sized dimensions be generally allowed to have any stride (and UpdateFlags would need this assumption of mutual exclusiveness removed too).
Actually I think RemoveAxisInplace needs this check anyways for the case of making an array 1D, sice it says it can remove an Axis that is larger then size 1 (though I am not sure that is intended to be used).

@certik
Copy link
Contributor
certik commented Sep 2, 2012

Thanks @seberg for the investigation. Well, we can try the above fix and run tests. That should reveal if it breaks something.

@seberg seberg mentioned this issue Sep 2, 2012
Closed
@seberg
Copy link
Member
seberg co 8000 mmented Sep 2, 2012

@certik added a PR with this idea...

@seberg seberg mentioned this issue Sep 3, 2012
Merged
seberg added a commit to seberg/numpy that referenced this issue Sep 4, 2012
@seberg
Tests related to Issue numpy#387
380ce94
@njsmith
Copy link
Member
njsmith commented Sep 21, 2012

Fixed by #420.

@njsmith njsmith closed this as completed Sep 21, 2012
certik pushed a commit to certik/numpy that referenced this issue Sep 30, 2012
@seberg @certik
Tests related to Issue numpy#387
4b6aa87
teoliphant pushed a commit to ContinuumIO/numpy that referenced this issue Nov 6, 2012
@seberg @teoliphant
Tests related to Issue numpy#387
ed41308
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@certik @stefanv @seberg @tomspur @njsmith
0