PDEP6 implementation pt 1: block.setitem, block.putmask #50626
Conversation
| @@ -206,7 +206,20 @@ def test_convert_dtypes( | |||
| # Test that it is a copy | |||
| copy = series.copy(deep=True) | |||
|
|
|||
| result[result.notna()] = np.nan | |||
| if result.notna().sum() > 0 and result.dtype in [ | |||
There was a problem hiding this comment.
doesnt have to be for this PR, but long-term should we also warn/raise in the no-op case?
There was a problem hiding this comment.
as in, warn if
ser[empty_mask] = incompatible_value?
I wouldn't think that would be necessary, but we can discuss it
|
9/21 files viewed! Getting there... |
| # note: commented-out in the EA case too | ||
| # FIXME: don't leave commented-out | ||
| # with tm.assert_produces_warning(FutureWarning, match=msg): | ||
| # ser[:] = invalid |
There was a problem hiding this comment.
this doesnt match the commented-out code in the masked tests. any chance it can be un-commented?
| or is_complex_dtype(df.dtypes.iloc[0]) | ||
| or is_object_dtype(df.dtypes.iloc[0]) | ||
| or is_datetime64_dtype(df.dtypes.iloc[0]) # TODO this one should warn | ||
| or is_timedelta64_dtype(df.dtypes.iloc[0]) # TODO this one should warn |
There was a problem hiding this comment.
are these TODOs things you want to do for this PR or follow-ups?
There was a problem hiding this comment.
this one addresses them: #53405
| @@ -1851,3 +1870,54 @@ def test_setitem_dict_and_set_disallowed_multiindex(self, key): | |||
| ) | |||
| with pytest.raises(TypeError, match="as an indexer is not supported"): | |||
| df.loc[key] = 1 | |||
|
|
|||
|
|
|||
| class TestSetitemValidation: | |||
There was a problem hiding this comment.
is there a viable way to share this with the analogous copy/pasted Series tests?
There was a problem hiding this comment.
maybe, but I can't think of anything that wouldn't overcomplicate it - any suggestions?
| @@ -189,6 +189,8 @@ def _check_set(df, cond, check_dtypes=True): | |||
| with pytest.raises(TypeError, match=msg): | |||
| df > 0 | |||
| return | |||
| if df is mixed_int_frame: | |||
There was a problem hiding this comment.
yup: pandas/tests/frame/indexing/test_where.py::TestDataFrameIndexingWhere::test_where_set[mixed_int]
| # GH#38303 | ||
| ser = Series([0, 0]) | ||
| ser.loc[0] = Series([42], index=[index]) |
There was a problem hiding this comment.
unrelatedly, this looks sketchy since the RHS is a sequence and the LHS is a scalar
|
small comments, otherwise LGTM |
| f"Setting an item of incompatible dtype is deprecated " | ||
| "and will raise in a future error of pandas. " | ||
| f"Value '{other}' has dtype incompatible with {self.dtype}, " | ||
| "please explicitly cast to a compatible dtype first.", |
There was a problem hiding this comment.
Thinking aloud. It would be difficult to suggest which dtype to cast? I guess object would be safest though
There was a problem hiding this comment.
in practice i think it will almost always be x->object or int->float
There was a problem hiding this comment.
maybe - but I'd suggest doing this separately if it's OK, this has already been open since January 😄 reckon we can get this in for 2.1? It should be pretty quick to rebase #53405, can do that right after this one
There was a problem hiding this comment.
Yep this would be fine as a follow up
|
Looks pretty good to me too. Should this have a whatsnew note for 2.1? And is it okay if not all of PDEP6 changes don't make it into 2.1? |
|
Probably best if the whatsnew is added in #53405 then. Is that PR pretty close? |
|
yeah it should be ready, I just need to rebase it after this one - but it doesn't look like there's too many conflicts to resolve fortunately |
|
Thanks @MarcoGorelli |
|
nice! |
This is pt 1. More changes would be needed to fully complete PDEP6 (
eablock.setitemandeablock.putmaskfor a start), but I'd suggest that we do this in stages. This PR is already pretty bigsome of this can be factored out, I'll do that first
pt2: #53405