Skip to content

Performance of get_loc on non-unique MultiIndex #19464

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 3 commits into from
Closed

Performance of get_loc on non-unique MultiIndex #19464

wants to merge 3 commits into from

Conversation

toobaz
Copy link
Member

@toobaz toobaz commented Jan 30, 2018 •
edited
Loading

  • tests passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry

The second commit is related only in the sense that the docs mentioned a PerformanceWarning due to (non-)sorting, while it was actually due to (non-)uniqueness.

@toobaz toobaz changed the title Mi get loc performance Performance of get_loc on non-unique MultiIndex Jan 30, 2018
@toobaz toobaz force-pushed the mi_get_loc_performance branch from 8b777cf to 804af26 Compare January 30, 2018 13:53
@codecov
Copy link

codecov bot commented Jan 30, 2018 •
edited
Loading

Codecov Report

Merging #19464 into master will decrease coverage by 0.23%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #19464      +/-   ##
==========================================
- Coverage   92.06%   91.82%   -0.24%     
==========================================
  Files         169      153      -16     
  Lines       50694    49493    -1201     
==========================================
- Hits        46671    45448    -1223     
- Misses       4023     4045      +22
Flag Coverage Δ
#multiple 90.22% <100%> (-0.25%) ⬇️
#single 41.85% <80%> (-0.47%) ⬇️
Impacted Files Coverage Δ
pandas/core/indexes/multi.py 95.08% <100%> (-0.17%) ⬇️
pandas/util/_doctools.py 0% <0%> (-12.88%) ⬇️
pandas/core/reshape/util.py 90.32% <0%> (-9.68%) ⬇️
pandas/util/_decorators.py 82.25% <0%> (-9.09%) ⬇️
pandas/core/common.py 92.01% <0%> (-5.33%) ⬇️
pandas/core/arrays/base.py 83.95% <0%> (-3.91%) ⬇️
pandas/io/formats/printing.py 89.38% <0%> (-3.71%) ⬇️
pandas/core/groupby/groupby.py 92.66% <0%> (-3.5%) ⬇️
pandas/core/dtypes/missing.py 91.95% <0%> (-1.62%) ⬇️
pandas/core/reshape/tile.py 93.37% <0%> (-1.37%) ⬇️
... and 100 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7725fa0...b72f9c5. Read the comment docs.

jreback
jreback requested changes Jan 31, 2018
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

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

can you add any asv's that are needed and show the results (or if there is already sufficient coverage, can you show the results)

doc/source/whatsnew/v0.23.0.txt Outdated
@@ -381,6 +381,7 @@ Performance Improvements
- :func:`Series` / :func:`DataFrame` tab completion limits to 100 values, for better performance. (:issue:`18587`)
- Improved performance of :func:`DataFrame.median` with ``axis=1`` when bottleneck is not installed (:issue:`16468`)
- Improved performance of :func:`MultiIndex.get_loc` for large indexes, at the cost of a reduction in performance for small ones (:issue:`18519`)
- Improved performance of :func:`MultiIndex.get_loc` for non-unique indexes, which as a consequence does not emit a ``PerformanceWarning`` any more
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the issue number (this PR number if no issue).

@jreback jreback added Performance Memory or execution speed performance MultiIndex labels Jan 31, 2018
@jreback
Copy link
Contributor

jreback commented Feb 24, 2018

rebase when you can and pls show perf

@toobaz
Copy link
Member Author

toobaz commented Feb 24, 2018

rebase when you can and pls show perf

Unfortunately I found out that performance drops dramatically in some cases... until I'm able to fix and merge #19539.

@jreback
Copy link
Contributor

jreback commented Feb 24, 2018

ok #19464 (comment)

keep open then?

@toobaz
Copy link
Member Author

toobaz commented Feb 24, 2018 •
edited
Loading

keep open then?

definitely... I think they are both significant improvements, I just need to understand what's wrong with 2.7 on Windows

@toobaz toobaz force-pushed the mi_get_loc_performance branch from 804af26 to cd7fd6b Compare May 8, 2018 06:40
@toobaz
Copy link
Member Author

toobaz commented May 8, 2018

Rebased, and #19539 did improve things significantly... but results are still not what one expects from a performance fix!

       before           after         ratio
     [2299693d]       [cd7fd6b1]
+         125±6ms          247±1ms     1.98  multiindex_object.Duplicated.time_duplicated_loc
-      14.4±0.3μs       11.9±0.1μs     0.82  multiindex_object.GetLoc.time_string_get_loc

SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.

I will have to investigate more.

@jreback
Copy link
Contributor

jreback commented Jul 28, 2018

can you rebase

@toobaz toobaz force-pushed the mi_get_loc_performance branch from cd7fd6b to b72f9c5 Compare August 2, 2018 22:15
@jreback jreback mentioned this pull request Sep 25, 2018
Merged
4 tasks
@jreback
Copy link
Contributor

jreback commented Nov 1, 2018

closing as stale. if you'd like to continue pls ping.

@jreback jreback closed this Nov 1, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jreback jreback jreback requested changes

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

Successfully merging this pull request may close these issues.
2 participants
@toobaz @jreback