Skip to content

ENH: Allow views using smaller coprime itemsizes #6097

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

Closed
wants to merge 1 commit into from

Conversation

ahaldane
Copy link
Member

Previously, this was allowed:

np.zeros(3, dtype='p,p').view('p,p,p')

But this was not:

np.zeros(2, dtype='p,p,p').view('p,p')

This commit makes the second case work like the first.

I have to admit it's not clear to me why the code treated the two cases differently before.

Previously, this was allowed:    np.zeros(3, dtype='p,p').view('p,p,p')
But this was not:                np.zeros(2, dtype='p,p,p').view('p,p')

This commit makes the second case work like the first.
/*
* Determine if last (or first if NPY_ARRAY_F_CONTIGUOUS) dimension
* is compatible
*/
newdim = PyArray_DIMS(self)[i] * PyArray_DESCR(self)->elsize;
if ((newdim % newtype->elsize) != 0) {
nbytes = PyArray_DIMS(self)[i] * PyArray_DESCR(self)->elsize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can use PyArray_DIM(self, i) here.

@charris
Copy link
Member

charris commented Jul 20, 2015

AFAICT, neither is currently allowed.

In [6]: np.zeros(2, 'p,p').view('p,p,p')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-02190acaad61> in <module>()
----> 1 np.zeros(2, 'p,p').view('p,p,p')

ValueError: new type not compatible with array.

In [7]: np.zeros(2, 'p,p,p').view('p,p')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-7-8075bf4648ec> in <module>()
----> 1 np.zeros(2, 'p,p,p').view('p,p')

ValueError: new type not compatible with array.

And the relevant code hasn't changed since 2011 (1.7 release I suspect). I don't see that either should be acceptable.

EDIT: OK, I see what the difference is. But what if the array is not contiguous?

@charris
Copy link
Member

charris commented Jul 20, 2015

Looks like contiguity is checked somewhere

In [6]: np.zeros(2, 'p,p').view('p,p,p')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-6-02190acaad61> in <module>()
----> 1 np.zeros(2, 'p,p').view('p,p,p')

ValueError: new type not compatible with array.

In [7]: np.zeros(2, 'p,p,p').view('p,p')
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
<ipython-input-7-8075bf4648ec> in <module>()
----> 1 np.zeros(2, 'p,p,p').view('p,p')

ValueError: new type not compatible with array.

I need to think about this a bit. It would seem that if the first is OK then the second should be OK also, but I'd like to check under what circumstances it is allowed. It seems a bit hinky in that it depends not only on array shape and element size, but also the strides.

@ahaldane
Copy link
Member Author

I think contiguity is accounted for a couple lines above the lines I changed (through a check for PyArray_ISONESEGMENT). I'm reasonably convinced that code is right, though the NPY_C_CONTIGUOUS and NPY_F_CONTIGUOUS flags have confused me before.

I admit this is a pretty odd case which is unlikely to be used. But the first example above (which works currently) came up in #5508 and #5548, and seemed important enough that a bunch of tests use it (see a few lines above the new test I added here). Somehow I had never noticed until now that while an increasing itemsize worked a decreasing one didn't. I happened on it just now while playing with structured arrays for another PR.

@njsmith
Copy link
Member

njsmith commented Jul 21, 2015

I think the 3 versus 2 in OP's original calls to zeros may be crucial?
On Jul 20, 2015 14:01, "Charles Harris" notifications@github.com wrote:

Looks like contiguity is checked somewhere

In [6]: np.zeros(2, 'p,p').view('p,p,p')

ValueError Traceback (most recent call last)
in ()
----> 1 np.zeros(2, 'p,p').view('p,p,p')

ValueError: new type not compatible with array.

In [7]: np.zeros(2, 'p,p,p').view('p,p')

ValueError Traceback (most recent call last)
in ()
----> 1 np.zeros(2, 'p,p,p').view('p,p')

ValueError: new type not compatible with array.

I need to think about this a bit. It would seem that if the first is OK
then the second should be OK also, but I'd like to check under what
circumstances it is allowed. It seems a bit hinky in that it depends not
only on array shape and element size, but also the strides.


Reply to this email directly or view it on GitHub
#6097 (comment).

@charris
Copy link
Member

charris commented Jul 21, 2015

@njsmith Yes, I noted that in the edit.

@ahaldane
Copy link
Member Author

Hmm after looking over #5508 again, I just realized that this PR is simply a more limited form of that PR - the changes here are almost word-for-word implemeted there, plus more. Maybe I will go over there and look at that code :)

Do you mind if I close this one in favor of that one?

@charris
Copy link
Member

charris commented Jul 21, 2015

@ahaldane Not at all ;) One less PR to review.

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.

3 participants