Skip to content
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: speed up PeriodArray creation by exposing dayfirst/yearfirst params #24118

Merged
merged 2 commits into from Dec 29, 2018

Conversation

Projects
None yet
5 participants
@qwhelan
Copy link
Contributor

commented Dec 5, 2018

As much of the time creating a PeriodArray from ints is actually spent importing/querying get_option('display.date_dayfirst') and its yearfirst cousin, this PR exposes those parameters in Period.__new__() to allow them to be queried once per array creation.

This yields a ~15x speedup:

asv compare upstream/master HEAD -s --sort ratio

Benchmarks that have improved:

       before           after         ratio
     [210538e4]       [32288230]
     <period_dayfirst~1>       <period_dayfirst>
-       102±0.9ms       7.08±0.2ms     0.07  period.PeriodIndexConstructor.time_from_ints('D', True)
-         101±2ms       6.56±0.3ms     0.06  period.PeriodIndexConstructor.time_from_ints('D', False)
  • closes #xxxx
  • tests added / passed
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff
  • whatsnew entry
@pep8speaks

This comment has been minimized.

Copy link

commented Dec 5, 2018

Hello @qwhelan! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on December 29, 2018 at 09:59 Hours UTC

@qwhelan qwhelan referenced this pull request Dec 5, 2018

Closed

WIP: speed up Period creation #23500

0 of 4 tasks complete
Show resolved Hide resolved pandas/_libs/tslibs/period.pyx Outdated
@jorisvandenbossche

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

Fine by me. Elsewhere we're trying to move towards fewer arguments in the constructors. Should we consider a dedicated constructor for this? (not necessarily for this PR)

I am personally -1 on adding this to the Period constructor.
(They are also not present on the Timestamp constructor. They are on DatetimeIndex, but I would rather see them deprecated there, in favor of to_datetime for all non-default string parsing).

I also find it strange that a display option actually determines how an input is parsed. I would rather deprecate that ability (as I understand this is what makes it slower?).

@qwhelan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

I also find it strange that a display option actually determines how an input is parsed. I would rather deprecate that ability (as I understand this is what makes it slower?).

Yeah, we're spending 90%+ of runtime importing/querying these display variables. Unfortunately, this same code path is used for handling DatetimeIndex slicing, so deprecation is likely to be tricky.

The caveat on the above is that this speedup is only when creating a PeriodArray directly from integers. This was not covered by our asv suite until the past week, but is leveraged in places like get_indexer_non_unique().

Regarding the name of the option, it seems the intent was for it to cover display + parsing, but only parsing was ever implemented (per #11501 (comment) )

@jreback

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2018

@qwhelan ok adding the benchmarks, but yea let's not mess with the Period constructor. where does the get_option things get called now that is slowings things down?

@qwhelan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

@jreback Period.__new__() calls _lib.parsers.parse_datetime_string without passing yearfirst or dayfirst. When either of these are None, parse_datetime_string imports get_option() and calls it for each missing parameter. This import is done inside the function due to a cyclic dependency and accounts for ~95% of the runtime in simple cases. The gains seen here are mostly due to eliminating an import-per-Period object (the rest is eliminating duplicative get_option() calls).

Given the desire to keep the signature the same, I dug into breaking the import cycle and think I found a sensible way to break it - this yields a 7x improvement (down from 15x). The short description of the cycle is that importing get_option() pulls in essentially all cython extensions via pandas.io.formats.printing, which imports is_sequence() from pandas._libs.lib. My thinking is to move the is_*() functions into a pandas._libs.inference.pyx module analogous to pandas.core.dtypes.inference.py (alternatively, we could guard the import in pandas.io.formats.printing). This breaks the cycle, allowing the get_option import to be moved into the global scope and lets us have most of the speedup without any API changes.

@jreback

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

@qwhelan look at how the import is deferred in pandas.io.pytables, e.g. _tables(). not super petty but should work here. basically

_get_option = None



def get_option():
     global _get_option
     if _get_option is None:
          from pandas.core.config_init import get_option
          _get_option = get_option
     return _get_option

then you can use it like

day_first = get_option().get(....)

@qwhelan qwhelan force-pushed the qwhelan:period_dayfirst branch from 3228823 to c779265 Dec 27, 2018

@codecov

This comment has been minimized.

Copy link

commented Dec 27, 2018

Codecov Report

Merging #24118 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24118      +/-   ##
==========================================
+ Coverage   92.32%   92.32%   +<.01%     
==========================================
  Files         166      166              
  Lines       52298    52298              
==========================================
+ Hits        48285    48286       +1     
+ Misses       4013     4012       -1
Flag Coverage Δ
#multiple 90.74% <ø> (ø) ⬆️
#single 43.05% <ø> (-0.01%) ⬇️
Impacted Files Coverage Δ
pandas/util/testing.py 87.84% <0%> (+0.09%) ⬆️

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 80295f9...bd9eddb. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Dec 27, 2018

Codecov Report

Merging #24118 into master will decrease coverage by 0.1%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #24118      +/-   ##
==========================================
- Coverage   92.31%    92.2%   -0.11%     
==========================================
  Files         163      162       -1     
  Lines       51987    51703     -284     
==========================================
- Hits        47990    47674     -316     
- Misses       3997     4029      +32
Flag Coverage Δ
#multiple 90.6% <ø> (-0.12%) ⬇️
#single 43.03% <ø> (+0.03%) ⬆️
Impacted Files Coverage Δ
pandas/io/s3.py 0% <0%> (-86.37%) ⬇️
pandas/io/parquet.py 76.92% <0%> (-7.7%) ⬇️
pandas/core/arrays/base.py 96.77% <0%> (-0.84%) ⬇️
pandas/compat/numpy/function.py 87.13% <0%> (-0.78%) ⬇️
pandas/io/common.py 72.09% <0%> (-0.78%) ⬇️
pandas/core/indexes/category.py 97.9% <0%> (-0.76%) ⬇️
pandas/core/indexes/interval.py 94.73% <0%> (-0.53%) ⬇️
pandas/core/tools/datetimes.py 85.27% <0%> (-0.49%) ⬇️
pandas/util/testing.py 87.41% <0%> (-0.43%) ⬇️
pandas/core/dtypes/concat.py 96.63% <0%> (-0.43%) ⬇️
... and 39 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 3086e0a...c779265. Read the comment docs.

@qwhelan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 27, 2018

@jreback Eliminated the constructor changes and replaced with your suggestion. Also added a new benchmark as the int case before was hitting a special case. Here's the new benchmark comparison:

$ asv compare upstream/master HEAD --sort ratio -s

Benchmarks that have improved:

       before           after         ratio
     [3086e0a1]       [b0a82749]
-        394±20μs          300±8μs     0.76  period.PeriodConstructor.time_period_constructor('D', False)
-        414±10μs          312±8μs     0.75  period.PeriodConstructor.time_period_constructor('D', True)
-         192±3ms         99.2±3ms     0.52  period.PeriodIndexConstructor.time_from_ints_daily('D', False)
-         194±3ms         95.6±2ms     0.49  period.PeriodIndexConstructor.time_from_ints_daily('D', True)
-         108±2ms         16.8±1ms     0.16  period.PeriodIndexConstructor.time_from_ints('D', False)
-         109±2ms         15.3±1ms     0.14  period.PeriodIndexConstructor.time_from_ints('D', True)
@jreback
Copy link
Contributor

left a comment

does this close the original issue? if so can you add a whatsnew note (or just add onto a prior one which I think is there)

Show resolved Hide resolved pandas/_libs/tslibs/parsing.pyx Outdated

@qwhelan qwhelan force-pushed the qwhelan:period_dayfirst branch 2 times, most recently from 1b3997f to c5089cd Dec 28, 2018

@qwhelan

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

@jreback Updated with whatsnew and comment. There's no specific issue open for this - I believe I stumbled across it while reviewing asv results.

@qwhelan qwhelan force-pushed the qwhelan:period_dayfirst branch 2 times, most recently from 2a49371 to c94f780 Dec 28, 2018

@qwhelan qwhelan force-pushed the qwhelan:period_dayfirst branch from c94f780 to bd9eddb Dec 29, 2018

@jreback jreback added this to the 0.24.0 milestone Dec 29, 2018

@jreback jreback merged commit 1d760af into pandas-dev:master Dec 29, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
pandas-dev.pandas Build #20181229.14 succeeded
Details
@jreback

This comment has been minimized.

Copy link
Contributor

commented Dec 29, 2018

thanks @qwhelan nice!

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Pingviinituutti added a commit to Pingviinituutti/pandas that referenced this pull request Feb 28, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.