PERF: For GH23814, return early in Categorical.__init__ #23888
Conversation
|
Hello @eoveson! Thanks for submitting the PR.
|
eoveson
changed the title
Codecov Report
@@ Coverage Diff @@
## master #23888 +/- ##
==========================================
+ Coverage 92.29% 92.3% +0.01%
==========================================
Files 161 161
Lines 51498 51556 +58
==========================================
+ Hits 47530 47590 +60
+ Misses 3968 3966 -2
Continue to review full report at Codecov.
|
gfyoung
added
Performance
|
@eoveson : Thanks for the PR! Can you run |
pandas/core/arrays/categorical.py
Outdated
| @@ -314,6 +314,16 @@ class Categorical(ExtensionArray, PandasObject): | |||
| def __init__(self, values, categories=None, ordered=None, dtype=None, | |||
| fastpath=False): | |||
|
|
|||
| # GH23814, for perf, if no optional params used and values already an | |||
There was a problem hiding this comment.
i think we can just move this down to where the fastpath check is now; you can add this on i think. this constructor is already amazing too complicated.
There was a problem hiding this comment.
I think at that point, the arg dtype, and maybe categories, will be set. I wanted to only use this early return if none of the optional args were specified (I believe @TomAugsperger was suggesting this in the issue thread).
There was a problem hiding this comment.
@eoveson I would still like to investiagte consolidating some of this code. This is a very complicated constructor and more code is not great here. See if you can add it lower down, even if its slightly lower perf.
There was a problem hiding this comment.
@jreback , Ok let me look into this and see if I can consolidate some of the code..
There was a problem hiding this comment.
Updated, please check it out when you get a chance
|
@gfyoung -- yes, ran a subset of the asv suite (tried to target categorical), I can run the entire suite also. It reported no significant difference (maybe because there was no existing test for this scenario? -- which is why I added the new perf test) |
The test output should list all performance tests that were run. If it's not there, create a new branch off |
@gfyoung, Yes, I saw the test I added show in the output when I ran the command I mentioned. Should I run all of the asv tests (tried running all asv tests, but it failed when it was about 1/3 done with a file access error for a temporary file), or should I target categorical tests? Btw, this is the error I saw when trying to run all the asv tests: [ 32.67%] ▒▒▒ Running (index_object.Indexing.time_get_loc--).
Traceback (most recent call last):
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\runpy.py", line 193, in _run_module_as_main
"__main__", mod_spec)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\runpy.py", line 85, in _run_code
exec(code, run_globals)
File "C:\Users\erikov\AppData\Local\Continuum\anaconda3\scripts\asv.exe\__main__.py", line 9, in <module>
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\main.py", line 38, in main
result = args.func(args)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\commands\__init__.py", line 49, in run_from_args
return cls.run_from_conf_args(conf, args)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\commands\continuous.py", line 72,in run_from_conf_args
launch_method=args.launch_method, **kwargs
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\commands\continuous.py", line 106, in run
_returns=run_objs, _machine_file=_machine_file)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\commands\run.py", line 406, in run
launch_method=launch_method)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\runner.py", line 349, in run_benchmarks
cwd=cache_dir)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\runner.py", line 515, in run_benchmark
cwd=cwd)
File "c:\users\erikov\appdata\local\continuum\anaconda3\lib\site-packages\asv\runner.py", line 647, in _run_benchmark_single_param
os.remove(result_file.name)
PermissionError: [WinError 5] Access is denied: 'C:\\Users\\erikov\\AppData\\Local\\Temp\\tmpfq5htpg5' |
Running the Categorical tests is fine. I'm concerned though...you didn't see any noticeable improvement in performance, even with your newly added test? |
|
Well, this change only helps in the case that they have passed in an existing instance of Categorical to Categorical.init, and used no optional params. Not sure how common that would be in the tests? I didn't see that test case in the file I added the test case to. But I'm also not exactly sure how this asv test suite works. How does it get a baseline to compare against (since machine specs are different)? Am I supposed to create a baseline on my machine without my changes, and then run with my changes? If so, I didn't do that. I simply ran the asv command I mentioned, so I'm not sure if I'm doing things correctly.. |
Right, but didn't you say you saw no substantial changes in performance?
Can you copy / paste the output of your ASV? |
|
I see now that you specifically mentioned the test I added. So I should have seen a difference for that test, so I guess I need to run with that new test, but without my real changes to init to create the baseline first? |
Exactly. That's why I said earlier:
|
Ah, makes sense, thanks. I'll compare the two branches and get back to you (I'll first work on the code consolidation requested by jreback since that may impact things). |
doc/source/whatsnew/v0.24.0.rst
Outdated
| @@ -1150,7 +1150,7 @@ Performance Improvements | |||
| - Improved performance of :func:`pd.concat` for `Series` objects (:issue:`23404`) | |||
| - Improved performance of :meth:`DatetimeIndex.normalize` and :meth:`Timestamp.normalize` for timezone naive or UTC datetimes (:issue:`23634`) | |||
| - Improved performance of :meth:`DatetimeIndex.tz_localize` and various ``DatetimeIndex`` attributes with dateutil UTC timezone (:issue:`23772`) | |||
|
|
|||
| - Improved performance of :meth:`Categorical.__init__` (:issue:`23814`) | |||
There was a problem hiding this comment.
say constructor rather than referring to __init__
There was a problem hiding this comment.
@jreback , updated doc string, and also added asv test that exercises the code (first one didn't, but left it since still useful) (you can see my comment about asv results to gfyoung)
|
@gfyoung , it turns out that asv test I added previously was not exercising the code (I should have been passing in a Series rather than Categorical to the constructor). I added a new asv test for this (but left the other one since it could still be useful). I re-ran asv, and did see a significant difference reported in that newly added test and one other test. I didn't expect that other test to change, so I re-ran the same command and looking at the numbers that test doesn't change much. (However, the reporting no longer says my newly added test shows significant difference in the second run of the command, even though I do see the same difference from the first run of the command). So I think things are ok now, but pasting the output here so you can take a look. Here is the first execution of the command (and then down below you will see the second one): $ asv continuous -f 1.1 upstream/master category-perf -b categorical [ 52.27%] ▒▒▒ ...oricals.CategoricalSlicing.time_getitem_list ok monotonic_incr 679~0us [ 52.27%] ▒▒▒ ================ ============ [ 53.03%] ▒▒▒ ...ls.CategoricalSlicing.time_getitem_list_like ok monotonic_incr 12.5 [ 53.03%] ▒▒▒ ================ ========== [ 53.79%] ▒▒▒ ...icals.CategoricalSlicing.time_getitem_scalar ok monotonic_incr 5.05 [ 53.79%] ▒▒▒ ================ ========== [ 54.55%] ▒▒▒ ...ricals.CategoricalSlicing.time_getitem_slice ok monotonic_incr 3.93 [ 54.55%] ▒▒▒ ================ ========== [ 55.30%] ▒▒▒ categoricals.Concat.time_concat 7.81▒0ms [ 68.94%] ▒▒▒ categoricals.Rank.time_rank_int 11.7▒4ms [ 75.00%] ▒ For pandas commit 3e01c38 <master^2> (round 2/2): [ 77.27%] ▒▒▒ ...oricals.CategoricalSlicing.time_getitem_list ok monotonic_incr 625 [ 77.27%] ▒▒▒ ================ =========== [ 78.03%] ▒▒▒ ...ls.CategoricalSlicing.time_getitem_list_like ok monotonic_incr 12.5 [ 78.03%] ▒▒▒ ================ ========== [ 78.79%] ▒▒▒ ...icals.CategoricalSlicing.time_getitem_scalar ok monotonic_decr 6.27 [ 78.79%] ▒▒▒ ================ ========== [ 79.55%] ▒▒▒ ...ricals.CategoricalSlicing.time_getitem_slice ok monotonic_incr 7.82 [ 79.55%] ▒▒▒ ================ ========== [ 80.30%] ▒▒▒ categoricals.Concat.time_concat 7.81▒0ms [ 93.94%] ▒▒▒ categoricals.Rank.time_rank_int 7.81▒4ms
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY. $ asv continuous -f 1.1 upstream/master category-perf -b categorical [ 52.27%] ▒▒▒ ...oricals.CategoricalSlicing.time_getitem_list ok monotonic_incr 601 [ 52.27%] ▒▒▒ ================ =========== [ 53.03%] ▒▒▒ ...ls.CategoricalSlicing.time_getitem_list_like ok monotonic_incr 6.41 [ 53.03%] ▒▒▒ ================ ========== [ 53.79%] ▒▒▒ ...icals.CategoricalSlicing.time_getitem_scalar ok monotonic_incr 4.51 [ 53.79%] ▒▒▒ ================ ========== [ 54.55%] ▒▒▒ ...ricals.CategoricalSlicing.time_getitem_slice ok monotonic_incr 7.16 [ 54.55%] ▒▒▒ ================ ========== [ 55.30%] ▒▒▒ categoricals.Concat.time_concat 7.81▒0ms [ 68.94%] ▒▒▒ categoricals.Rank.time_rank_int 7.81▒3ms [ 75.00%] ▒ For pandas commit 3e01c38 <master^2> (round 2/2): [ 77.27%] ▒▒▒ ...oricals.CategoricalSlicing.time_getitem_list ok monotonic_incr 601 [ 77.27%] ▒▒▒ ================ =========== [ 78.03%] ▒▒▒ ...ls.CategoricalSlicing.time_getitem_list_like ok monotonic_incr 11.4 [ 78.03%] ▒▒▒ ================ ========== [ 78.79%] ▒▒▒ ...icals.CategoricalSlicing.time_getitem_scalar ok monotonic_incr 4.37 [ 78.79%] ▒▒▒ ================ ========== [ 79.55%] ▒▒▒ ...ricals.CategoricalSlicing.time_getitem_slice ok monotonic_incr 7.23 [ 79.55%] ▒▒▒ ================ ========== [ 80.30%] ▒▒▒ categoricals.Concat.time_concat 7.81▒0ms [ 93.94%] ▒▒▒ categoricals.Rank.time_rank_int 7.81▒3ms BENCHMARKS NOT SIGNIFICANTLY CHANGED. |
|
@eoveson can you show a before / after using timeit in ipython |
For sure. Before my change: In [2]: s = pd.Series(list('abcd') * 1000000).astype('category')
In [3]: %timeit s == 'a'
25.7 ms ± 409 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)
In [4]: %timeit s.cat.codes == s.cat.categories.get_loc('a')
3.29 ms ± 70.6 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)After change: In [6]: s = pd.Series(list('abcd') * 1000000).astype('category')
In [7]: %timeit s == 'a'
5.24 ms ± 97.7 µs per loop (mean ± std. dev. of 7 runs, 100 loops each)
In [8]: %timeit s.cat.codes == s.cat.categories.get_loc('a')
3.28 ms ± 70 µs per loop (mean ± std. dev. of 7 runs, 100 loops each) |
|
thanks @eoveson |
git diff upstream/master -u -- "*.py" | flake8 --diff