fix: handle horizontal vs vertical bar plot orientation in subplot iteration - #243
fix: handle horizontal vs vertical bar plot orientation in subplot iteration#243jooyoungseo with Copilot wants to merge 3 commits into
Conversation
…eration Co-authored-by: jooyoungseo <19754711+jooyoungseo@users.noreply.github.com>
7e80e35 to
c81c34a
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a critical bug where maidr.show() would fail with a TypeError: 'NoneType' object is not iterable when used with horizontal bar plots. The issue stemmed from incorrect axis label extraction logic that didn't account for plot orientation differences.
Key changes:
- Added orientation-aware axis selection for extracting category labels (Y-axis for horizontal bars, X-axis for vertical bars)
- Updated value extraction to use appropriate dimension based on orientation (width for horizontal, height for vertical)
- Enhanced error handling with null checks and fallback label generation
| """ | ||
| Extract plot data for bar plots. | ||
|
|
||
| For vertical bar plots, categories are on X-axis and values on Y-axis. | ||
| For horizontal bar plots, categories are on Y-axis and values on X-axis. | ||
|
|
||
| Returns | ||
| ------- | ||
| list | ||
| List of dictionaries containing x and y data points. | ||
| """ |
There was a problem hiding this comment.
The docstring follows a good structure but is missing the Parameters section. According to PEP 257 and NumPy style guidelines, even if a method has no parameters, it's better to be explicit about this in the docstring format for consistency.
| """ | ||
| Extract bar container data with proper orientation handling. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| plot : list[BarContainer] | None | ||
| List of bar containers from the plot. | ||
|
|
||
| Returns | ||
| ------- | ||
| list | None | ||
| List of bar heights/widths, or None if extraction fails. | ||
| """ |
There was a problem hiding this comment.
The docstring is well-structured and follows NumPy style guidelines. However, it would benefit from adding an example section showing how to use this method, especially given the complexity of handling different orientations.
|
@copilot Address review comments. |
Co-authored-by: jooyoungseo <19754711+jooyoungseo@users.noreply.github.com>
|
| this PR | on main |
|---|---|
branch on plot[0].orientation to pick the category axis |
BarPlot._extract_orientation (maidr/core/plot/barplot.py:100), read into self._orientation |
extract_level(self.ax, MaidrKey.X) / MaidrKey.Y by orientation |
self._level_key — MaidrKey.Y if self._is_horizontal else MaidrKey.X (:96) |
get_width() for horizontal, get_height() for vertical |
barplot.py:270-272, same branch |
| default labels when levels are missing | falls back to self._bar_position(patch) per bar (:198) |
main also emits orientation into the schema, which this PR does not.
Measured on the exact script from #239 against current main:
orientation: horz
data: [{'x': 3.7, 'y': 'Matplotlib\n1.48ms'},
{'x': 1.3, 'y': 'Seaborn\n3.26ms'}]
Value on x, category on y — correct for a horizontal bar.
And the issue it targets no longer reproduces
I drove #239's script in Chromium against the bundled runtime: both bars are reachable, with Left/Right. Full trace and the reason it can look otherwise are in this comment — short version, the first arrow press announces where you already are, so pressing ↑ twice gives one point and then "No more data to display", which sounds like the chart holds a single value. That behaviour is identical for a plain vertical ax.bar, so it is not an orientation defect.
Suggested disposition
Close this PR as superseded. Its base is ce7967de from September 2025, so a rebase would be substantial, and the diff would be empty against main either way.
The one thing here main does not have is the guard for levels is None. That is unreachable now — extract_level returns a list and the fallback covers a short one — but if it is worth a belt-and-braces check, it is a two-line change rather than this PR.
Generated by Claude Code
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.