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

PERF: Improve replace perf #12745

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
@sinhrks
Member

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 label Mar 30, 2016

@sinhrks sinhrks added this to the 0.18.1 milestone Mar 30, 2016

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Mar 30, 2016

Contributor

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

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

This comment has been minimized.

Show comment
Hide comment
@sinhrks

sinhrks Mar 30, 2016

Member

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

Member

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

View changes

Show outdated Hide outdated pandas/tests/series/test_replace.py
@@ -0,0 +1,276 @@
# coding=utf-8
# pylint: disable-msg=E1101,W0612

This comment has been minimized.

@jreback

jreback Mar 30, 2016

Contributor

great!

@jreback

jreback Mar 30, 2016

Contributor

great!

@sinhrks

This comment has been minimized.

Show comment
Hide comment
@sinhrks

sinhrks Mar 30, 2016

Member

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

Member

sinhrks commented Mar 30, 2016

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

@sinhrks sinhrks changed the title from PERF: Improve replace perf to WIP: PERF: Improve replace perf Mar 30, 2016

@jreback jreback removed this from the 0.18.1 milestone Apr 17, 2016

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io May 21, 2016

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

codecov-io commented May 21, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Sep 9, 2016

Contributor

can you rebase / update?

Contributor

jreback commented Sep 9, 2016

can you rebase / update?

@jreback jreback added the Dtypes label Nov 23, 2016

@jreback

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Nov 23, 2016

Contributor

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

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 from WIP: PERF: Improve replace perf to PERF: Improve replace perf Nov 29, 2016

@sinhrks

This comment has been minimized.

Show comment
Hide comment
@sinhrks

sinhrks Nov 29, 2016

Member

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

Member

sinhrks commented Nov 29, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jreback

jreback Nov 30, 2016

Contributor

thanks!

Contributor

jreback commented Nov 30, 2016

thanks!

@sinhrks sinhrks deleted the sinhrks:replace_perf branch Nov 30, 2016

jorisvandenbossche added a commit that referenced this pull request Dec 15, 2016

[Backport #12745] PERF: Improve replace perf
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 <sinhrks@gmail.com>

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