ENH: Allow using dtype classes in arr.astype()
#23154
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit also moves the check for legacy dtypes (but does not yet delete it).
ngoldbaum
reviewed
Feb 3, 2023
There was a problem hiding this comment.
I've left a few minor inline comments.
I also tried this using ASCIIDType and it seems work as I'd expect:
import os
os.environ["NUMPY_EXPERIMENTAL_DTYPE_API"] = "1"
import numpy as np
from asciidtype import ASCIIDType
arr = np.array(["this", "is", "an", "array"]).astype(ACIIDType)
print(repr(arr))prints on this PR:
array(['this', 'is', 'an', 'array'], dtype=ASCIIDType(5))
Awesome!
|
@ngoldbaum Looks like this is ready. |
|
Agreed, sorry for not explicitly approving. |
ngoldbaum
approved these changes
Feb 20, 2023
arr.astype()arr.astype()
|
Thanks Sebastian. |
ngoldbaum
added a commit
to ngoldbaum/numpy
that referenced
this pull request
Mar 16, 2023
This enables writing np.array(some_object, dtype=type(np.dtype('i'))). This
is a follow-on from numpy#23154, see that PR
for more details.
I had to add a new include to `ctors.h` to bring in the definition of the
`npy_dtype_info` struct. Since `ctors.h` is included in many other files inside
numpy, I found that I needed to modify fewer includes across numpy if I moved
the definition of `npy_dtype_info` to `common.h` from `descriptor.h`. The new
includes of `common.h` are needed to support later includes of `ctors.h` in
those files. If anyone has an alternate place to put `npy_dtype_info` that would
cause less churn of includes I'd love to hear about it.
I spent a bunch of time tweaking the reference counts. I'm reasonably confident
this is correct but not 100%, an additional careful pass over the reference
count logic from a reviewer would be very appreciated.
I could have made `_PyArray_FromAny` and `_PyArray_CheckFromAny` take just a
`npy_dtype_info` struct, but I found it made the reference count logic more
complicated, since `PyArray_FromAny` and `PyArray_CheckFromAny` steal the
reference to the descriptor they are passed and I needed to conserve that
behavior. Also both functions support passing in a `NULL` pointer for the
descriptor and I needed to maintain that behavior as well.
The change to `ucsnarrow.h` fixes a preexisting conflict with the prototype
in `ucsnarrow.c` that triggered a compiler error while I was working on this.
ngoldbaum
added a commit
to ngoldbaum/numpy
that referenced
this pull request
Mar 17, 2023
This enables writing np.array(some_object, dtype=type(np.dtype('i'))). This
is a follow-on from numpy#23154, see that PR
for more details.
I had to add a new include to `ctors.h` to bring in the definition of the
`npy_dtype_info` struct. Since `ctors.h` is included in many other files inside
numpy, I found that I needed to modify fewer includes across numpy if I moved
the definition of `npy_dtype_info` to `common.h` from `descriptor.h`. The new
includes of `common.h` are needed to support later includes of `ctors.h` in
those files. If anyone has an alternate place to put `npy_dtype_info` that would
cause less churn of includes I'd love to hear about it.
I spent a bunch of time tweaking the reference counts. I'm reasonably confident
this is correct but not 100%, an additional careful pass over the reference
count logic from a reviewer would be very appreciated.
I could have made `_PyArray_FromAny` and `_PyArray_CheckFromAny` take just a
`npy_dtype_info` struct, but I found it made the reference count logic more
complicated, since `PyArray_FromAny` and `PyArray_CheckFromAny` steal the
reference to the descriptor they are passed and I needed to conserve that
behavior. Also both functions support passing in a `NULL` pointer for the
descriptor and I needed to maintain that behavior as well.
The change to `ucsnarrow.h` fixes a preexisting conflict with the prototype
in `ucsnarrow.c` that triggered a compiler error while I was working on this.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This enables writing
arr.astype(type(np.dtype("S"))), which may seem a bit ridiculous, but is the current way that user-dtypes would be able to adapt a parametric DType based on object array data mainly and otherwise based on the array's dtype (not data).(Yes, there may be a point in allowing to adapt to the values even when the array is not an object array initially)
This PR does not yet adept to allow
np.array([...], dtype=DType_class)to do the same, but implements the necessary machinery (or a plausible way of making progress there).There are a few commits which are roughly sorted (a deletion of the extract function is in a following commit).
The main code is:
The final refactor of
PyArray_AdaptDescriptorToArrayand actual change is then rather small.Ping @ngoldbaum since it may interest you and also it would be good to have a bit of feedback on whether this seems reasonable. The main alternative is decide on using abstract instanes more broadly (which would be closer to what we used to have but was something I tried to avoid a bit initially).