Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
New "accepts units" decorator#10411
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
.travis.yml Outdated
| @@ -65,19 +65,6 @@ env: | |||
| matrix: | |||
| include: | |||
| - python: 2.7 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've got rid of the py27 build to avoid having to do funny things with importingsignature.
+1 for the general idea, but may be easier to move the decoratorunder preprocess_data so that you don't have to fiddle with the data kwarg yourself? |
I'm not sure that would work, because eventually it would be good to apply this decorator to e.g. |
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) |
It seems to me that early conversion is necessary, so that you can proceed to work with numbers. |
re: ax.set_xlim: that one can't take a data kwarg anyways, so I don't see how things are related? |
@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. |
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) |
3fdb8d7 tof44ee44CompareI 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? |
It seems like an improvement to me. |
lib/matplotlib/units.py Outdated
| has_data = (('data' in arguments) and | ||
| (arguments['data'] is not None)) | ||
| if has_data: | ||
| data = arguments['data'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Perhaps move the decorator under _preprocess_data, so that you don't have to reproduce its logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 adata=None in the signatures and only let_preprocess_data work on that. Feels less magical and simplifies the_preprocess_data code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think that would work, since the idea is that if I doax.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 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Don't think that's done in_preprocess_data. If I understand the code correctly, this is fromcbook.index_of() viaAxes._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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Separate topic: If I'm correct, the four previous lines can be simplified todata = arguments.get('data')
and usedata is not None instead of has_data in the following.
8a7f1a9 to07bf0d6CompareI think that's all the easy methods taken care of, feel free to review! |
| convert_x, convert_y : list | ||
| A list of integers or strings, indicating the arguments to be converted | ||
| """ | ||
| def decorator(func): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The current implementation hides the docstring of the decorated functions. Not an expert on decorators, but it might be sufficient to decoratedecorator(func) with@wraps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Eeek good catch, will see what I can do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Doing the@wraps seems to work
Reviews needed bump! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just found one docstring that needs updating. Otherwise, this is 👍 from me.
lib/matplotlib/axes/_base.py Outdated
| @@ -3004,14 +3005,13 @@ def _validate_converted_limits(self, limit, convert): | |||
| The limit value after call to convert(), or None if limit is None. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't think this function returns anything now, no?
| @@ -3356,6 +3350,7 @@ def get_ylim(self): | |||
| """ | |||
| return tuple(self.viewLim.intervaly) | |||
| @munits._accepts_units(convert_y=['bottom', 'top']) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 originalset_ylim() code.)
Need another sign off to merge |
ping@anntzer,@tacaswell - this seems good to me, but is too technical for me to know all the implications. |
| def decorator(func): | ||
| @functools.wraps(func) | ||
| def wrapper(*args, **kwargs): | ||
| axes = args[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It should be noted in the docstring that the first parameter must be an Axes.
lib/matplotlib/units.py Outdated
| has_data = (('data' in arguments) and | ||
| (arguments['data'] is not None)) | ||
| if has_data: | ||
| data = arguments['data'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Separate topic: If I'm correct, the four previous lines can be simplified todata = arguments.get('data')
and usedata is not None instead of has_data in the following.
| # unit info, convert argument, and replace in *arguments* with | ||
| # converted values | ||
| for arg in convert_x: | ||
| if has_data and arguments[arg] in data: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This does not work for structured numpy arrays:
>>> a = np.array([(1, 2), (3, 4)], dtype=[('x', float), ('y', float)])>>> 'x' in aFalse>>> print(a['x'])[1, 3]See also#10872 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Do you know what works instead? Do we officially support structured numpy arrays?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
lib/matplotlib/units.py Outdated
| import numpy as np | ||
| from matplotlib.cbook import iterable, safe_first_element | ||
| def _accepts_units(convert_x=[], convert_y=[]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
no mutable defaults please!
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? |
I'm happy to let this slip to |
Proof of concept "accepts units" decoratorAdd helper functionMssing commasFix docstrings disappearingIgnore None when converting dataRemove returns section in doc
Add some more unit decoratorsAdd unit decorator to mplot3d
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. |
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. |
@dstansby is this still active? Closing, but feel free to re-open! |
🤷♂️ 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. |
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
xandxlimcould 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.