Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
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
| 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.
anntzer commentedFeb 10, 2018
+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? |
dstansby commentedFeb 10, 2018
I'm not sure that would work, because eventually it would be good to apply this decorator to e.g. |
tacaswell commentedFeb 11, 2018
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 commentedFeb 11, 2018
It seems to me that early conversion is necessary, so that you can proceed to work with numbers. |
anntzer commentedFeb 11, 2018
re: ax.set_xlim: that one can't take a data kwarg anyways, so I don't see how things are related? |
dstansby commentedFeb 14, 2018
@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 commentedFeb 14, 2018
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 tof44ee44Comparedstansby commentedApr 17, 2018
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 commentedApr 17, 2018
It seems like an improvement to me. |
| has_data= (('data'inarguments)and | ||
| (arguments['data']isnotNone)) | ||
| ifhas_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 to07bf0d6Comparedstansby commentedApr 18, 2018
I 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 | ||
| """ | ||
| defdecorator(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
dstansby commentedApr 30, 2018
Reviews needed bump! |
dopplershift left a 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.
Just found one docstring that needs updating. Otherwise, this is 👍 from me.
lib/matplotlib/axes/_base.py Outdated
| Returns | ||
| ------- | ||
| 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?
| """ | ||
| returntuple(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.)
dopplershift commentedMay 7, 2018
Need another sign off to merge |
jklymak commentedJul 4, 2018
ping@anntzer,@tacaswell - this seems good to me, but is too technical for me to know all the implications. |
| defdecorator(func): | ||
| @functools.wraps(func) | ||
| defwrapper(*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.
| has_data= (('data'inarguments)and | ||
| (arguments['data']isnotNone)) | ||
| ifhas_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 | ||
| forarginconvert_x: | ||
| ifhas_dataandarguments[arg]indata: |
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.
| frommatplotlib.cbookimportiterable,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!
tacaswell commentedJul 8, 2018
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 commentedJul 9, 2018
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
dstansby commentedSep 16, 2018
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 commentedSep 16, 2018
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 commentedFeb 11, 2019
@dstansby is this still active? Closing, but feel free to re-open! |
dstansby commentedFeb 12, 2019
🤷♂️ 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.