CLN: Ensure that setitem ops don't coerce values #51671
Conversation
| @@ -100,6 +100,6 @@ def test_insert_frame(self): | |||
| # GH#42403 | |||
| df = DataFrame({"col1": [1, 2], "col2": [3, 4]}) | |||
|
|
|||
| msg = r"Expected a 1D array, got an array with shape \(2, 2\)" | |||
| msg = "Expected a one-dimensional object, got a DataFrame with 2 instead." | |||
There was a problem hiding this comment.
"2" here sounds like it is referring to 2 dimensions, which is accurate but i think not intended
There was a problem hiding this comment.
Yikes yes, I forgot to add columns, thx
There was a problem hiding this comment.
Updated, otherwise ok?
I think making this stricter helps in the long run
| return _reindex_for_setitem(value, self.index) | ||
| elif is_dict_like(value): | ||
| # Using a DataFrame would mean coercing values to one dtype | ||
| assert not isinstance(value, DataFrame) |
There was a problem hiding this comment.
no other cases that can be affected by disallowing this here?
There was a problem hiding this comment.
None that I can think of or find. Isetitem iterates over the columns now. Don’t think that we can get here with a DataFrame now
There was a problem hiding this comment.
Not for this PR, but some things I saw while double-checking:
in isetitem isinstance(value, DataFrame) case should we check that len(loc) == len(value.columns)?
_iset_item is only called from _replace_columnwise, where we know we have a Series with matching index, might be simpler to skip sanitize_column?
Might make sense to refactor _set_item back into __setitem__, as the latter isn't that complicated and the former has a name really similar to a bunch of other names
There was a problem hiding this comment.
- Yes we should probably raise if they don't match
- Should be faster at least
- Makes setitem pretty long, but shouldn't really hurt?
phofl
added
the
Indexing
doc/source/whatsnew/vX.X.X.rstfile if fixing a bug or adding a new feature.cc @jbrockmendel
Want to ensure that we don't coerce dtypes accidentally in the future. We shouldn't get here with DataFrames if we can avoid it. Changed the behavior of the single_block case a couple of weeks back.