[REVIEW] Add handling of mixed numeric types in to_dlpack
#9585
Conversation
galipremsagar
added
feature request
|
rerun tests |
| else: | ||
| raise TypeError( | ||
| f"Input of type {type(cudf_obj)} cannot be converted " | ||
| "to DLPack tensor" | ||
| ) | ||
|
|
||
| return libdlpack.to_dlpack(gdf_cols) | ||
| if any( |
There was a problem hiding this comment.
It's perhaps slightly more future proof to error based on a dtype not being present in a predefined supported set rather then checking if a dtype is in an unsupported set. That way you dont have to worry about updating it here as we add dtypes
There was a problem hiding this comment.
Yeah, also I think we can be more direct here:
if any(not cudf.api.types.is_numeric(col.dtype) for col in gdf._data._columns)
There was a problem hiding this comment.
Changed it to be more resilient with cudf.api.types._is_non_decimal_numeric_dtype.
galipremsagar
added
2 - In Progress
There was a problem hiding this comment.
Opinion: I think it's best to not include DataFrame etc. in dlpack.py. According to https://blog.cleancoder.com/uncle-bob/2012/08/13/the-clean-architecture.html, dependencies should go inward. to/from_dlpack are user facing functions that accepts any cudf object as input so IMO is outer most layer. This may also help alleviate the circular import issue you have with find_common_type.
|
rerun tests |
|
did a run for cupy/ cuda 11.5 sake. |
Codecov Report
@@ Coverage Diff @@
## branch-21.12 #9585 +/- ##
================================================
- Coverage 10.79% 10.64% -0.15%
================================================
Files 116 117 +1
Lines 18869 19344 +475
================================================
+ Hits 2036 2059 +23
- Misses 16833 17285 +452
Continue to review full report at Codecov.
|
galipremsagar
added
3 - Ready for Review
galipremsagar
added
5 - Ready to Merge
|
@gpucibot merge |
Resolves: #7123
This PR adds a common dtype casting as requested here: #7123 (comment)