Skip to content

PERF: Use generator expression for Blocks.replace_list #50778

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.

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 11 commits into from
Feb 28, 2023

Conversation

lithomas1
Copy link
Member

@lithomas1 lithomas1 commented Jan 16, 2023

  • 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.
       before           after         ratio
     [852518eb]       [3902ad1a]
     <maybe-convert-less-mem^2>       <replace-use-less-mem>
-      2.68±0.02s       2.35±0.03s     0.88  series_methods.Replace.time_replace_dict(1000)
-       2.73±0.1s       2.38±0.04s     0.87  series_methods.Replace.time_replace_list(1000)
-            203M             105M     0.52  series_methods.Replace.peakmem_replace_dict(100)
-            203M             105M     0.52  series_methods.Replace.peakmem_replace_list(100)
-           1.11G             106M     0.10  series_methods.Replace.peakmem_replace_list(1000)
-           1.11G             106M     0.10  series_methods.Replace.peakmem_replace_dict(1000)

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.

inplace=True is not covered by benchmarks but should be unchanged.

else:
# GH#38086 faster if we know we dont need to check for regex
masks = [missing.mask_missing(values, s[0]) for s in pairs]
masks = (
extract_bool_array(missing.mask_missing(values, s[0])) for s in pairs
Copy link
Member

Choose a reason for hiding this comment

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

is there a chance of an API change here (not necessarily a bad one) with something like

src_list = [1, 2, 3]
dest_list = [2, 3, 4]
values = np.array([1, 2, 3])

IIUC in the status quo we end up with [2, 3, 4] but with this we'd end up with all-4s?

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, looks like this would be invalid for inplace=True. Good catch!

I'll also add a test case, since it looks like this isn't caught by CI.

@mroeschke mroeschke added the Performance Memory or execution speed performance label Jan 17, 2023
@lithomas1 lithomas1 marked this pull request as ready for review February 2, 2023 22:08
@jbrockmendel
Copy link
Member

Might address #6697?

@lithomas1
Copy link
Member Author

Might address #6697?

Only partially. This optimization only works for inplace=False. We'd need to write our own replace function, to avoid generating the masks.

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.

Needs rebase, otherwise LGTM

@lithomas1 lithomas1 merged commit 4179a44 into pandas-dev:main Feb 28, 2023
@lithomas1 lithomas1 deleted the replace-use-less-mem branch February 28, 2023 22:15
@lithomas1 lithomas1 added this to the 2.0 milestone Mar 1, 2023
@lithomas1
Copy link
Member Author

lithomas1 commented Mar 1, 2023

@meeseeksdev backport 2.x

@lithomas1
Copy link
Member Author

@meeseeksdev backport 2.0.x

meeseeksmachine pushed a commit to meeseeksmachine/pandas that referenced this pull request Mar 1, 2023
phofl pushed a commit that referenced this pull request Mar 1, 2023
…r Blocks.replace_list) (#51714)

Backport PR #50778: PERF: Use generator expression for Blocks.replace_list

Co-authored-by: Thomas Li <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Performance Memory or execution speed performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants