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

ENH: add rcParam for ConciseDate and interval_multiples #17022

Merged

Conversation

jklymak
Copy link
Member

@jklymak jklymak commented Apr 3, 2020

PR Summary

This is a proposal for two new date rcParam entries:

date.converter which takes a string. If its 'concise' then mdates.ConciseDateConverter is used for dates, otherwise mdates.AutoDateConverter is used (as before). Default is the same as before: 'auto'

date.interval_multiples sets whether 'interval_multiples' is used (True by default) for the auto locators.

Note this needs a new mdates.register_converters to work after matplotlib.dates has been imported by matplotlib.pyplot. EDIT: fixed to allow automatic changing of the rcParams....

import matplotlib
import datetime
import matplotlib.pyplot as plt
import matplotlib.dates as mdates


y = range(43)
datex = [datetime.date(2020,2,21)+datetime.timedelta(x) for x in range(43)]
fig, axs = plt.subplots(2, 2, constrained_layout=True, figsize=(6,7))

for m, converter in enumerate(['auto', 'concise']):
    for n, interval_multiples in enumerate([True, False]):

        plt.rcParams['date.converter'] = converter
        plt.rcParams['date.interval_multiples'] = interval_multiples
        ax = axs[m, n]
        ax.plot(datex, y)
        if m == 0:
            ax.xaxis.set_tick_params(rotation=45)
plt.show()

Dates

Needs tests and a what's new...

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

units.registry[datetime.date] = DateConverter()
units.registry[datetime.datetime] = DateConverter()

def register_converters():
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic should really be in a _validate_date_converter validator, no? otherwise that means that setting the rcParam from anywhere but the matplotlibrc would have no effect.
(And yes, that means we may either need to try to import matplotlib.dates in rcsetup, or, if that's not possible, special case initial import time and re-validate once we import matplotlib.dates).

Copy link
Member Author

Choose a reason for hiding this comment

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

Does a validator get called every time the rcParam dict is updated? I guess that would be a good way to do that, if a little magical.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there an easy way to tell if the validation is the initial one or a subsequent one? Because if not, I'm not convinced this is worth the bother.

@tacaswell tacaswell added this to the v3.3.0 milestone Apr 3, 2020
@tacaswell
Copy link
Member

I agree this seems reasonable, but share @anntzer 's concern that as written it won't trigger when the rcparams are changed.

@jklymak
Copy link
Member Author

jklymak commented Apr 4, 2020

Unless someone has a straightforward way to update the converter during the init of the rcParams without importing datetime, I'm not convinced the magic of being able to change the rcParam on the fly is worthwhile. People can just call the new mdates.set_converters if they need to change the rcParams during a script.

@anntzer
Copy link
Contributor

anntzer commented Apr 4, 2020

ok, but in that case you need at least to block updating of the rcParam once matplotlib is imported (again, with a custom validator) (and even then I doubt I'd accept the feature). I don't like that solution but I think it's expected (well, at least I expect) that setting a variable in the matplotlibrc is equivalent to setting it in python code, so if it's not at least it should error out cleanly.

@jklymak
Copy link
Member Author

jklymak commented Apr 4, 2020

Well again is there an easy way to know if the validator is being run for first time? If not I don’t see how this can work.

Frankly I think the magic imbued in rcParams is needlessly complicated and we should not think of it as something that must allow real time toggles for everything.

@anntzer
Copy link
Contributor

anntzer commented Apr 4, 2020

It's not so much called once as called from the matplotlibrc, I'm sure the right amount of stack walking could work.
I agree the rcparams magic is a pain, but I feel fairly strongly that rcParams["foo"] = "bar" should behave like foo: bar in your matplotlibrc.
Or, to look at it differently: the current design wouldn't work if the rcparam was set in a style file either.

@jklymak
Copy link
Member Author

jklymak commented Apr 5, 2020

OK, not ridiculously ugly. I just bail if the matplotlib.dates import fails...

@anntzer
Copy link
Contributor

anntzer commented Apr 5, 2020

Much better :)

@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from 1a6da7b to 0b05b77 Compare April 5, 2020 16:23
@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from d22ebe4 to f2a84b1 Compare April 30, 2020 18:15
@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 5, 2020
@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from f2a84b1 to e4c7774 Compare June 19, 2020 04:20
@jklymak jklymak marked this pull request as ready for review June 19, 2020 04:20
@jklymak
Copy link
Member Author

jklymak commented Jun 19, 2020

... I forgot about this one. Its now rebased and should be ready for review...

lib/matplotlib/dates.py Outdated Show resolved Hide resolved
@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from 6cb6516 to 22923c5 Compare June 19, 2020 23:45

class _rcParam_helper:
"""
Never instatiated, but manages the rcParams for dates...
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps merge the comment above and the docstring...

converter = DateConverter

interval_multiples = cls.int_mult
print(interval_multiples)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops


interval_multiples = cls.int_mult
print(interval_multiples)
units.registry[np.datetime64] = converter(
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps just instantiate a single converter and assign to all 3?

Copy link
Member Author

Choose a reason for hiding this comment

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

... I guess so? the old code instantiated three converters, so I'm a little hesitant. OTOH, I'm not sure why it did, and I don't think we lose any flexibility this way...

try:
import matplotlib.dates as mdates
mdates._rcParam_helper.set_converter(s)
except Exception as e:
Copy link
Contributor

Choose a reason for hiding this comment

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

add a comment that this can only fail at initial import time?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - so this gives me a bit of pause. Does this cause any import slowdowns?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you could write mdates = sys.modules.get("matplotlib.dates"); if mdates: ... as matplotlib.dates will be in sys.modules once everything is set up properly...

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I prefer that....

Copy link
Member

Choose a reason for hiding this comment

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

@jklymak do you still want to change how this is written?

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

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

Needs perhaps a few comments/docstring fixes, but other than that looks fine to me.

@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch 2 times, most recently from 6b6f120 to 2cdcfe7 Compare June 20, 2020 15:54
Copy link
Member Author

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Add doc in ConciseDateFormatter

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Modulo a example with a plot in the whats new.

@@ -1884,17 +1888,19 @@ class ConciseDateConverter(DateConverter):
# docstring inherited

def __init__(self, formats=None, zero_formats=None, offset_formats=None,
show_offset=True):
show_offset=True, interval_multiples=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

kwonly?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

@@ -1832,8 +1832,11 @@ class DateConverter(units.ConversionInterface):
The 'unit' tag for such data is None or a tzinfo instance.
"""

@staticmethod
def axisinfo(unit, axis):
def __init__(self, interval_multiples=True):
Copy link
Contributor

Choose a reason for hiding this comment

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

kwonly?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure

plt.rcParams['date.converter'] = 'auto'
plt.rcParams['date.interval_multiples'] = False
ax = axs[1]
ax.plot(dates, y)
Copy link
Member Author

@jklymak jklymak Jun 25, 2020

Choose a reason for hiding this comment

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

This renders as https://39494-1385122-gh.circle-artifacts.com/0/doc/build/html/users/next_whats_new/rcparams_dates.html I'm forgetting how to get the plot to show in the docs? OK figured it out

@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from dfd8924 to 4f29425 Compare June 25, 2020 15:45
@jklymak
Copy link
Member Author

jklymak commented Jun 25, 2020

Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

Still approve, leave it to @jklymak if we should change how the conditional import is done.

Anyone can merge.

@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from 4f29425 to abc06c5 Compare June 26, 2020 04:34
@jklymak
Copy link
Member Author

jklymak commented Jun 26, 2020

Still approve, leave it to @jklymak if we should change how the conditional import is done.

Anyone can merge.

Ooops, duh - I missed one of the two cases. Thanks for pointing that out.

@jklymak jklymak force-pushed the enh-rcparams-concisedate-intervals branch from 610f5fb to a793bde Compare July 7, 2020 05:16
@tacaswell tacaswell merged commit 7ed6bc2 into matplotlib:master Jul 7, 2020
@jklymak jklymak deleted the enh-rcparams-concisedate-intervals branch July 7, 2020 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants