-
-
Notifications
You must be signed in to change notification settings - Fork 9
fixes for StringDType #22
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
Conversation
Honestly, now that I see arbitrary sized strings passing length seems weird. For most functions, the long-term thing would be a ufunc replacement. For others, I suppose we have no good story, we could hide something away like Anyway, don't want to derail trials here, just seems like something better is needed if we really want full support for something like |
{ | ||
PyObject *ret_bytes = NULL; | ||
PyTypeObject *scalar_type = Py_TYPE(scalar); | ||
// FIXME: handle bytes too |
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.
Maybe numpy bytes only? Normal bytes shouldn't be necessary, Python 3 doesn't do it. NumPy bytes are just weird because they serve the dual purpose of an ascii/latin1 string.
Agreed that passing a size is weird. I'd like to come back to that, I agree that it might be better to just expose all the functionality in |
"common_instance called on unequal StringDType instances"); | ||
return NULL; | ||
} | ||
Py_INCREF(dtype1); |
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.
👍
stringdtype/stringdtype/src/dtype.c
Outdated
|
||
long size = 0; | ||
|
||
if (!PyArg_ParseTupleAndKeywords(args, kwds, "|l:ASCIIDType", kwargs_strs, |
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.
if (!PyArg_ParseTupleAndKeywords(args, kwds, "|l:ASCIIDType", kwargs_strs, | |
if (!PyArg_ParseTupleAndKeywords(args, kwds, "|l:StringDType", kwargs_strs, |
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.
good catch!
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.
Looks good with a minor change.
ab6ef04
to
865c973
Compare
This brings over some functionality from
asciidtype
. I also include someasciidtype
changes here to keepstringdtype
andasciidtype
more uniform.StringScalar
astr
subclass. This makes it possible to use the functions innp.char
to manipulateStringDType
datapartition
andrpartition
wrappers to make sure the result of those functions has uniform types.common_instance
. Also simplifies it a bit sinceStringDType
isn't parametric.StringDType
with an optionalsize
argument, which gets ignored. This makes the API more uniform with numpy's other string dtypes. In particular, this way of creating a string dtype is now used innp.char
, so this also allowsStringDType
to work with functions innp.char
. Currently we just ignore thesize
but in principle we could use it as a size hint? I'm not sure if that's actually useful.