TST, TYP: _use_dynamic_x

[MarcoGorelli]

This condition (which, from the current plotting test suite, is unreachable) was introduced in #9814, when _use_dynamic_x was written.

Is there any reason why get_period_alias(freq) would return None if freq is a valid time frequency? If so, I'll add a test - else, this PR removes unreachable code.

Added an annotation for data and the return type while I was here - is there a way to annotate ax?

UPDATE

As it turns out (thanks jbrockmendel and jorisvandenbossche), this line can be hit, so I've included a test which does

[jbrockmendel]

Is there any reason why get_period_alias(freq) would return None if freq is a valid time frequency?

ive been looking at this for the last couple days. AFAICT the idea is that not all DateOffset subclasses can be the .freq for a Period, so if we see a DateOffset here, if it does not represent a valid Period, we look to see if there is another offset that does represent a valid Period and is "close enough" by some heuristic.

I think the Easter offset might be an example that returns None

[jbrockmendel]

Looks like theres a lot of untested code in these files, some may be pruneable: https://codecov.io/gh/pandas-dev/pandas/tree/master/pandas/plotting/_matplotlib

[MarcoGorelli]

Hey @jbrockmendel , thanks for your input

I think the Easter offset might be an example that returns None

Yes, it is the case that

get_period_alias('Easter') is None
Out[4]: True

But how would you make a DataFrame where the index has Easter frequency, so that in _use_dynamic_x freq would be equal to Easter before get_period_alias gets called?

I'm struggling with the intuition behind this function - do you know why is it called _use_dynamic_x?

AFAICT, if the data's index's frequency (or, if it has no frequency, the frequency of the subplot, as long as the subplot has lines on it):

• is no more frequent than daily (e.g. weekly), it returns True if the first element in the index is normalised (i.e. starts at midnight)
• is more frequent than daily (e.g. hourly), it returns True if the first element in the index is no more granular than freq

[jorisvandenbossche]

I don't know if it is relevant for the code you are changing, but an example frequency for which you can make a DataFrame with such an index, and for which there is no period alias:

In [20]: freq = pd.offsets.WeekOfMonth(week=2, weekday=2)

In [21]: freq  
Out[21]: <WeekOfMonth: week=2, weekday=2>

In [22]: pd.date_range("2012-01-01", periods=5, freq=freq)
Out[22]: 
DatetimeIndex(['2012-01-18', '2012-02-15', '2012-03-21', '2012-04-18',
               '2012-05-16'],
              dtype='datetime64[ns]', freq='WOM-3WED')

In [23]: pd.tseries.frequencies.get_period_alias(freq) is None
Out[24]: True

[MarcoGorelli]

Thanks @jorisvandenbossche ! Yes, that would be relevant - I'll change this PR so there's a test which hits these lines

[MarcoGorelli]

@jorisvandenbossche @jbrockmendel thanks for your help here

I've added a test which uses WeekOfMonth, and it does indeed hit the previously uncovered lines of code.

But when I try replacing WeekOfMonth with Easter, i.e.

        freq = Easter()
        bts = tm.makeTimeSeries(5).asfreq(freq)
        _, ax = self.plt.subplots()
        bts.plot(ax=ax)

then when we get here, we have

freq
Out[2]: <Easter>
get_period_alias(freq)
Traceback (most recent call last):
  File "/home/marco/.conda/envs/pandas-dev/lib/python3.8/site-packages/IPython/core/interactiveshell.py", line 3331, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-3-1cfff0cb20d2>", line 1, in <module>
    get_period_alias(freq)
  File "/home/marco/pandas-dev/pandas/plotting/_matplotlib/timeseries.py", line 167, in get_period_alias
    freq = freq.rule_code
  File "pandas/_libs/tslibs/offsets.pyx", line 532, in pandas._libs.tslibs.offsets.BaseOffset.rule_code.__get__
  File "pandas/_libs/tslibs/offsets.pyx", line 528, in pandas._libs.tslibs.offsets.BaseOffset._prefix.__get__
NotImplementedError: Prefix not defined

which was reported in #26921

[jbrockmendel]

the annotation looks right, havent looked closely at the tests

[pep8speaks]

Hello @MarcoGorelli! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-06-04 13:19:04 UTC

[jreback]

lgtm. @jorisvandenbossche ok here?

[jorisvandenbossche]

Thanks @MarcoGorelli !