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

Jump to bottom

PERF: avoid copy in _obj_with_exclusions, _selected_obj #51090

Merged
merged 5 commits into from
Feb 1, 2023
Merged

PERF: avoid copy in _obj_with_exclusions, _selected_obj #51090

merged 5 commits into from
Feb 1, 2023

Conversation

jbrockmendel
Copy link
Member

  • closes #xxxx (Replace xxxx with the GitHub issue number)
  • Tests added and passed if fixing a bug or adding a new feature
  • All code checks passed.
  • Added type annotations to new arguments/methods/functions.
  • Added an entry in the latest doc/source/whatsnew/vX.X.X.rst file if fixing a bug or adding a new feature.

cc @rhshadrach

import pandas as pd
import numpy as np
import psutil
proc = psutil.Process()

np.random.seed(456123)
df = pd.DataFrame(
    np.random.randn(10_000_000, 5),
    columns=["A", "B", "C", "D", "E"],
)

grps = np.arange(10).repeat(1_000_000)
gb = df.groupby(grps)

gb2 = gb[["C"]]

In [3]: np.shares_memory(gb2._obj_with_exclusions["C"], df["C"])
True   # <- PR
False  # <- main

In [5]: proc.memory_info()
Out[5]: pmem(rss=658894848, vms=36076212224, pfaults=169250, pageins=0)  <- main
Out[5]: pmem(rss=578924544, vms=35993063424, pfaults=149692, pageins=0)  <- PR

@mroeschke mroeschke added Groupby Performance Memory or execution speed performance labels Jan 31, 2023
rhshadrach
rhshadrach reviewed Feb 1, 2023
View reviewed changes
Copy link
Member

@rhshadrach rhshadrach left a comment

Choose a reason for hiding this comment

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

lgtm; I think we can remove a case the logic in _selected_obj, but might be wrong.

pandas/core/groupby/groupby.py
Comment on lines +730 to +735
if self._selection is not None:
if is_hashable(self._selection):
# i.e. a single key, so selecting it will return a Series.
# In this case, _obj_with_exclusions would wrap the key
# in a list and return a single-column DataFrame.
return self.obj[self._selection]
Copy link
Member

Choose a reason for hiding this comment

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

I think any time self.obj[self._selection] results in a Series will be when self is a SeriesGroupBy and self.obj is a Series, which is handled above.

If the frame has duplicate columns, then we can windup in the case where self._selection is hashable and self is a DataFrameGroupBy, but in this case I think self.obj[self_selection] will then agree with self._obj_with_exclusions (both frames).

So I think we can still return obj_with_exclusions here.

Copy link
Member Author

Choose a reason for hiding this comment

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

That logic works for SeriesGroupBy/DataFrameGroupBy, but this code path is shared by Resampler, which defines _gotitem kludgily. Until we change that, test_resample_groupby_agg_listlike would fail if we return obj_with_exclusions here

Copy link
Member Author

Choose a reason for hiding this comment

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

Until we change that

(this is part of why i pinged you about the non-unique columns in agg_dict_like. If we can make sure to always pass subset to _gotitem, i think that makes it a lot easier to de-kludge Resample._gotitem)

@rhshadrach rhshadrach added this to the 2.0 milestone Feb 1, 2023
@rhshadrach
Copy link
Member

Do you think the memory perf improvement is worth a note in the whatsnew?

@jbrockmendel
Copy link
Member Author

Do you think the memory perf improvement is worth a note in the whatsnew?

added

@rhshadrach rhshadrach merged commit 3b94119 into pandas-dev:main Feb 1, 2023
@rhshadrach
Copy link
Member

Thanks @jbrockmendel

@jbrockmendel jbrockmendel deleted the perf-getitem-no-copy branch February 1, 2023 22:03
@jbrockmendel
Copy link
Member Author

i think the describe PR will need rebasing (might want to wait until after #51113 is merged) and it should be good to go

@jorisvandenbossche jorisvandenbossche mentioned this pull request Jan 7, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rhshadrach rhshadrach rhshadrach approved these changes

Assignees
No one assigned
Labels
Groupby Performance Memory or execution speed performance
Projects
None yet
Milestone
2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@jbrockmendel @rhshadrach @mroeschke