Skip to content
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.

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

FIX: new date rcParams weren't being evaluated #17869

Merged
merged 1 commit into from Jul 15, 2020

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Jul 9, 2020

PR Summary

Closes #17867

#17022 introduced new rcparams for dates. However, this changed the registration mechanism for dates and broke date plotting if the entry was not in the users matplotlibrc.

This new version fixes that logic (and adds a bit more argument checking)

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/next_api_changes/* if API changed in a backward-incompatible way

lib/matplotlib/rcsetup.py Outdated Show resolved Hide resolved
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.

Fixes the original issue.

lib/matplotlib/dates.py Outdated Show resolved Hide resolved
@dstansby dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 9, 2020
@efiring
Copy link
Member

efiring commented Jul 13, 2020

This looks OK and I wouldn't block it, but I wonder whether it would be cleaner to move dates._rcParam_helper into rcsetup. It would require importing units in rcsetup, but it would put _rcParam_helper in the only module that actually uses it and it would eliminate importing the dates module inside methods. (If it were moved, it would be renamed to something like _date_converter_helper.) This reorganization could be done in a separate PR for 3.4; it looks like the present PR needs to go into 3.3 ASAP.

@jklymak
Copy link
Member Author

jklymak commented Jul 13, 2020

My concern wth that is it becomes even more mysterious how the units registry works for dates. I also am not sure that the import can happen at the beginning of rcsetup.py. But I get confused by the import order to be honest.

@@ -1926,6 +1926,8 @@ class _rcParam_helper:
@classmethod
def set_converter(cls, s):
"""Called by validator for rcParams date.converter"""
if s not in ['concise', 'auto']:
Copy link
Member

Choose a reason for hiding this comment

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

Use cbook._check_in_list()?

Copy link
Member Author

Choose a reason for hiding this comment

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

s is just a string, not a dict.

Copy link
Member

Choose a reason for hiding this comment

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

_check_in_list doesn't check dicts?

Copy link
Member

Choose a reason for hiding this comment

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

It would work:

cbook._check_in_list(['concise', 'auto'], s=s)

The example in the docstring is misleading; as far as I can see, this would rarely if ever be called with more than one keyword argument. The reason it requires a keyword argument is that this supplies a name for the variable when reporting the error.

Copy link
Member

@efiring efiring left a comment

Choose a reason for hiding this comment

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

It looks good enough for now. My refactoring suggestion can be deferred or ignored.

@efiring efiring merged commit ca01a39 into matplotlib:master Jul 15, 2020
@jklymak
Copy link
Member Author

jklymak commented Jul 15, 2020

Yeah, its not necessarily a bad suggestion - I just didn't fully grok it.

@jklymak jklymak deleted the fix-date-rcparam branch July 15, 2020 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. topic: date handling topic: rcparams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datetime plotting broken on master
5 participants