Add pylibcudf.Scalar.from_py for construction from Python strings, bool, int, float
#17898
Conversation
mroeschke
added
improvement
mroeschke
changed the title
pylibcudf.Scalar.from_py for construction from Python strings, bool, int, float
| if isinstance(py_val, py_bool): | ||
| dtype = DataType(type_id.BOOL8) | ||
| c_val = make_numeric_scalar(dtype.c_obj) | ||
| (<numeric_scalar[cbool]*>c_val.get()).set_value(py_val) | ||
| elif isinstance(py_val, int): | ||
| dtype = DataType(type_id.INT64) | ||
| c_val = make_numeric_scalar(dtype.c_obj) | ||
| (<numeric_scalar[int64_t]*>c_val.get()).set_value(py_val) | ||
| elif isinstance(py_val, float): | ||
| dtype = DataType(type_id.FLOAT64) | ||
| c_val = make_numeric_scalar(dtype.c_obj) | ||
| (<numeric_scalar[double]*>c_val.get()).set_value(py_val) | ||
| elif isinstance(py_val, str): | ||
| dtype = DataType(type_id.STRING) | ||
| c_val = make_string_scalar(py_val.encode()) | ||
| else: | ||
| raise NotImplementedError(f"{type(py_val).__name__} is not supported.") |
There was a problem hiding this comment.
Can we rewrite this pattern with singledispatch? I anticipate this cascade eventually getting arbitrarily large and we'll start paying the price of failing through earlier cases to get to later ones. That also gives you the benefit of (since each overload is a separate function) being able to cdef the c_val type accordingly in each case.
There was a problem hiding this comment.
Good idea, how would you handle creating the final Scalar with multiple dispatched functions? I'm referring to this logic
cdef Scalar s = Scalar.__new__(Scalar)
s.c_obj.swap(c_val)
s._data_type = dtype
return s
There was a problem hiding this comment.
Oh would this just go in a separate method so it can be used in each dispatched method?
There was a problem hiding this comment.
Eg.
@staticmethod
def _finalize_scalar(c_obj, dtype):
cdef Scalar s = Scalar.__new__(Scalar)
s.c_obj.swap(c_obj)
s._data_type = dtype
return s
|
|
||
| @classmethod | ||
| def from_py(cls, py_val): | ||
| """Convert a Python standard library object to a Scalar. |
There was a problem hiding this comment.
Should we expand this doc string if this method is public?
| dtype = DataType(type_id.STRING) | ||
| c_val = make_string_scalar(py_val.encode()) | ||
| else: | ||
| raise NotImplementedError(f"{type(py_val).__name__} is not supported.") |
There was a problem hiding this comment.
I'm not sure if this is a nitpick (or incorrect) but should we distinguish between a type (that can be converted to a scalar) but is not implemented yet and a type that cannot be converted to a scalar? If its the latter, then we should raise TypeError, right?
There was a problem hiding this comment.
It is a nitpick, but not incorrect. You're right, that would be optimal. The difficulty is in separating the two. We could have this fallback case be a TypeError and then add another NotImplementedError case that checks isinstance on a bunch of types that we expect to support. It'll be very difficult to capture all of them exhaustively, though. We'll probably have some things falling through to TypeError that we actually want to support for a while if we do this.
Description
Towards #17054
Checklist