[REVIEW] Fix dataframe setitem with ndarray types
#10056
Conversation
galipremsagar
added
bug
galipremsagar
added
5 - Ready to Merge
| @@ -1123,7 +1123,15 @@ def __setitem__(self, arg, value): | |||
| for col_name in self._data: | |||
| self._data[col_name][mask] = value | |||
| else: | |||
| if isinstance(value, DataFrame): | |||
| if isinstance(value, (cupy.ndarray, np.ndarray)): | |||
| _setitem_with_dataframe( | |||
There was a problem hiding this comment.
I see the array was being converted to dataframe in order to make use of _setitem_with_dataframe. Ideally I wish we can separate the logic of column selection from this helper and directly replace the columns targeted. But that's out of the scope for sure.
There was a problem hiding this comment.
Yeah I agree, it would be nice to just convert the array to a column without having to deal with all the DataFrame junk but I don't see an easy way to do that at present :(
| if isinstance(value, (cupy.ndarray, np.ndarray)): | ||
| _setitem_with_dataframe( | ||
| input_df=self, | ||
| replace_df=cudf.DataFrame(value), |
There was a problem hiding this comment.
Any chance we can make use of factory method Dataframe._from_data here instead of public ctor?
There was a problem hiding this comment.
I did check this but realized we have the logic that handles ndarray's in DataFrame constructor and will need this type of inputs to go through that code-paths.
There was a problem hiding this comment.
_from_data assumes that the inputs are already columns and it won't call as_column. We could manually call that function and construct a suitable data dict, but again this feels like grounds for a future refactor rather than something to do now. At some point we should review all of cudf to see where we construct Frames via constructor rather than using one of the faster factories, I think many if not most of them can be rewritten.
Codecov Report
@@ Coverage Diff @@
## branch-22.02 #10056 +/- ##
================================================
- Coverage 10.49% 10.41% -0.08%
================================================
Files 119 119
Lines 20305 20540 +235
================================================
+ Hits 2130 2139 +9
- Misses 18175 18401 +226
Continue to review full report at Codecov.
|
|
@gpucibot merge |
|
@gpucibot merge |
Fixes: #9928
This PR fixes 2d array assignment in
setitem