IGNORE: Lockout new axis converters by jklymak · Pull Request #9736 · matplotlib/matplotlib

jklymak · 2017-11-09T21:09:54Z

Keeping here in case we want to use instead of #9776, but #9776 is my preferred solution.

PR Summary

A script like:

    base = datetime.datetime(2017, 1, 1, 0, 10, 0)
    time = [base - datetime.timedelta(days=x) for x in range(0, 3)]
    data = [0., 2., 4.]
    fig, ax = plt.subplots()
    ax.plot(time, data)
    ax.plot(['a', 'b'], [1., 2.])

would run and make a very non-sensical and mislabeled plot. This now returns a TypeError if this is done.

Note that the following will not be caught, and will result in an ugly error that pops up all the time:

    dt = datetime.datetime.fromordinal(ix).replace(tzinfo=UTC)
ValueError: ordinal must be >= 1

    base = datetime.datetime(2017, 1, 1, 0, 10, 0)
    time = [base - datetime.timedelta(days=x) for x in range(0, 3)]
    data = [0., 2., 4.]
    fig, ax = plt.subplots()
    ax.plot(time, data)
    ax.plot([0., 3.], [1., 2.])

The problem here is that the update_units function also gets called by set_xlim and set_ylim, and if these are on auto it passes bare floats. Maybe that can be fixed, but is a bigger project...

See #9713

PR Checklist

Has Pytest style unit tests
Code is PEP 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

story645 · 2017-11-09T21:18:46Z


+        if (self.converter is not None) and (self.converter != converter):
+            raise TypeError('Attempting to plot data that is '
+                            'registered to be converted with %s but '


Should the language be something like "data with units %s which is incompatible with existing axis units %s" - kinda wondering if registered convertors is too weedy for users.

Open to this - I find the word "units" very confusing in this context. Looking at the jpl stuff I can see there was an attempt to let the data have different units and then get converted by what the axis units were (i.e. if xunits='centimetres' but the x-axis units were 'meters' 1.0 would be plotted as 0.01) but that seems to have been lost along the way. Or I'm misunderstanding (more likely)

I think that's sort of what the basic units example might be about too-same numbers, but we'll do the conversion internally. Maybe just stick to type? "attempting to plot data of type %s that is incompatiable with existing axis data of type %s"

Oooh, except it returns fun things like matplotlib.dates.DateConverter not the data type. I could state what the new type is (i.e. type blah cannot be processed by axis unit converter matplotlib.dates.DateConverter)

I like that last suggestions @jklymak

FWIW a converter that converts meters should also be able convert centimeters fine (they're both lengths!)

jklymak · 2017-11-09T23:19:21Z

OK, so I have a new version that actually locks out everything, but still passes most tests. I've not uploaded yet...

It fails tests where people try to override the units of the axis:

def test_fill_units():
        def test_single_date():
        time1 = [721964.0]
        data1 = [-65.54]

        fig = plt.figure()
        plt.subplot(211)
        plt.plot_date(time1, data1, 'o', color='r')

Fails because they try to pass a float time after they have set the axis to the "datetime" converter.

My tendency would be to just rewrite these (very few) tests so they behave and update this PR, but thought I'd seek input... I can't tell what level of folks require this mixing of modes. Ironically the jpl folks have such an example:

def test_fill_units():
        from datetime import datetime
        import matplotlib.testing.jpl_units as units
        units.register()

        # generate some data
        t = units.Epoch("ET", dt=datetime(2009, 4, 27))
        value = 10.0 * units.deg
        day = units.Duration("ET", 24.0 * 60.0 * 60.0)

        fig = plt.figure()

        # Top-Left
        ax1 = fig.add_subplot(221)
        ax1.plot([t], [value], yunits='deg', color='red')
        ax1.fill([733525.0, 733525.0, 733526.0, 733526.0],
                 [0.0, 0.0, 90.0, 0.0], 'b')

dstansby

I am very in favor of this, since a single axis should only have a single unit being plotted on it!

dstansby · 2017-12-27T21:59:32Z

+                            'converter %s' % (converter.__class__,
+                                self.converter.__class__))
+
        neednew = self.converter != converter


This line can change to neednew = self.converter == None, and maybe change the variable name neednew to needupdate too since that's really what it means now.

dstansby · 2017-12-27T22:02:14Z


+        if (self.converter is not None) and (self.converter != converter):
+            raise TypeError('Attempting to plot data that is '
+                            'registered to be converted with %s but '


I like that last suggestions @jklymak

FWIW a converter that converts meters should also be able convert centimeters fine (they're both lengths!)

dstansby · 2017-12-27T22:03:34Z

Oh, should definitely get an API change note too.

jklymak · 2017-12-28T17:09:48Z

@dstansby Did you have a chance to look at #9776 which is probably what I think we should consider using? I think this approach of locking an axis to a converter should be thought about seriously - and maybe by someone more experienced w/ the units code than me. This all came up wrt datetime code and categorical code. So, @tacaswell, @anntzer, @story645 should chime in on if this is a good protection to add or not.

anntzer · 2017-12-28T20:17:07Z

Haven't thought about this recentely but IIRC making the implicit (pass-thru) converter explicit (i.e. #9776) seemed a better approach.

jklymak added 2 commits November 9, 2017 12:53

If an axis has a non-None converter, don't allow another non-None

08a800c

TST: test that two different converters same axis give TypeError

393a05b

jklymak changed the title ~~Lockoutnewconverters~~ Lockout new axis converters Nov 9, 2017

story645 reviewed Nov 9, 2017

View reviewed changes

dstansby added the topic: units and array ducktypes label Nov 9, 2017

dstansby added this to the v2.2 milestone Nov 9, 2017

This was referenced Nov 13, 2017

Rethink categoricals. #9774

Closed

WIP: Lockout new converters Part 2 #9776

Closed

dstansby requested changes Dec 27, 2017

View reviewed changes

jklymak changed the title ~~Lockout new axis converters~~ IGNORE: Lockout new axis converters Jan 3, 2018

jklymak closed this Jan 3, 2018

QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018

Navigation Menu

Repository navigation

IGNORE: Lockout new axis converters - #9736

IGNORE: Lockout new axis converters#9736
jklymak wants to merge 2 commits into
matplotlib:masterfrom
jklymak:lockoutnewconverters

jklymak commented Nov 9, 2017 •

edited

Loading

Uh oh!

story645 Nov 9, 2017

Uh oh!

jklymak Nov 9, 2017

Uh oh!

story645 Nov 9, 2017

Uh oh!

jklymak Nov 9, 2017

Uh oh!

dstansby Dec 27, 2017

Uh oh!

jklymak commented Nov 9, 2017 •

edited

Loading

Uh oh!

dstansby left a comment

Uh oh!

dstansby Dec 27, 2017

Uh oh!

dstansby Dec 27, 2017

Uh oh!

dstansby commented Dec 27, 2017

Uh oh!

jklymak commented Dec 28, 2017

Uh oh!

anntzer commented Dec 28, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jklymak commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

story645 Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

jklymak Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

story645 Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

jklymak Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

dstansby Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

jklymak commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dstansby left a comment

Choose a reason for hiding this comment

Uh oh!

dstansby Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

dstansby Dec 27, 2017

Choose a reason for hiding this comment

Uh oh!

dstansby commented Dec 27, 2017

Uh oh!

jklymak commented Dec 28, 2017

Uh oh!

anntzer commented Dec 28, 2017

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jklymak commented Nov 9, 2017 •

edited

Loading

jklymak commented Nov 9, 2017 •

edited

Loading