FYI I'm working on fixing the test builds now - I think those are the result of my change after all so I'll figure them out.

This looks good to me. But we should make sure our internal units also work, and maybe downstream packages. I don't understand how this could have been wrong before and still passed all the tests.

Can we add a test totest_dates.py to make sure that this does something intelligent w/ dates? I don't see thattest_dates.py ever checksbar().

@story645, does categoricals do OK under this change? I assume categoricals testsbar()?

@ngoldbaum are you able to check that this change is OK from theyt side of things?

@jklymak yup, categoricals tests bar so assuming the tests are run, the tests are still doing what's expected.

@jklymak This isn't a problem because of JPL's units, this is a problem with bar and any units. While testing it with different kinds of units would be nice, I don't see how it's necessary since all units go through the same API. All current tests pass (as the checks show) - I just added more tests to show this case. AFAIK, there were no unit tests for bar with units before. So I'd vote for merging the request and if you feel like adding even more tests after that, that would be great too. In the future, I'll be sending another pull request with even more general plot tests that use units as well.

I don't see how it's necessary since all units go through the same API

I think that's only true if you register your unit system with matplotlib.

I'll try to test this PR with yt's unit system tomorrow.

I don't see how it's necessary since all units go through the same API.

I think that was my point. If this PR is correct, thenbar must not have worked with units before. That it works now it great, I just think it'd be nice to have a quick test. You are touching a general fcn, so I don't think its out of line to ask for tests that prove that it works generally.

@jklymak Sorry - I'm still confused. How am I suppose to prove that something works generally? I ran the unit tests that exist - to me, that's how to prove that it still works. If you feel that there is some specific use case in bar() that needs to be tested, feel free to spell out what it is but I have seen that.

Maybe I'm reading too much into this but to be honest with you, this is a little frustrating. An MPL developer updated bar() recently and broke this feature (I don't know why). If there are general bar unit tests that need to be written, I think they should have done that then. I found the bug, wrote a test case showing the bug, fixed the bug, and made sure all the existing tests still work. From my perspective, that's way more that most users are ever going to supply you with. It feels like you're asking a user (me) to do more work and be more rigorous than the actual developers are doing in the first place.

I'm just asking that we add abar test totest_dates.py, which are the only units we use internally (except for categories, which@story645 indicates are still working fine).

We need such a test. That its not already there is certainly not your fault. Its probably not there becausebar wasn't working before...

But, if we add the test after this PR goes in, and the test fails, then we have to revert this PR, and figure out how to make this PR work. Or, we could just add the test to this PR, which'll prove this PR doesn't break anything, and seems simpler in my opinion.

If you don't care to add the test yourself, would you be troubled if I force-pushed to this PR?

OK - fine. I put this test in:

@image_comparison(baseline_images=['bar_date'], extensions=['png'],                  savefig_kwarg={'dpi': 120}, style='mpl20')def test_bar():    day = datetime.timedelta(1)    x = [0, 1, 2]    w = [1*day, 2*day, 3*day]    b = datetime.datetime(2009, 4, 25)    fig, ax = plt.subplots()    ax.bar(x, w, bottom=b)    ax.set_ylim([b-1*day, b+w[-1]+1*day])

And that fails because the datetime unit converter doesn't handle units of duration (timedelta) values which (IMO) is the correct way to specify widths for a datetime bar plot. If I change the w variable to be floats, it will plot. But - the axis formatter is not set to be dates so it just shows floating point values (this may or may not be a problem depending on what you think should happen). So in my opinion, there are one or two issues with the datetime unit converter class that are exposed with this.

Neither of these really has anything to do with this pull request or the changes I made to bar - they are both issues inside the datetime converter. I'd recommend opening a new bug report and assigning it to whomever is cognizant of the datetime converters.

@TD22057 Thanks a lot for the extra effort. Sorry that it didn't pan out, but this PR definitely doesn't make things worse....

Seems to work just fine with yt's unit system.

I think this PR is more evidence that my "convert units" decorator would be very useful ;) (#10411)

Thanks@TD22057 !