Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
FIX: (broken)bar(h) math before units#12903
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
60264b2 to01f85afComparedopplershift commentedNov 28, 2018
I like the fix. Any reason not to add your example code as a test? |
Uh oh!
There was an error while loading.Please reload this page.
59deaf4 to3e8129bComparejklymak commentedNov 28, 2018
Yep, done! |
a37c072 to98be140Compare45cf538 to39c592fComparejklymak commentedNov 28, 2018 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
EDIT: ooops, had an errant print statement in there what was being appended to the end of each of the stdout streams, and that was different each time. The files themselves are identical. 🙄 |
39c592f to1792e48Compare1792e48 to53458ccComparedopplershift commentedNov 28, 2018
I like the point about |
jklymak commentedNov 29, 2018
That was a test that failed my original change but now passes, so yay tests! But I think that case is covered in test_category |
dopplershift commentedNov 29, 2018
Cool, never mind then. |
lib/matplotlib/axes/_axes.py Outdated
| x0=x | ||
| x=self.convert_xunits(x) | ||
| width=self.convert_xunits(width) | ||
| width=self._convert_dx(x0,x,width,self.convert_xunits) |
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 don't convertx on the line above, and letconvert_dx do the conversion of it (and return it as well if you're worried about the performance of converting twice)? It looks a bit weird that convert_dx takes unitized first and third args, but already converted second arg.
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 did it for performance reasons (both unitized and non unitized are needed for xerr as well. ). But I have no evidence the call takes any significant time.
QuLogic 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.
Some alignment issues; also needs a rebase.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
53458cc to78b1bfeCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
TST: Add test of broken_barhFIX: make bar work with timedeltasTST: check bar and barh work with timedeltaDOC: fix private method docstringFIX: revert list comprehensionTST: fix testFIX: throw error xr not tw-tuple
2c7e237 to1dfdc29Comparejklymak commentedJan 11, 2019
lib/matplotlib/axes/_axes.py Outdated
| else: | ||
| raiseValueError('invalid orientation: %s'%orientation) | ||
| x,height,width,y,linewidth=np.broadcast_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.
Should this go below unit conversion? Otherwise I think units will be dropped if width is a list of unitized stuff?
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.
y= [1 ,2,4]w= [timedelta(hours=2),timedelta(hours=3),timedelta(hours=3)]print(np.broadcast_arrays(y,w))
I don't think this drops units.
[array([1, 2, 4]), array([datetime.timedelta(seconds=7200), datetime.timedelta(seconds=10800), datetime.timedelta(seconds=10800)], dtype=object)]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 will drop stuff like pint units or astropy units.
I remember some discussion a while ago which basically reached the conclusion that we cannot callany numpy function before unit handling (tbh I was rather unhappy about that, but the goal is not to relitigate this here), perhaps@dopplershift will remember this better.
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.
Can someone provide an example? This works fine for all the categorical, unit, and date tests. If we are supporting something that doesn’t survive broadcasting we need to start testing it.
I’m not sure why we broadcast here at all anyway.
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.
AFAICS, broadcasting is used to support scalar parameters such asplt.bar([1, 2, 3], [3,5,2], width=0.5)
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.
For the record,broadcast_arrayswill drop pint units--it might possibly work with astropy since it's an ndarray subclass. Broadcasting itself works fine, it's calling functions that need to allocate new arrays that's the problem; there are currently no hooks to allow overriding what type to use when allocating an empty array.
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.
Agreed - sorry, should have responded here - I did move the broadcast after the convert...
timhoffm commentedJan 12, 2019
Discussion on units and numpy functions pending. Otherwise this looks good. |
anntzer commentedJan 12, 2019
See e.g.#7562 (review),#7562 (comment) for units discussion; some comments are also present e.g. in the errorbar() implementation. (Personally, I think "not being able to use numpy functions at all before units handling" is really not a great policy, but heh...) |
jklymak commentedJan 12, 2019
Yes, this is truly a PITA But see latest for an attempt to deal with all the goofy cases (empty arguments, wrapped arguments, singletons, numpy arrays; am I missing any?) |
jklymak commentedJan 12, 2019
So, can we introduce a testing dependency like pandas for pint so we can have an example of a units package that wraps? They have matplotlib units registry. We can of course skip the test if pint won't install. I think pint is pretty lightweight. W/o that example, its pretty hard to test that we don't break things. |
anntzer commentedJan 12, 2019
Let's not relitigate this here; there have been extremely long threads elsewhere already. |
jklymak commentedJan 12, 2019
Relitigate having |
anntzer commentedJan 12, 2019
General unit support strategy. |
jklymak commentedJan 12, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
importmatplotlib.pyplotaspltimportnumpyasnpimportpintureg=pint.UnitRegistry()ureg.setup_matplotlib(True)x= [1.,2]*ureg.cmheight= [10,20]#ax.bar(datetime(2018, 1, 1), 1,# width=timedelta(hours=3))fig,ax=plt.subplots()ax.bar(x,height,width=[.2]*ureg.cm)plt.show() works fine.
fails with I think thats on I'll note that ax.plot(0.1*ureg.cm,1,'d') works, but gives a |
dopplershift commentedJan 23, 2019
@jklymak For future reference for testing, matplotlib/lib/matplotlib/tests/test_units.py Lines 12 to 37 in4a85eb5
|
jklymak commentedJan 23, 2019
@dopplershift yeah, I know about that. But I wasn't sure it was the same enough.. I still think its worth thinking about adding pint as a (optional) test dependency. Astropy too, if they have a robust units framework. Also your input on#13254 would probably be helpful if you didn't see it. |
dopplershift commentedJan 23, 2019
Well, the whole goal of that class in
(#13254 is in my backlog) |
Uh oh!
There was an error while loading.Please reload this page.
PR Summary
OK, this works w/
bar,barh, andbroken_barh, and I think it even passes the tests....EDIT: now works w/ bar, but see issue below with the pdf (and pgf) determinism tests...EDIT: OK, this may not work: i.e. consider
ax.bar(['a', 'b'], ['c', 'd']). Width defaults to 0.8, so we can't add 0.8 to 'a'. It works fine fordatetimeandtimedeltabecause they have addition defined, but you can't add a float to a string.So, we may have to go back to something like
delta = convert_units_delta(x, delta)and then assume that the converter can do the math.Closes#12862
Alsocloses#11290, though it doesn't address the call for a timedelta formatter, but at least it doesn't crash! Now time deltas work for both
barandbarh.fails on master. Now works by making sure that the timedelta math is done before the unit conversion is done.
PR Checklist