FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

FIX: new date rcParams weren't being evaluated by jklymak · Pull Request #17869 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

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

Merged
efiring merged 1 commit into
matplotlib:masterfrom
jklymak:fix-date-rcparam
Jul 15, 2020
ConversationCommits (1)ChecksFiles changed
Merged

FIX: new date rcParams weren't being evaluated#17869
efiring merged 1 commit into
matplotlib:masterfrom
jklymak:fix-date-rcparam

Conversation

jklymak commented Jul 9, 2020

Copy link
Copy Markdown
Member

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

jklymak added this to the v3.4.0 milestone Jul 9, 2020
dstansby reviewed Jul 9, 2020
View reviewed changes
Comment thread lib/matplotlib/rcsetup.py Outdated
dstansby approved these changes Jul 9, 2020
View reviewed changes

dstansby left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixes the original issue.

Comment thread lib/matplotlib/dates.py Outdated
dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jul 9, 2020
jklymak force-pushed the fix-date-rcparam branch from a7abf30 to 20a599b Compare July 11, 2020 15:11
jklymak force-pushed the fix-date-rcparam branch from 20a599b to 3c84dc3 Compare July 11, 2020 15:54

efiring commented Jul 13, 2020

Copy link
Copy Markdown
Member

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 commented Jul 13, 2020

Copy link
Copy Markdown
Member Author

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.

timhoffm reviewed Jul 13, 2020
View reviewed changes
Comment thread lib/matplotlib/dates.py
@classmethod
def set_converter(cls, s):
"""Called by validator for rcParams date.converter"""
if s not in ['concise', 'auto']:

timhoffm Jul 13, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Use cbook._check_in_list()?

jklymak Jul 13, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

s is just a string, not a dict.

QuLogic Jul 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

_check_in_list doesn't check dicts?

efiring Jul 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

This was referenced Jul 14, 2020
Closed
Closed
efiring approved these changes Jul 15, 2020
View reviewed changes

efiring left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

efiring merged commit ca01a39 into matplotlib:master Jul 15, 2020

jklymak commented Jul 15, 2020

Copy link
Copy Markdown
Member Author

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

jklymak deleted the fix-date-rcparam branch July 15, 2020 00:07
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

efiring efiring approved these changes
QuLogic QuLogic left review comments
timhoffm timhoffm left review comments
dstansby dstansby approved these changes

Assignees

No one assigned

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

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

datetime plotting broken on master

5 participants

Back | FazBrowse Home | New Git URL