Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TST: parametrize CoW indexing tests for extension (nullable) dtypes #51208

Merged
merged 6 commits into from
Feb 20, 2023
Merged

TST: parametrize CoW indexing tests for extension (nullable) dtypes #51208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bc3963c
TST: parametrize CoW indexing tests for nullable dtypes
jorisvandenbossche Feb 6, 2023
eab36b6
Merge remote-tracking branch 'upstream/main' into cow-tests-parametri…
jorisvandenbossche Feb 8, 2023
e1a1c3e
update tests
jorisvandenbossche Feb 8, 2023
a13399b
Merge remote-tracking branch 'upstream/main' into cow-tests-parametri…
jorisvandenbossche Feb 16, 2023
2fd543e
Merge branch 'main' into cow-tests-parametrize-nullable
phofl Feb 16, 2023
86eefc6
fix test
jorisvandenbossche Feb 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pandas/core/internals/managers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -827,8 +827,9 @@ def _slice_take_blocks_ax0(
# A non-consolidatable block, it's easy, because there's
# only one item and each mgr loc is a copy of that single
# item.
deep = not (only_slice or using_copy_on_write())
for mgr_loc in mgr_locs:
newblk = blk.copy(deep=not only_slice)
newblk = blk.copy(deep=deep)
Copy link
Member

Choose a reason for hiding this comment

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

Could this have caused test_reindex_copies_ea to fail? Not sure but can't see how this test could have been impacted otherwise

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, yes, that's the test that I added in the precursor PR. With CoW we now don't copy (the test was added in the assumption we always copy with copy=True, while now we don't copy in case of CoW)

Copy link
Member Author

Choose a reason for hiding this comment

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

But, for most other methods I think we are actually still honoring the copy=True keyword .. Should we do that here as well for reindex with EAs?

(although we should also stop honoring the copy=True keyword at some point, when CoW is enabled, xref #50535)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I would prefer not honouring copy anymore for CoW. I'll start tracking the cases down where we are still using copy.

newblk.mgr_locs = BlockPlacement(slice(mgr_loc, mgr_loc + 1))
blocks.append(newblk)

Expand Down
Loading