New "accepts units" decorator #10411

dstansby · 2018-02-10T19:05:39Z

This PR implements a decorator that marks a function as accepting units, and also does the unit conversion. The decorator takes a list of parameters that can accept unitised data, so for example both x and xlim could be converted as part of the same function.

The decorator itself is currently not very neat code, but it works as a proof of concept.

This would also make the creation of "methods that accept units" registry really easy, as anything that is decorated could be automatically added to it, and a page in the docs with methods that support units could be automatically generated.

dstansby · 2018-02-10T19:06:43Z

.travis.yml

@@ -65,19 +65,6 @@ env:

 matrix:
  include:
-    - python: 2.7


I've got rid of the py27 build to avoid having to do funny things with importing signature.

anntzer · 2018-02-10T20:38:26Z

+1 for the general idea, but may be easier to move the decorator under preprocess_data so that you don't have to fiddle with the data kwarg yourself?

dstansby · 2018-02-10T20:41:49Z

I'm not sure that would work, because eventually it would be good to apply this decorator to e.g. ax.set_xlim() which doesn't have the preprocess_data decorator.

tacaswell · 2018-02-11T04:07:33Z

I am not sure if it is better to pull the conversion as early as possible (as proposed here) or to push it as late as possible (into the draw methods)

efiring · 2018-02-11T06:33:56Z

It seems to me that early conversion is necessary, so that you can proceed to work with numbers.

anntzer · 2018-02-11T10:25:59Z

re: ax.set_xlim: that one can't take a data kwarg anyways, so I don't see how things are related?

dstansby · 2018-02-14T22:18:46Z

@anntzer Ah, I see what you mean now. I tried it the other way round, but the data decorator seems to do funny things with the function signature and I couldn't get it to work.

anntzer · 2018-02-14T23:19:02Z

May be worth (but not necessarily easy) to make the data decorator set the correct signature so that the units decorator can be made to work? (just throwing ideas around)

dstansby · 2018-04-17T13:26:50Z

I think this is ready - if everyone is still keen I will try and roll it out to a few other functions that do the unit conversion in house near the beginning of their definition?

dopplershift · 2018-04-17T18:11:57Z

It seems like an improvement to me.

anntzer · 2018-04-18T10:17:49Z

lib/matplotlib/units.py

+            has_data = (('data' in arguments) and
+                        (arguments['data'] is not None))
+            if has_data:
+                data = arguments['data']


Perhaps move the decorator under _preprocess_data, so that you don't have to reproduce its logic here?

I tried that a while ago, and from what I remember it became quite complicated because _preprocess_data alters functions signatures in a funny way.

I'm not quite sure, that it's a good idea for _preprocess_data to alter the function signature anyway. I would be happy explicitly writing a data=None in the signatures and only let _preprocess_data work on that. Feels less magical and simplifies the _preprocess_data code.

I don't think that would work, since the idea is that if I do ax.plot(dataframe['Distance']) it will automatically get the index of the dataframe as the x-values. Anyway, if anyone wants to alter the _preprocess_data decorator feel free to in another PR, but this one is for units 😉

Don't think that's done in _preprocess_data. If I understand the code correctly, this is from cbook.index_of() via Axes._process_plot_var_args(), which is independent of _preprocess_data. 😇

Anyway, I'm +/-0 on using the copied code from _preprocess_data in the new decorator vs. including the unit stuff there. Just saying that altering the function signature can (and maybe) should be removed from _preprocess_data, if that was the only reason preventing inclusion.

One final remark. If the decision is, that this functionality should be kept in separate decorators, which would be a vailid point, maybe it's worth to factor out common functionality.

Separate topic: If I'm correct, the four previous lines can be simplified to
data = arguments.get('data')

and use data is not None instead of has_data in the following.

dstansby · 2018-04-18T15:24:19Z

I think that's all the easy methods taken care of, feel free to review!

timhoffm · 2018-04-18T15:45:40Z

lib/matplotlib/units.py

+    convert_x, convert_y : list
+        A list of integers or strings, indicating the arguments to be converted
+    """
+    def decorator(func):


The current implementation hides the docstring of the decorated functions. Not an expert on decorators, but it might be sufficient to decorate decorator(func) with @wraps.

Eeek good catch, will see what I can do.

Doing the @wraps seems to work

dstansby · 2018-04-30T17:16:08Z

Reviews needed bump!

dopplershift

Just found one docstring that needs updating. Otherwise, this is 👍 from me.

dopplershift · 2018-05-01T01:47:35Z

lib/matplotlib/axes/_base.py

@@ -3004,14 +3005,13 @@ def _validate_converted_limits(self, limit, convert):
        The limit value after call to convert(), or None if limit is None.


I don't think this function returns anything now, no?

dopplershift · 2018-05-07T21:07:04Z

lib/matplotlib/axes/_base.py

@@ -3356,6 +3350,7 @@ def get_ylim(self):
        """
        return tuple(self.viewLim.intervaly)

+    @munits._accepts_units(convert_y=['bottom', 'top'])


I just wanted to note that this fixes a bug for us where we ran into problems setting limits using units. (Note the lack of a call to _process_unit_info() in the original set_ylim() code.)

dopplershift · 2018-05-07T21:08:04Z

Need another sign off to merge

jklymak · 2018-07-04T19:35:37Z

ping @anntzer, @tacaswell - this seems good to me, but is too technical for me to know all the implications.

timhoffm · 2018-07-04T20:16:39Z

lib/matplotlib/units.py

+    def decorator(func):
+        @functools.wraps(func)
+        def wrapper(*args, **kwargs):
+            axes = args[0]


It should be noted in the docstring that the first parameter must be an Axes.

timhoffm · 2018-07-04T20:34:16Z

lib/matplotlib/units.py

+            has_data = (('data' in arguments) and
+                        (arguments['data'] is not None))
+            if has_data:
+                data = arguments['data']


Separate topic: If I'm correct, the four previous lines can be simplified to
data = arguments.get('data')

and use data is not None instead of has_data in the following.

timhoffm · 2018-07-04T21:00:59Z

lib/matplotlib/units.py

+            # unit info, convert argument, and replace in *arguments* with
+            # converted values
+            for arg in convert_x:
+                if has_data and arguments[arg] in data:


This does not work for structured numpy arrays:

>>> a = np.array([(1, 2), (3, 4)], dtype=[('x', float), ('y', float)]) >>> 'x' in a False >>> print(a['x']) [1, 3]

See also #10872 (comment)

Do you know what works instead? Do we officially support structured numpy arrays?

See the _has_item() function in #10872 for a working check.

https://matplotlib.org/api/_as_gen/matplotlib.axes.Axes.plot.html says:

All indexable objects are supported. This could e.g. be a dict, a pandas. DataFame or a structured numpy array.

Not sure if I have written it in there with the last changes. But at least nobody has vetoed. Anyway, I think it's desirable to support structured numpy arrays.

tacaswell · 2018-07-08T22:35:20Z

lib/matplotlib/units.py


 import numpy as np

 from matplotlib.cbook import iterable, safe_first_element


+def _accepts_units(convert_x=[], convert_y=[]):


no mutable defaults please!

tacaswell · 2018-07-08T22:39:47Z

I am a bit concerned about this being half applied to the code base but also concerned about trying to touch everything all at once.

Can you also add a section to the documentation a "how to handle uints" section do the docs?

dstansby · 2018-07-09T02:30:37Z

I'm happy to let this slip to 3.1, and expand it properly and give everyone a bit more time to think about it, especially since I don't have much time to work on it at the moment.

Proof of concept "accepts units" decorator Add helper function Mssing commas Fix docstrings disappearing Ignore None when converting data Remove returns section in doc

Add some more unit decorators Add unit decorator to mplot3d

dstansby · 2018-09-16T16:58:37Z

After some more thinking, it strikes me that this might not be a possible way to approach units, because in theory a user could change the plotting units (e.g. from seconds to hours) at any point before draw time, so we really do need to keep copies of the input "quantities" in order to do this conversion at some point down the line.

jklymak · 2018-09-16T17:22:13Z

Agreed. I sent an email around with a units MEP some of which was misguided, but this is what I think would make the whole thing work. If a data variable can be converted it gets packed in a dict that has the original data object so we can always rerun the conversion on the original object. That way we can convert to np float right away without worrying that we can’t undo it.

jklymak · 2019-02-11T21:49:57Z

@dstansby is this still active? Closing, but feel free to re-open!

dstansby · 2019-02-12T09:10:02Z

🤷‍♂️ don't really know any more. This is at lest a consistent approach, which is arguably better than the current mess, but whether it's the correct one or not I am unsure of.

dstansby added the topic: units and array ducktypes label Feb 10, 2018

dstansby added this to the v3.0 milestone Feb 10, 2018

dstansby commented Feb 10, 2018

View reviewed changes

dstansby force-pushed the unit-decorator branch from 72cd67a to 7013b6c Compare February 10, 2018 19:09

dstansby force-pushed the unit-decorator branch from 665b399 to 1494113 Compare February 11, 2018 10:56

dstansby force-pushed the unit-decorator branch from 1494113 to cffa364 Compare February 14, 2018 22:20

dstansby mentioned this pull request Mar 1, 2018

Fix for failing bar plot w/ units #10623

Merged

dstansby changed the title ~~[WIP] New "accepts units" decorator~~ New "accepts units" decorator Mar 1, 2018

dstansby force-pushed the unit-decorator branch 2 times, most recently from 3fdb8d7 to f44ee44 Compare April 13, 2018 15:55

anntzer reviewed Apr 18, 2018

View reviewed changes

dstansby force-pushed the unit-decorator branch 2 times, most recently from 8a7f1a9 to 07bf0d6 Compare April 18, 2018 12:47

timhoffm reviewed Apr 18, 2018

View reviewed changes

jklymak added the status: needs revision label Apr 18, 2018

dstansby force-pushed the unit-decorator branch from 0462d2e to bc61047 Compare April 18, 2018 16:29

dstansby added status: needs review and removed status: needs revision labels Apr 18, 2018

tacaswell requested a review from dopplershift May 1, 2018 01:40

dopplershift requested changes May 1, 2018

View reviewed changes

dstansby force-pushed the unit-decorator branch from bc61047 to a786018 Compare May 6, 2018 20:02

dopplershift reviewed May 7, 2018

View reviewed changes

dopplershift approved these changes May 7, 2018

View reviewed changes

dstansby force-pushed the unit-decorator branch from a786018 to 8c97270 Compare May 8, 2018 07:35

dstansby added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jun 24, 2018

jklymak added the status: needs rebase label Jul 4, 2018

timhoffm reviewed Jul 4, 2018

View reviewed changes

dstansby force-pushed the unit-decorator branch from 8c97270 to b37ee96 Compare July 5, 2018 13:04

dstansby removed the status: needs rebase label Jul 5, 2018

tacaswell reviewed Jul 8, 2018

View reviewed changes

tacaswell modified the milestones: v3.0, v3.1 Jul 9, 2018

jklymak removed status: needs review Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labels Aug 17, 2018

dstansby added 3 commits September 16, 2018 10:43

Proof of concept "accepts units" decorator

8e0a9cf

Proof of concept "accepts units" decorator Add helper function Mssing commas Fix docstrings disappearing Ignore None when converting data Remove returns section in doc

Apply unit decorator to more functions

81eff1f

Add some more unit decorators Add unit decorator to mplot3d

Small fixes

f42e3c4

dstansby force-pushed the unit-decorator branch from b37ee96 to f42e3c4 Compare September 16, 2018 09:45

Apply to more functions

ddc3ee5

jklymak closed this Feb 11, 2019

dstansby mentioned this pull request Sep 29, 2019

[WIP] Add the ability for unit converters to convert back to data with units #12270

Closed

		@@ -3004,14 +3005,13 @@ def _validate_converted_limits(self, limit, convert):
		The limit value after call to convert(), or None if limit is None.

New "accepts units" decorator #10411

New "accepts units" decorator #10411

Conversation

dstansby commented Feb 10, 2018

Choose a reason for hiding this comment

anntzer commented Feb 10, 2018

dstansby commented Feb 10, 2018

tacaswell commented Feb 11, 2018

efiring commented Feb 11, 2018

anntzer commented Feb 11, 2018

dstansby commented Feb 14, 2018

anntzer commented Feb 14, 2018

dstansby commented Apr 17, 2018

dopplershift commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Apr 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dstansby commented Apr 30, 2018

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift commented May 7, 2018

jklymak commented Jul 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Jul 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timhoffm Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tacaswell commented Jul 8, 2018

dstansby commented Jul 9, 2018

dstansby commented Sep 16, 2018

jklymak commented Sep 16, 2018

jklymak commented Feb 11, 2019

dstansby commented Feb 12, 2019

timhoffm Apr 18, 2018 •

edited

Loading

timhoffm Jul 4, 2018 •

edited

Loading

timhoffm Jul 5, 2018 •

edited

Loading