Skip to content

CLN: dont consolidate in indexing #34679

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

Merged
merged 3 commits into from
Jun 26, 2020
Merged

CLN: dont consolidate in indexing #34679

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9ac871e
CLN: dont consolidate in indexing
jbrockmendel Jun 9, 2020
bfa2195
Merge branch 'master' of https://github.com/pandas-dev/pandas into pe…
jbrockmendel Jun 21, 2020
d034e94
revert bit
jbrockmendel Jun 21, 2020
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
2 changes: 0 additions & 2 deletions pandas/core/indexing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1676,7 +1676,6 @@ def isetter(loc, v):
ser = v
else:
# set the item, possibly having a dtype change
ser._consolidate_inplace()
Copy link
Member

Choose a reason for hiding this comment

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

this one is a no-op since ser is a series (I think?), so can easily be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

This is my thought too

ser = ser.copy()
ser._mgr = ser._mgr.setitem(indexer=pi, value=v)
ser._maybe_update_cacher(clear=True)
Expand Down Expand Up @@ -1788,7 +1787,6 @@ def isetter(loc, v):
self.obj._check_is_chained_assignment_possible()

# actually do the set
self.obj._consolidate_inplace()
Copy link
Member

Choose a reason for hiding this comment

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

Removing this one will need a performance check, IMO, and also has potential API changes that might need investigation

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this consolidation turns out to be redundant with the self.obj._is_mixed_type check near the start of this method, since also does consolidate_inplace under the hood (though id prefer that it didnt)

and also has potential API changes that might need investigation

I've been thinking about this, since the same issue came up on the last call w/r/t always-view column-indexing. It isn't clear to me what type/amount of "investigation" would satisfy this concern. Do you have something in mind?

self.obj._mgr = self.obj._mgr.setitem(indexer=indexer, value=value)
self.obj._maybe_update_cacher(clear=True)

Expand Down