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

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

  1. matplotlib
  2. matplotlib

Repository navigation

IGNORE: Lockout new axis converters - #9736

Closed
jklymak wants to merge 2 commits into
matplotlib:masterfrom
jklymak:lockoutnewconverters
ConversationCommits (2)ChecksFiles changed
Closed

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

Conversation

jklymak commented Nov 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

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

jklymak changed the title Lockoutnewconverters Lockout new axis converters Nov 9, 2017
story645 reviewed Nov 9, 2017
View reviewed changes
Comment thread lib/matplotlib/axis.py

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 '

story645 Nov 9, 2017

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

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.

jklymak Nov 9, 2017

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

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)

story645 Nov 9, 2017

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

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"

jklymak Nov 9, 2017

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

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)

dstansby Dec 27, 2017

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

I like that last suggestions @jklymak

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

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

jklymak commented Nov 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

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')

This was referenced Nov 13, 2017
Closed
Closed
dstansby requested changes Dec 27, 2017
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

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

Comment thread lib/matplotlib/axis.py
'converter %s' % (converter.__class__,
self.converter.__class__))

neednew = self.converter != converter

dstansby Dec 27, 2017

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

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.

Comment thread lib/matplotlib/axis.py

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 '

dstansby Dec 27, 2017

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

I like that last suggestions @jklymak

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

dstansby commented Dec 27, 2017

Copy link
Copy Markdown
Member

Oh, should definitely get an API change note too.

jklymak commented Dec 28, 2017

Copy link
Copy Markdown
Member Author

@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 commented Dec 28, 2017

Copy link
Copy Markdown
Contributor

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

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
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

story645 story645 left review comments
dstansby dstansby requested changes

Assignees

No one assigned

Labels

topic: units and array ducktypes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL