Unit handling improvements #18531
Unit handling improvements #18531
Conversation
db696a4 to
98ebd9d
Compare
lib/matplotlib/axes/_base.py
Outdated
| datasets : dict | ||
| Mapping of axis names (per `._get_axis_map`) to datasets. | ||
| kwargs : dict | ||
| Other parameters from which unit info may be popped. Note that |
There was a problem hiding this comment.
This is a bit vague. Can we be more explicit? Which entries will be popped under which conditions?
|
I am a bit concerned about the dict order -> list return mapping, but am not going to block over it. |
|
Actually I realized passing a list of pairs is nicer for another reason: sometimes you need to convert multiple datasets on a single axis, and convert_units does not necessarily support converting a ragged array of them, so we need to instead pass them one at a time. |
|
it was too hard to maintain a sensible commit log so I just squashed everything. |
|
|
||
| Parameters | ||
| ---------- | ||
| datasets : list |
There was a problem hiding this comment.
You might consider accepting a single tuple as well. This seems to be a common use case and results in a little less cluttered code. But no strong opinion, it's interal anyway.
There was a problem hiding this comment.
I don't think it really gains us much, and would make the implementation (and documentation) more complex. Let's say I wouldn't block a PR implementing that, but won't do it here.
There was a problem hiding this comment.
Fair enough. Even though the added implementation complexity is just
if isinstance(datasets, tuple):
datasets = [datasets]
... and fix incorrect converter used for horizontal histograms.
PR Summary
1st commit: Change the signature of
_process_unit_infoto make it work both for the 2D and the 3D cases (instead of passingxdata,ydataas separate named parameters, pass a single axis-to-dataset mapping). This avoids having to redefine it for the 3D case.2nd commit: Make
_process_unit_infodirectly perform unit conversion as well (nearly all calls to_process_unit_infoare followed by a call toconvert_x/yunits, and it semantically makes sense to have both of them together, too).3rd commit: Fix incorrect unit application for horizontal histograms (which should use the y units, not the x units).
PR Checklist