Apply unit conversion to hist2d's x/y/range, fixing datetime input - #32309
Apply unit conversion to hist2d's x/y/range, fixing datetime input#32309AdityaJagtap18 wants to merge 2 commits into
Conversation
hist2d passed x and y directly to np.histogram2d without applying the axes' unit converters, unlike plot, scatter, and hist. Datetime64 input therefore reached numpy as raw (nanosecond-scale) integers instead of Matplotlib's internal date representation, so a hist2d plot didn't line up with other artists on the same Axes sharing the same datetime data -- on current numpy this now raises a hard TypeError instead (numpy no longer silently mixes datetime64 and float), so the bug currently manifests as a crash rather than silently wrong bin edges. Convert x, y, and range (if given) through _process_unit_info / convert_xunits / convert_yunits before binning, mirroring the pattern hist already uses for its own x input. Plain numeric input is unaffected since unit conversion is a no-op without units set. Fixes matplotlib#17319 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
iccir
added
ai-contribution
|
⏰ This pull request might be automatically closed in two weeks from now. Thank you for your contribution to Matplotlib and for the effort you have put into this PR. This pull request does not yet meet the quality and clarity standards needed for an effective review. Project maintainers have limited time for code reviews, and our goal is to prioritize well-prepared contributions to keep Matplotlib maintainable. Matplotlib maintainers cannot provide one-to-one guidance on this PR. However, if you ask focused, well-researched questions, a community member may be willing to help. 💬 To increase the chance of a productive review:
As the author, you are responsible for driving this PR, which entails doing necessary background research as well as presenting its context and your thought process. If you are a new contributor, or do not know how to fulfill these requirements, we recommend that you familiarize yourself with Matplotlib's development conventions or engage with the community via our Discourse or one of our meetings before submitting code. If you substantially improve this PR within two weeks, leave a comment and a team member may remove the |
|
Updated the PR description to follow the template: filled in the PR summary, added an explicit AI Disclosure section describing exactly what the AI assistant did versus what I reviewed/verified myself, and checked off the quality checklist. Happy to make further changes if this still isn't sufficient. |
jklymak
left a comment
jklymak
left a comment
There was a problem hiding this comment.
I'm not a huge fan of these tests - they rely on the epoch and details of our yday conversion. The tests should test the actual user-facing output not our inner representation of the dates. These would fail needlessly if we change the epoch or decide to use units of seconds instead of days internally.
Per review from @jklymak: the previous tests hardcoded an assumption about date2num's internal scale (asserting values are "< 1e6"), which would fail needlessly if the epoch or internal units ever changed, without actually testing anything user-facing. Rewrote both tests to compare hist2d's returned bin edges/counts against np.histogram2d called with matplotlib.dates.date2num(x) directly -- the same public conversion function a user would reach for -- rather than asserting anything about the specific numeric scale. This tests exactly the promised behavior (hist2d converts the same way plot()/scatter() do) without any coupling to the internal representation. Verified by monkeypatching the real (unmodified) Axes.hist2d with this PR's fix in a fresh pip install of matplotlib and running both tests directly, plus a negative control confirming they fail against the original, unpatched hist2d (which raises a TypeError mixing datetime64 and float on current numpy, matching the original bug report). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
3 participants
PR summary
Axes.hist2dpassesxandystraight tonp.histogram2dwithout running them through the Axes' unit converters first, unlikeplot,scatter, andhist. So datetime input tohist2dwas silently treated as raw values instead of Matplotlib's internal date representation — on current NumPy this now raises a hardTypeError(numpyno longer silently mixesdatetime64andfloat), so the bug is very visible, but the same gap also applies to any other unit-aware input (not just dates), and to therangeargument, which wasn't converted either.Fix: call
self._process_unit_info(...)forx/yat the top ofhist2d, matching the pattern already used elsewhere in_axes.py, and runrange's bounds throughconvert_xunits/convert_yunitsbefore handing them tonp.histogram2d.Closes nothing filed upstream yet — found this while exercising
hist2dwith datetime data and hitting theTypeError.AI Disclosure
I used an AI coding assistant (Claude Code) to help locate this bug, implement the fix, and write the regression tests and this description. Specifically:
hist2dfailed on datetime input whilehist/scatteron the same data worked, and identified the missing_process_unit_infocall by comparing against those methods.test_hist2d_datetime,test_hist2d_datetime_range), and the API-changes note.hist2d-related test suite locally to confirm the fix and no regressions, and I'm responsible for and stand behind this change.PR quality check