Fix unit support with `plot` and `pint` #4803

dopplershift · 2015-07-28T03:31:36Z

pint is a unit support library that wraps (almost) anything with mathematical-aware unit operations. Since it tries to quack like a numpy array, but is not actually an ndarray subclass, this creates problems when thrown into functions like atleast_1d. atleast_1d converts all arguments, regardless of whether they are properly dimensioned, into numpy arrays (or subclasses).

Currently, pint can't even trigger matplotlib's unit infrastructure, as evidenced by:

import pint
import numpy as np
import matplotlib.units as munits

units = pint.UnitRegistry()

# Implementation of matplotlib support for unit conversion using pint
class PintConverter(object):
    @staticmethod
    def convert(value, unit, axis):
        print('converting', value, 'to', unit, 'axis units: ', axis.get_units())
        if hasattr(value, 'units'):
            return value.to(unit)
        else:
            return units.Quantity(value, axis.get_units()).to(unit).magnitude

    @staticmethod
    def axisinfo(unit, axis):
        print('axisinfo')
        return None

    @staticmethod
    def default_units(x, axis):
        print('defaultunits')
        return x.units

# Register the class
munits.registry[units.Quantity] = PintConverter()

import matplotlib.pyplot as plt
t = np.linspace(0, 10) * units.sec
v = 30 * units('m/s')
d = t * v

fig,ax = plt.subplots(1,1)
l, = plt.plot(t, d)
ax.yaxis.set_units(units.inch)
ax.autoscale_view()
plt.show()

This example triggers no print statements on master. With these changes, unit support works (I think) properly with pint.

mdboom · 2015-07-28T12:19:38Z

👍

Just an aside, I've been working on astropy.units support for matplotlib over here: astropy/astropy#3981.

tacaswell · 2015-07-28T12:43:43Z

I am poking the same section of code in #4787 to pull out the index if it exists.

shoyer · 2015-07-29T04:46:39Z

This is nice idea, but it would almost certainly break existing code using plt.plot. The problem is that it's not safe to assume that all duck-array types (with a shape) can be indexed with np.newaxis.

To my surprise, this actually does work for pandas Index and Series objects:

>>> pd.Series(range(5))[:, None]
array([[0],
       [1],
       [2],
       [3],
       [4]])

But there are other duck array types that can't handle this operation, even though they implement __array__ to support conversion to numpy arrays. For example:

>>> xray.DataArray(range(5))[:, None]
IndexError: too many indices

h5py datasets are another example, off the top of my head.

To be fair, this tradeoff might be worthwhile. In many cases, plotting duck arrays already fails, and this may not be a feature that matplotlib ever intended to support. But it's certainly worth considering the broader question of what classes of duck array types matplotlib intends to support as input to plotting functions. In many cases, calling np.asarray is the appropriate response to unexpected input.

After all that discussion, it occurs to me that a better approach here would be simply to explicitly look for types that have been registered with matplotlib's unit system rather than to whitelist all objects with a shape attribute.

dopplershift · 2015-07-29T16:33:36Z

Well _check_1d is the simplest possible implementation that accomplished the use cases I wanted. It's simple enough to enhance the duck typing to check the operations we actually need. I prefer this approach because, technically, this PR is orthogonal to the units support--it's fixing the conversion to array by atleast_1d when it's not needed.

dopplershift · 2015-07-29T17:17:33Z

I've updated this to have more thorough checks. The bonus here is that it seems to eliminate any changes to the examples, though I won't completely trust that until Travis finishes...

shoyer · 2015-07-29T18:00:40Z

lib/matplotlib/cbook.py

+    '''
+    Converts a sequence of less than 1 dimension, to an array of 1
+    dimension; leaves everything else untouched.
+    '''


What about instead using something like the following?

def asarray_units_okay(x): if type(x) in units.registry: return x else: return np.asarray(x) def atleast_1d_units_okay(x); x = asarray_units_okay(x) if x.ndim == 0: x = x[np.newaxis] return x

I am nervous about preemptively checking all allowed operations.

This breaks anything using units that relies upon having atleast_1d called; this includes the "simple" unit framework in the examples.

Otherwise, the decomposition seems fine.

dopplershift · 2015-08-14T18:23:05Z

@tacaswell Any chance we can sneak this in for 1.5?

tacaswell · 2015-08-14T18:43:37Z

Yes, but this is going to conflict with the logic added in https://github.com/matplotlib/matplotlib/pull/4829/files#diff-a7f4f21aa80e975d813bc436acb68d8cL280
only question is how manage that.

I am sure how the discussion with @shoyer ended, it looks like the comment should have been folded as addressed, but is pinned to the documentation?

dopplershift · 2015-08-14T18:56:00Z

I'm willing to rebase/rework this once #4829 lands.

As far as the discussion goes, @shoyer and I disagree about the approach. I don't think we should rely upon the units registry, and that pre-emptively checking operations should be fine. Your thoughts here would be most welcome.

tacaswell · 2015-08-14T19:08:53Z

I like the preemptive checking better as I think it plays better with some of the label/index related checking else where. Relying on the unit registry also adds a bit more order dependence then is strictly necessary as in most cases the conversion to float for plotting is deferred to draw time where as the proposed alternative method would possibly strip some of the data earlier.

@jrevans might have useful thoughts on this as well.

tacaswell · 2015-09-13T00:30:05Z

@dopplershift This needs a rebase now

jkseppan · 2015-09-13T12:54:45Z

There's a rebase on my branch fix-plot-units, but since this PR adds no tests, it's hard to say if the rebase still fixes the original issue. I would recommend adding a test case.

See jkseppan/matplotlib@ab04ebabcd

dopplershift · 2015-09-14T15:34:52Z

Rebase done. I agree about the tests, but my problem is that I couldn't readily come up with a good test that didn't involve depending/ingesting pint.

dopplershift · 2015-09-14T15:48:57Z

I can confirm that the "test" I posted at the top works with the rebased branch. I'll take a quick look at a stubbed out "pint" that works for testing.

dopplershift · 2015-09-14T16:45:22Z

Alright, simple test added (and now I know mock!). I verified that this test fails before my changes, and succeeds after.

tacaswell · 2015-09-14T16:50:33Z

remember to white-list the new test file __init__.py

dopplershift · 2015-09-14T16:58:10Z

@tacaswell huh?

tacaswell · 2015-09-14T17:02:26Z

This is the list of tests that are run by default https://github.com/matplotlib/matplotlib/blob/master/lib/matplotlib/__init__.py#L1416 (and this is what travis runs)

dopplershift · 2015-09-14T18:24:50Z

I don't understand the failures (or at least how they can only fail sometimes):

AttributeError: 'module' object has no attribute 'test_units'

tacaswell · 2015-09-14T18:32:04Z

That helpful exception means that there is an import error in test_units (knowing that cost me too many hours of my life).

dopplershift · 2015-09-14T18:37:50Z

Thanks, turns out I didn't understand about importing mock.

This is catching the problems with pint-like libraries who wrap numpy arrays.

Even if the argument to atleast_1d is array-compatible and has sufficient dimensions, it is still converted from its original type to a numpy array. This is overly aggressive for our purposes (especially unit support), so replace the direct call with a helper function that only calls atleast_1d if it's actually necessary.

FIX: unit support with `plot` and `pint`

tacaswell added the status: needs review label Jul 28, 2015

dopplershift added the topic: units and array ducktypes label Jul 28, 2015

tacaswell added this to the proposed next point release milestone Jul 29, 2015

dopplershift force-pushed the fix-plot-units branch from 4cb45dc to 20cb005 Compare July 29, 2015 17:16

shoyer reviewed Jul 29, 2015
View reviewed changes

tacaswell modified the milestones: next point release, proposed next point release Aug 14, 2015

tacaswell added the status: needs revision label Aug 14, 2015

tacaswell removed the status: needs review label Aug 22, 2015

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Sep 2, 2015

tacaswell mentioned this pull request Sep 2, 2015

outstanding issues from jpl #4897

Closed

17 tasks

dopplershift force-pushed the fix-plot-units branch from 20cb005 to 7e1f13e Compare September 14, 2015 15:33

tacaswell added status: needs review and removed status: needs revision labels Sep 14, 2015

dopplershift force-pushed the fix-plot-units branch from 7e1f13e to ab03007 Compare September 14, 2015 16:44

dopplershift force-pushed the fix-plot-units branch from fd4f962 to f2d1e8a Compare September 14, 2015 17:12

dopplershift force-pushed the fix-plot-units branch from f2d1e8a to 03e4cc1 Compare September 14, 2015 18:37

dopplershift added 3 commits September 14, 2015 12:55

Add test for unit conversion.

0925e38

This is catching the problems with pint-like libraries who wrap numpy arrays.

Add test_units to whitelist.

9b65233

dopplershift force-pushed the fix-plot-units branch from 03e4cc1 to 9b65233 Compare September 14, 2015 18:56

tacaswell added a commit that referenced this pull request Sep 14, 2015

Merge pull request #4803 from dopplershift/fix-plot-units

c27970c

FIX: unit support with `plot` and `pint`

tacaswell merged commit c27970c into matplotlib:master Sep 14, 2015

tacaswell removed the status: needs review label Sep 14, 2015

dopplershift deleted the fix-plot-units branch September 14, 2015 19:30

dopplershift mentioned this pull request Aug 8, 2017

axhline/axvline broken with pint.Quantity #8910

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unit support with `plot` and `pint` #4803

Fix unit support with `plot` and `pint` #4803

dopplershift commented Jul 28, 2015

mdboom commented Jul 28, 2015

tacaswell commented Jul 28, 2015

shoyer commented Jul 29, 2015

dopplershift commented Jul 29, 2015

dopplershift commented Jul 29, 2015

shoyer Jul 29, 2015

dopplershift Jul 29, 2015

dopplershift commented Aug 14, 2015

tacaswell commented Aug 14, 2015

dopplershift commented Aug 14, 2015

tacaswell commented Aug 14, 2015

tacaswell commented Sep 13, 2015

jkseppan commented Sep 13, 2015

dopplershift commented Sep 14, 2015

dopplershift commented Sep 14, 2015

dopplershift commented Sep 14, 2015

tacaswell commented Sep 14, 2015

dopplershift commented Sep 14, 2015

tacaswell commented Sep 14, 2015

dopplershift commented Sep 14, 2015

tacaswell commented Sep 14, 2015

dopplershift commented Sep 14, 2015

Fix unit support with plot and pint #4803

Fix unit support with plot and pint #4803

Conversation

dopplershift commented Jul 28, 2015

mdboom commented Jul 28, 2015

tacaswell commented Jul 28, 2015

shoyer commented Jul 29, 2015

dopplershift commented Jul 29, 2015

dopplershift commented Jul 29, 2015

shoyer Jul 29, 2015

Choose a reason for hiding this comment

dopplershift Jul 29, 2015

Choose a reason for hiding this comment

dopplershift commented Aug 14, 2015

tacaswell commented Aug 14, 2015

dopplershift commented Aug 14, 2015

tacaswell commented Aug 14, 2015

tacaswell commented Sep 13, 2015

jkseppan commented Sep 13, 2015

dopplershift commented Sep 14, 2015

dopplershift commented Sep 14, 2015

dopplershift commented Sep 14, 2015

tacaswell commented Sep 14, 2015

dopplershift commented Sep 14, 2015

tacaswell commented Sep 14, 2015

dopplershift commented Sep 14, 2015

tacaswell commented Sep 14, 2015

dopplershift commented Sep 14, 2015

Fix unit support with `plot` and `pint` #4803

Fix unit support with `plot` and `pint` #4803