Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Merged
anntzer merged 5 commits intomatplotlib:masterfromjklymak:fix-broken-barh-units
Jan 13, 2019

Conversation

jklymak
Copy link
Member

@jklymakjklymak commentedNov 27, 2018
edited
Loading

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. considerax.bar(['a', 'b'], ['c', 'd']). Width defaults to 0.8, so we can't add 0.8 to 'a'. It works fine fordatetime andtimedelta because they have addition defined, but you can't add a float to a string.

So, we may have to go back to something likedelta = 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 bothbar andbarh.

importmatplotlib.pyplotaspltfromdatetimeimportdatetime,timedeltalo=datetime(2018,11,9,20,0,0)hi=datetime(2018,11,9,22,0,0)fig,ax=plt.subplots()ax.set_xlim(lo-timedelta(hours=1),hi)ax.set_ylim(0,30)ax.broken_barh([(lo,timedelta(hours=1))], (10,20))plt.show()

fails on master. Now works by making sure that the timedelta math is done before the unit conversion is done.

PR Checklist

  • Has Pytest style unit tests
  • Code isFlake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@dopplershift
Copy link
Contributor

I like the fix. Any reason not to add your example code as a test?

@jklymak
Copy link
MemberAuthor

I like the fix. Any reason not to add your example code as a test?

Yep, done!

@jklymakjklymakforce-pushed thefix-broken-barh-units branch 2 times, most recently froma37c072 to98be140CompareNovember 28, 2018 05:19
@jklymakjklymak changed the titleFIX: brokenbarh math before unitsFIX: (broken)bar(h) math before unitsNov 28, 2018
@jklymakjklymakforce-pushed thefix-broken-barh-units branch 3 times, most recently from45cf538 to39c592fCompareNovember 28, 2018 18:53
@jklymak
Copy link
MemberAuthor

jklymak commentedNov 28, 2018
edited
Loading

Failed tests aretest_backend_pdf.py::test_determinism_hatches which usesax.bar. But I don't see what the error could be. I simplified the test to not include any hatches, and it still fails. I compare the three files this test makes using diff, and diff says they are different, but Adobe Acrobat doesn't report any differences, and I certainly cannot see any.

Any hints as to why these are different at the binary level, but not practically different?

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. 🙄

@dopplershift
Copy link
Contributor

I like the point aboutax.bar(['a', 'b'], ['c', 'd']). Unless it's redundant somehow, could you add that as a test as well?

@jklymak
Copy link
MemberAuthor

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

story645 reacted with laugh emoji

@dopplershift
Copy link
Contributor

Cool, never mind then.

x = self.convert_xunits(x)
width = self.convert_xunits(width)
width = self._convert_dx(x0, x,width, self.convert_xunits)
Copy link
Contributor

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.

Copy link
MemberAuthor

@jklymakjklymakNov 29, 2018
edited
Loading

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.

Copy link
Member

@QuLogicQuLogic left a 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.

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
@jklymak
Copy link
MemberAuthor

Ping@QuLogic@timhoffm@anntzer - you all had comments after two approvals ;-) want to re-approve and merge if you think its an improvement? Thanks!

@@ -2172,23 +2192,25 @@ def bar(self, x, height, width=0.8, bottom=None, *, align="center",
else:
raise ValueError('invalid orientation: %s' % orientation)

x, height, width, y, linewidth = np.broadcast_arrays(
Copy link
Contributor

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?

Copy link
MemberAuthor

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)]

Copy link
Contributor

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.

Copy link
MemberAuthor

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.

Copy link
Member

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)

Copy link
Contributor

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.

Copy link
MemberAuthor

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
Copy link
Member

Discussion on units and numpy functions pending. Otherwise this looks good.

@anntzer
Copy link
Contributor

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
Copy link
MemberAuthor

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
Copy link
MemberAuthor

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
Copy link
Contributor

Let's not relitigate this here; there have been extremely long threads elsewhere already.

@jklymak
Copy link
MemberAuthor

Relitigate havingpint tests?

@anntzer
Copy link
Contributor

General unit support strategy.

@jklymak
Copy link
MemberAuthor

jklymak commentedJan 12, 2019
edited
Loading

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.

ax.bar(x, height, width=.2 * ureg.cm)

fails with

Traceback (most recent call last):  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_axes.py", line 2057, in _convert_dx    dx = [convert(x0 + ddx) - x for ddx in dx]  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_axes.py", line 2057, in <listcomp>    dx = [convert(x0 + ddx) - x for ddx in dx]  File "/Users/jklymak/pint/pint/quantity.py", line 1404, in __getitem__    "supports indexing".format(self._magnitude))TypeError: Neither Quantity object nor its magnitude (0.2)supports indexingDuring handling of the above exception, another exception occurred:Traceback (most recent call last):  File "/Users/jklymak/pint/pint/quantity.py", line 1400, in __getitem__    value = self._magnitude[key]TypeError: 'float' object is not subscriptableDuring handling of the above exception, another exception occurred:Traceback (most recent call last):  File "testBar.py", line 14, in <module>    ax.bar(x, height, width=.2 * ureg.cm)  File "/Users/jklymak/matplotlib/lib/matplotlib/__init__.py", line 1643, in inner    return func(ax, *map(sanitize_sequence, args), **kwargs)  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_axes.py", line 2232, in bar    width = self._convert_dx(width, x0, x, self.convert_xunits)  File "/Users/jklymak/matplotlib/lib/matplotlib/axes/_axes.py", line 2063, in _convert_dx    dx = convert(dx)  File "/Users/jklymak/matplotlib/lib/matplotlib/artist.py", line 180, in convert_xunits    return ax.xaxis.convert_units(x)  File "/Users/jklymak/matplotlib/lib/matplotlib/axis.py", line 1481, in convert_units    if munits.ConversionInterface.is_numlike(x):  File "/Users/jklymak/matplotlib/lib/matplotlib/units.py", line 126, in is_numlike    for thisx in x:  File "/Users/jklymak/pint/pint/quantity.py", line 1404, in __getitem__    "supports indexing".format(self._magnitude))TypeError: Neither Quantity object nor its magnitude (0.2)supports indexing

I think thats onpint, but I could be wrong. I openedhgrecco/pint#751 there to see what they suggest.

I'll note that

ax.plot(0.1*ureg.cm,1,'d')

works, but gives a/Users/jklymak/pint/pint/quantity.py:1377: UnitStrippedWarning: The unit of the quantity is stripped. warning.

@dopplershift
Copy link
Contributor

@jklymak For future reference for testing,lib/matplotlib/tests/test_units.py already has a quick implementation of a class that wraps a numpy array specifically to allow testing things that work like pint:

classQuantity(object):
def__init__(self,data,units):
self.magnitude=data
self.units=units
defto(self,new_units):
factors= {('hours','seconds'):3600, ('minutes','hours'):1/60,
('minutes','seconds'):60, ('feet','miles'):1/5280.,
('feet','inches'):12, ('miles','inches'):12*5280}
ifself.units!=new_units:
mult=factors[self.units,new_units]
returnQuantity(mult*self.magnitude,new_units)
else:
returnQuantity(self.magnitude,self.units)
def__getattr__(self,attr):
returngetattr(self.magnitude,attr)
def__getitem__(self,item):
ifnp.iterable(self.magnitude):
returnQuantity(self.magnitude[item],self.units)
else:
returnQuantity(self.magnitude,self.units)
def__array__(self):
returnnp.asarray(self.magnitude)

@jklymak
Copy link
MemberAuthor

@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
Copy link
Contributor

Well, the whole goal of that class intest_units.py is to be same enough, so...if it's not, we should add to it. What I've been doing there is:

  1. Find failure with pint
  2. Reproduce failure with our class, adding as necessary
  3. Fix problem in matplotlib
  4. Verify fixed with pint

(#13254 is in my backlog)

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dopplershiftdopplershiftdopplershift left review comments

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm left review comments

@tacaswelltacaswelltacaswell approved these changes

@anntzeranntzeranntzer approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.

broken_barh appears not to work with datetime/timedelta objects ax.bar doesn't work correctly when width is a timedelta64 object
6 participants
@jklymak@dopplershift@timhoffm@anntzer@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp