pandas (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

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

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?).

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) )

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

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

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

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

Choose a reason for hiding this comment

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

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)

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

Pingviinituutti pushed a commit to Pingviinituutti/pandas that referenced this pull request

Feb 28, 2019