BUG: Make xticks from _quarterly_finder() line up better

[elidourado]

When I plot a series of quarterly data that spans more than 11 years, my vertical grid lines get placed a year before where they should, i.e., one year before a year that is divisible by the default annual spacing. Changing one line in the _quarterly_finder() function, from +1 to +1970, fixes this for me. Can anyone please confirm the issue and that this fixes it?

[phofl]

Hi, could you add a test for this?

[elidourado]

Hey, I'm not too familiar with the project's test infrastructure, so I apologize if this isn't too convenient, but here's a standalone test that works for me.

import pandas as pd
import numpy as np
from pandas import Period
from pandas.plotting._matplotlib import converter


def test_quarterly_finder(data):
    vmin = data[0]
    vmax = data[-1]
    (vmin, vmax) = (int(vmin), int(vmax))
    if vmin > vmax:
        (vmin, vmax) = (vmax, vmin)
    span = vmax - vmin + 1
    if span < 45:  # the quarterly finder is only invoke if the span is >= 45
        return
    nyears = span / 4
    (min_anndef, maj_anndef) = converter._get_default_annual_spacing(nyears)
    result = converter._quarterly_finder(data[0], data[-1], 'Q')
    quarters = pd.PeriodIndex(pd.arrays.PeriodArray(
        np.array([x[0] for x in result]), freq='Q'))
    majors = np.array([x[1] for x in result])
    minors = np.array([x[2] for x in result])
    major_quarters = quarters[majors]
    minor_quarters = quarters[minors]
    check_major_years = major_quarters.year % maj_anndef == 0
    check_minor_years = minor_quarters.year % min_anndef == 0
    check_major_quarters = major_quarters.quarter == 1
    check_minor_quarters = minor_quarters.quarter == 1
    assert (np.all(check_major_years) and np.all(check_minor_years)
            and np.all(check_major_quarters) and np.all(check_minor_quarters))


start_date = pd.to_datetime('1947-01-01')
span15 = pd.date_range(start_date, periods=15*4,
                       freq='Q').to_period(freq='Q').asi8
span30 = pd.date_range(start_date, periods=30*4,
                       freq='Q').to_period(freq='Q').asi8
span80 = pd.date_range(start_date, periods=80*4,
                       freq='Q').to_period(freq='Q').asi8
span150 = pd.date_range(start_date, periods=150*4,
                        freq='Q').to_period(freq='Q').asi8

test_quarterly_finder(span15)
test_quarterly_finder(span30)
test_quarterly_finder(span80)
test_quarterly_finder(span150)

[mroeschke]

Please review https://pandas.pydata.org/pandas-docs/stable/development/contributing_codebase.html#test-driven-development-code-writing for adding unit tests. We cannot accept bug fixes without a unit test

[pep8speaks]

Hello @elidourado! 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 2022-07-15 17:28:32 UTC

[mroeschke]

1. Could you put this test in pandas/tests/plotting/test_converter.py?
2. Can this test be simplified at all?

[elidourado]

I moved the test over to that file and also simplified the initial test data generation a bit. I'm not sure how to simplify the test further without losing coverage or sacrificing clarity. Open to guidance, however.

[elidourado]

Let me know what else I can do to get this committed!

[mroeschke]

Could you just use 4 individual asserts? it will make it easier to diagnose if one of the asserts fails in the future

[mroeschke]

Looks like the code checks are failing below with the red x. Please review the logs to see what formatting checks may have failed

[mroeschke]

Looks fairly good. Could you add a note in the whatsnew/1.5.0.rst about the bug fix? (Visualization section)

[elidourado]

Done. I had to make a new visualization section in the 1.5.0 what's new file. There is an existing plotting section. Would it be better to consolidate and move the note there?

[mroeschke]

Thanks for sticking with this @elidourado