-
-
Notifications
You must be signed in to change notification settings - Fork 9.5k
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
ENH: Create string dtype instances from the abstract dtype #22923
Conversation
34a9624
to
c7d7984
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, a few comments, but generally I am happy with adding this.
Documenting this seems awkward (even the release note...) until we have a clear spelling for type(np.dtype('U'))
.
After that, it seems pretty obvious to document it in the class.
c7d7984
to
43626f2
Compare
43626f2
to
64395b4
Compare
Thanks for the comments! I used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy, although that swap would be nice (adding a test would also be nice maybe, since 2**30
hits it).
I will assume nobody minds this tiny API addition, so planning to merge soon.
} | ||
|
||
if (size < 0) { | ||
PyErr_Format(PyExc_ValueError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed this to value error, because I thought even the other ones make more sense that way. But then, it shares the error with other paths, so only did this one.
If anyone doesn't like, happy to follow up. Will merge once tests are done, thanks Nathan.
Following up from #22863 (comment), this makes it possible to create string dtype instances from an abstract string
DTypeMeta
.Should I support any additional arguments or keywords to
__new__
besidessize
? Possiblybyteorder
?Should this be documented somewhere? If so, where?