Skip to content

PERF: Improve replace perf #12745

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
Closed
wants to merge 1 commit into from
Closed

PERF: Improve replace perf #12745

wants to merge 1 commit into from

Conversation

sinhrks
Copy link
Member

@sinhrks sinhrks commented Mar 30, 2016

  • tests added / passed
  • passes git diff upstream/master | flake8 --diff
  • whatsnew entry

When .replace is called with dict, replacements are done per value. Current impl try to soft convert the dtype in every replacement, but it is enough to be done in the final replacement.

Bench

-  712.83ms   355.93ms      0.50  replace.replace_convert.time_replace_series_timestamp
-     1.50s   698.21ms      0.46  replace.replace_convert.time_replace_frame_timestamp
-     3.12s   690.48ms      0.22  replace.replace_convert.time_replace_frame_timedelta
-     1.69s   354.83ms      0.21  replace.replace_convert.time_replace_series_timedelta

@sinhrks sinhrks added the Performance Memory or execution speed performance label Mar 30, 2016
@sinhrks sinhrks added this to the 0.18.1 milestone Mar 30, 2016
@jreback
Copy link
Contributor

jreback commented Mar 30, 2016

notice that BoolBlock.replace doesn't have convert arg at all! so not tested / working. can you give a check.

@sinhrks sinhrks force-pushed the replace_perf branch 2 times, most recently from 48637d9 to 31ae2e3 Compare March 30, 2016 14:40
@sinhrks
Copy link
Member Author

sinhrks commented Mar 30, 2016

Ah, found some .replace() conversion looks buggy. I'll submit a separate issue as the fix not looks straightforward.

jreback
jreback reviewed Mar 30, 2016
View reviewed changes
pandas/tests/series/test_replace.py
@@ -0,0 +1,276 @@
# coding=utf-8
# pylint: disable-msg=E1101,W0612
Copy link
Contributor

Choose a reason for hiding this comment

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

great!

@sinhrks
Copy link
Member Author

sinhrks commented Mar 30, 2016

See #12747. Change this PR title to WIP until #12747 is fixed.

@sinhrks sinhrks changed the title PERF: Improve replace perf WIP: PERF: Improve replace perf Mar 30, 2016
@sinhrks sinhrks force-pushed the replace_perf branch 2 times, most recently from dcf2ace to 904d4e1 Compare April 10, 2016 20:05
@jreback jreback removed this from the 0.18.1 milestone Apr 17, 2016
@codecov-io
Copy link

codecov-io commented May 21, 2016 •
edited
Loading

Current coverage is 85.27% (diff: 90.00%)

Merging #12745 into master will decrease coverage by <.01%

@@             master     #12745   diff @@
==========================================
  Files           144        144          
  Lines         50915      50921     +6   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits          43419      43423     +4   
- Misses         7496       7498     +2   
  Partials          0          0

Powered by Codecov. Last update dfeae39...ffc59b0

@jreback
Copy link
Contributor

jreback commented Sep 9, 2016

can you rebase / update?

@sinhrks sinhrks force-pushed the replace_perf branch 2 times, most recently from db1e776 to e16a99f Compare November 21, 2016 22:21
@jreback jreback added the Dtype Conversions Unexpected or buggy dtype conversions label Nov 23, 2016
@jreback
Copy link
Contributor

jreback commented Nov 23, 2016

lgtm. can we merge this independent of the other PR's? (so it could go in 0.19.2)

@sinhrks sinhrks changed the title WIP: PERF: Improve replace perf PERF: Improve replace perf Nov 29, 2016
@sinhrks
Copy link
Member Author

sinhrks commented Nov 29, 2016 •
edited
Loading

It is independent. #12780 fixes some existing bugs separately.

@jreback jreback added this to the 0.19.2 milestone Nov 30, 2016
@jreback jreback closed this in e299560 Nov 30, 2016
@jreback
Copy link
Contributor

jreback commented Nov 30, 2016

thanks!

@sinhrks sinhrks deleted the replace_perf branch November 30, 2016 12:20
jorisvandenbossche pushed a commit that referenced this pull request Dec 15, 2016
@sinhrks @jorisvandenbossche
[Backport #12745] PERF: Improve replace perf
7479d41 
When .replace is called with
`dict`, replacements are done per value. Current impl try to soft
convert the dtype in every replacement, but it is enough to be done in
the final replacement.

Author: sinhrks <[email protected]>

Closes #12745 from sinhrks/replace_perf and squashes the following commits:

ffc59b0 [sinhrks] PERF: Improve replace perf

(cherry picked from commit e299560)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Dtype Conversions Unexpected or buggy dtype conversions Performance Memory or execution speed performance
Projects
None yet
Milestone
0.19.2
Development

Successfully merging this pull request may close these issues.
3 participants
@sinhrks @jreback @codecov-io