8000 FIX: properly handle dates when intmult is true by jklymak · Pull Request #17727 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

FIX: properly handle dates when intmult is true #17727

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jul 3, 2020
Merged

FIX: properly handle dates when intmult is true #17727

merged 3 commits into from
Jul 3, 2020

Conversation

jklymak
Copy link
Member
@jklymak jklymak commented Jun 23, 2020

PR Summary

Currently, if interval_multiples=True (now the default), and a long enough numbers of days is being plotted, the locator finds the 1st and 22nd of the month as dates. That makes sense if interval_multiples=False, but is silly for interval_multiples=True.

dates = np.arange('2001-01-10', '2001-05-23', dtype='datetime64[D]')
y = np.sin(dates.astype(float) / 10)
fig, ax = plt.subplots(constrained_layout=True)

#plt.rcParams['date.converter'] = 'concise'
#plt.rcParams['date.interval_multiples'] = True
ax.plot(dates, y)

plt.show()

picks dates 1, and 22 of the month. This PR fixes that to be 1 and 15, and to not raise a warning:

Before

Before11

After

After1

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@jklymak jklymak force-pushed the fix-proper-day-of-month branch from 5560a6a to 5b48457 Compare June 23, 2020 00:14
@jklymak
Copy link
Member Author
jklymak commented Jun 23, 2020

Codecov is made because we never call the warning in the tests. I'm not sure what case this would actually get called, but could try if folks think its necessary.

@tacaswell
Copy link
Member

How pathological of an example do you need to trigger the warning? We have had cases of un-exercised warnings bit-rotting and turning into exceptions due to missing variables / bad formatting / etc so a test would be better, but not if it is going to be a huge amount of work.

@jklymak jklymak force-pushed the fix-proper-day-of-month branch from 5b48457 to fed1db7 Compare June 23, 2020 04:20
@jklymak
Copy link
Member Author
jklymak commented Jun 23, 2020

OK, this warning comes up if we don't satisfy the maxticks constraint, but then it tells the user to change something they can't change anyhow. I think we should just drop the warning.

@QuLogic
Copy link
Member
QuLogic commented Jun 23, 2020

but then it tells the user to change something they can't change anyhow.

What do you mean? It's an instance attribute. It's even documented in the class docstring.

@jklymak
Copy link
Member Author
jklymak commented Jun 23, 2020

Ooops! Fair enough....

@jklymak
Copy link
Member Author
jklymak commented Jun 23, 2020

So what happens here is that there is special logic for the DAILY frequency for which the highest intervald is 14. If you have 145 days in your time interval, you would fail the test num <= interval * (self.maxticks[freq] - 1) because you'd have too many ticks. Indeed, you'd expect to get one at 1 Jan, 15 Jan, and 29 Jan. However if interval_multiples is True, we explicitly drop this last tick, so there is only a tick at 1 Jan, 15 Jan, 1 Feb, etc. So there are not actually too many ticks, and reaching this point is fine.

The compromise I reached is to keep the warning in, but exclude checking it for the case of interval_multiples=True and the DAILY frequency.

@jklymak jklymak force-pushed the fix-proper-day-of-month branch from 80e41a6 to 8e51231 Compare June 23, 2020 22:42
@jklymak
Copy link
Member Author
jklymak commented Jun 24, 2020

Added extra test (though there was no test for this warning before)

@jklymak jklymak force-pushed the fix-proper-day-of-month branch from 03b3d28 to 2fe9d1a Compare June 24, 2020 16:06
dstansby
dstansby requested changes Jun 26, 2020
View reviewed changes
Copy link
Member
@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

👍 for the fix, I noticed this funny behavior the other day! Looks like python 3.6 is raising more than 1 warning though.

@jklymak
8000 Copy link
Member Author
jklymak commented Jun 26, 2020

I just assumed that was a flaky test. Its funny that the test passes py3.6 on Azure, but fails on Travis. I really don't understand what might be different. py test versions?

with pytest.warns(UserWarning) as rec:
           locs = locator()
>           assert len(rec) == 1
E           assert 5 == 1
E            +  where 5 = len(WarningsChecker(record=True))

@jklymak
Copy link
Member Author
jklymak commented Jun 26, 2020

This test only fails on python 3.6 on Travis with

       with pytest.warns(UserWarning) as rec:
2523            locs = locator()
            print(rec)
>           assert len(rec) == 1
E           assert 5 == 1
E            +  where 5 = len(WarningsChecker(record=True))
lib/matplotlib/tests/test_dates.py:962: AssertionError
----------------------------- Captured stdout call -----------------------------
WarningsChecker(record=True)

I'm completely befuddled by this, as the same machinery is used elsewhere in this file with no problem.

@QuLogic
Copy link
Member
QuLogic commented Jun 26, 2020

Print all the values in rec; I suspect this might be a warning about setting locale for datetimes?

matplotlib/lib/matplotlib/testing/__init__.py

Lines 34 to 42 in 45d27c9

try:
locale.setlocale(locale.LC_ALL, 'en_US.UTF-8')
except locale.Error:
try:
locale.setlocale(locale.LC_ALL, 'English_United States.1252')
except locale.Error:
_log.warning(
"Could not set locale to English/United States. "
"Some date-related tests may fail.")

@jklymak
Copy link
Member Author
jklymak commented Jun 27, 2020

Trying that now. Not sure how you knew that might be the problem, but...

I'm still not clear why that would only affect this one test set up...

@QuLogic
Copy link
Member
QuLogic commented Jun 27, 2020

It's the only warning I know of that sometimes happens in containers; may or may not be the actual problem though.

@QuLogic
Copy link
Member
QuLogic commented Jun 27, 2020

Not quite as I guessed:

{message : ResourceWarning("unclosed file <_io.BufferedReader name='/usr/share/zoneinfo/UTC'>",), category : 'ResourceWarning', filename : '/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/dateutil/tz.py', lineno : 930, line : None}
{message : ResourceWarning("unclosed file <_io.BufferedReader name='/usr/share/zoneinfo/UTC'>",), category : 'ResourceWarning', filename : '/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/dateutil/tz.py', lineno : 930, line : None}
{message : ResourceWarning("unclosed file <_io.BufferedReader name='/usr/share/zoneinfo/UTC'>",), category : 'ResourceWarning', filename : '/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/dateutil/tz.py', lineno : 930, line : None}
{message : ResourceWarning("unclosed file <_io.BufferedReader name='/usr/share/zoneinfo/UTC'>",), category : 'ResourceWarning', filename : '/home/travis/virtualenv/python3.6.7/lib/python3.6/site-packages/dateutil/tz.py', lineno : 930, line : None}

I suggest using pytest.warns(UserWarning, match="AutoDateLocator was unable"), and I think it ignores unmatched warnings.

@jklymak jklymak force-pushed the fix-proper-day-of-month branch from fe61d62 to 78a4234 Compare June 27, 2020 04:24
@jklymak jklymak requested a review from dstansby June 27, 2020 05:57
QuLogic
QuLogic reviewed Jun 29, 2020
View reviewed changes
@timhoffm timhoffm mentioned this pull request Jul 3, 2020
Closed
@tacaswell tacaswell added this to the v3.4.0 milestone Jul 3, 2020
jklymak and others added 3 commits July 3, 2020 15:05
@tacaswell tacaswell force-pushed the fix-proper-day-of-month branch from f9c895f to c8c9254 Compare July 3, 2020 19:08
@tacaswell
Copy link
Member

Took the liberty of re-basing.

@tacaswell
Copy link
Member

Anyone can merge on green.

@QuLogic QuLogic merged commit b2a11c7 into matplotlib:master Jul 3, 2020
@jklymak jklymak mentioned this pull request Jul 21, 2020
Closed
@jklymak jklymak deleted the fix-proper-day-of-month branch December 10, 2020 00:04
@jklymak jklymak mentioned this pull request Dec 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@tacaswell tacaswell tacaswell approved these changes

@anntzer anntzer anntzer approved these changes

@dstansby dstansby dstansby approved these changes

Assignees
No one assigned
Labels
topic: date handling
Projects
None yet
Milestone
v3.4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@jklymak @tacaswell @QuLogic @anntzer @dstansby
0