-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix axes.hist bins units #15347
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix axes.hist bins units #15347
Conversation
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.
tiny formatting point but basically looks good.
lib/matplotlib/tests/test_axes.py
Outdated
@@ -1754,6 +1754,27 @@ def test_hist_datetime_datasets(): | |||
ax.hist(data, stacked=False) | |||
|
|||
|
|||
@pytest.mark.parametrize("bins_preprocess", | |||
[lambda bins: 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 think that if you leave out this test case, you can actually assert that the bins are what you expect them to be, since all the other ones return valid representations of the underlying data and should thus set the histogram binning to the same values. Maybe move the None
test to a separate test?
This test does ensure that the current failure mode (raising an error) doesn't occur, but it doesn't ensure that these different types actually work as expected, which would be nice.
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.
On closer inspection, the previous test implicitly covers the None
case, so I think just removing the None
case and then asserting that the bins are equal to mpl.dates.date2num(date_edges)
is whats required, so I did that.
We merged a PR that now causes this to fail for a couple of reasons. The svg failure is not your problem, but the unused import failure in the flake8 check should be fixed before we merge. Srry for the inconvenience! |
42c188d
to
7a1b89e
Compare
4763de1
to
db32b26
Compare
datetime.datetime(2019, 3, 1)] | ||
|
||
fig, ax = plt.subplots() | ||
_, bins, _ = ax.hist(data, bins=bins_preprocess(date_edges), stacked=True) |
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.
Would it be better to parametrize on stacked
as well, to tighten up the implementation here?
Please don't let this comment block the merge, I think overall these tests look good to me.
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 agree we could parameterize this test better, but that also applies to the surrounding hist tests. Maybe merge this and if I find the time this weekend I can do a mini refactor of some tests?
…347-on-v3.2.x Backport PR #15347 on branch v3.2.x (Fix axes.hist bins units)
PR Summary
Issue #15332 raised a problem with the
bins=
argument toaxes.hist
, where it would fail if the bins aredatetime.datetime
objects (although the data can bedatetime.datetime
objects).In
axes.hist
we callconvert_xunits
onx
andbin_range
. This PR also callsconvert_xunits
onbins
if it is a sequence (and not a scalar or string).PR Checklist