PERF: speed up PeriodArray creation by exposing dayfirst/yearfirst params

[qwhelan]

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]

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

[jorisvandenbossche]

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]

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]

@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]

@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]

@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(....)

[codecov]

Codecov Report

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

@@            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]

Codecov Report

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

@@            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]

@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]

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)

[qwhelan]

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

[jreback]

thanks @qwhelan nice!