Skip to content

Conversation

seberg
Copy link
Member

@seberg seberg commented Apr 26, 2023

This PR should be reviewed in multiple steps, because I included a first commit which is pretty simple, but an important refactor:

  • The first maintenance commit changes PyArray_NewFromDescr_int to use a flags object as a last thing (because I didn't want to pass 3 flags). It then adds a third flag which denotes that we know the dtype is good. That flag is generally only considered valid for creating a view, but for those it makes a lot of sense (and implies/side-steps the S0 allowing flag, which we still need, but is probably more of a stop-gap).
  • A small follow-up, because it made a check/code path obsoltete in ufunc.resolve_dtypes
  • The DEP commit: This removes an old Deprecation which is related to the first part. When a dtype= is given in array or astype calls, that cast was previously performed after absorbing subarray dimensions in some code paths. This is not the case anymore now.

As mentioned in the commit message. The combination of the two commits not only expires the FutureWarning, it also closes gh-23083.

seberg added 4 commits April 26, 2023 14:57
In some cases we know that we want to use the *exact* dtype that we already
have (mainly when taking views).  This is also useful internally because there
are very rare code-paths were we even create temporary arrays that contain
subarray dtypes.
This is now OK to just support, we won't replace things and things
should work out for the most part (probably).
This unfortunately switches over the C-only path when FromArray
is called with a subarray dtype.

Together with the previous commit (which is very simple but in a sense
does the heavy lifting):

Closes numpygh-23083
@seberg seberg force-pushed the subarray-cleanup branch from 53f64f2 to 1d4f1ac Compare April 26, 2023 12:59
}
else {
nbytes = descr->elsize = sizeof(npy_ucs4);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This whole change is just one indentation level.

@seberg seberg requested a review from mattip April 26, 2023 19:34
Copy link
Member

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

This looks like a good cleanup! I agree the new enum is much clearer than the old boolean flags at the end of PyArray_NewFromDescr_int's signature.

I only looked carefully at the structural changes to that function since I was working there recently and didn't look closely at the behavior changes that finalizing the deprecations leads to.

PyArray_FLAGS(self),
ensure_array ? NULL : (PyObject *)self,
(PyObject *)self);
(PyObject *)self, _NPY_ARRAY_ENSURE_DTYPE_IDENTITY);
Copy link
Member

Choose a reason for hiding this comment

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

nice

@mattip mattip merged commit 4d5e600 into numpy:main Apr 27, 2023
@mattip
Copy link
Member

mattip commented Apr 27, 2023

Thanks @seberg. This is a nice cleanup and refactor of a clunky interface

@seberg seberg deleted the subarray-cleanup branch April 27, 2023 15:22
@seberg
Copy link
Member Author

seberg commented Apr 27, 2023

Thanks for the review. This was started a bit ago, so I will note it but I really don't think we should worry: I think there is one code path were the deprecation was missing.

That code-path should do complete and utter nonsense though (due to the way broadcasting works), which is why I want to just not worry about it.

(A new (array_shape + subarray_shape) doesn't broadcast sensibly with (array_shape) except in very strange arguably buggy ways. So I will consider that last path a bug-fix really.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: Assign scalars into new arrays with subarray dims fails for higher dimensions
3 participants