Skip to content

Conversation

phofl
Copy link
Member

@phofl phofl commented Dec 10, 2022

@phofl phofl added Indexing Related to indexing on series/frames, not to indexes themselves DataFrame DataFrame data structure NA - MaskedArrays Related to pd.NA and nullable extension arrays labels Dec 10, 2022
Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good, just got a suggestion to improve testing

Comment on lines +3829 to +3831
for i, idx in enumerate(loc):
arraylike = self._sanitize_column(value.iloc[:, i])
self._iset_item_mgr(idx, arraylike, inplace=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the tests you've added, i and idx are always the same - is it possible to add a test where i and idx differ?

Copy link
Member

@MarcoGorelli MarcoGorelli Dec 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as in, if I changed this to

                arraylike = self._sanitize_column(value.iloc[:, i])
                self._iset_item_mgr(i, arraylike, inplace=False)

then there should be a test which fails

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, adjusted one of the tests

Copy link
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me! Over to @jbrockmendel

@MarcoGorelli MarcoGorelli added this to the 2.0 milestone Dec 30, 2022
df = DataFrame([[1, 2, 3], [4, 5, 6]])
rhs = DataFrame([[11, 12], [13, 14]], dtype="Int64")

df.isetitem([0, 1], rhs)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick can we make a test file for isetitem. otherwise lgtm

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Member

@jbrockmendel jbrockmendel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mroeschke mroeschke merged commit 02d22f3 into pandas-dev:main Jan 4, 2023
@mroeschke
Copy link
Member

Thanks @phofl

@phofl phofl deleted the 49922 branch January 4, 2023 13:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DataFrame DataFrame data structure Indexing Related to indexing on series/frames, not to indexes themselves NA - MaskedArrays Related to pd.NA and nullable extension arrays
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BUG: frame.isetitem incorrectly casts Int64 columns to dtype object
4 participants