Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
ENH: Add broadcasted hatch to grouped_bar#30683
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
ENH: Add broadcasted hatch to grouped_bar#30683
Conversation
ilakkmanoharan commentedOct 27, 2025
Python 3.11 on macos-14 -- The CI run completed successfully overall — 9,752 tests passed, with only two image comparison tests failing: |
ilakkmanoharan commentedOct 27, 2025
CI Test Summary (macOS 15, Python 3.11, Python 3.12, Python 3.13) 🛠️ Next Steps to Resolve in CI Visually confirm that the failed-diff PNGs show only minor pixel-level changes (such as antialiasing or font edges): Summary: All functionality and numerical behavior are correct. The two failing tests are harmless visual differences in PDF rendering, not functional errors. |
ilakkmanoharan commentedOct 27, 2025
timhoffm commentedOct 27, 2025
Your posts are quite wordy and not to the point. Are they AI generated? No offense, but I cannot detect their intention and reading through lengthy texts is not a good use of core developer time. Please try to be more concise. |
ilakkmanoharan commentedOct 27, 2025
@timhoffm My intention is only to make the explanation more elaborate for clarity and understanding. If my comments appear lengthy, I’d suggest using AI tools to quickly summarize or extract the key points — that might make it easier to grasp the context without spending much time reading through. |
ilakkmanoharan commentedOct 27, 2025
timhoffm commentedOct 27, 2025
I appreciated the intention. But it doesn’t add to clarity and understanding. Instead it hides the relevant information behind a wall of text. Your two lengthy posts would better be written as
Conciseness is key to effective communication.
Sorry, but a hard no to that. We shouldn’t build lengthy answers by AI, and let another AI interpret them. This is a waste of developer time and compute and likely will it not get the key information transferred. I as a human reviewer expect that information is presented in human-targeted format, not that I have to run the answer through AI to digest what it might mean. |
lib/matplotlib/axes/_axes.py Outdated
| lefts= ( | ||
| group_centers-0.5*group_distance+margin_abs | ||
| +i* (bar_width+bar_spacing_abs) | ||
| ) |
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.
nit but can you get your autoformatter to turn off this style of formatting:
(group_centers-0.5*group_distance+margin_abs+i* (bar_width+bar_spacing_abs) )
The mpl codebase doesn't use this convention for parenthesis and it's hard to read. The mpl guidelines can be found athttps://matplotlib.org/devdocs/devel/coding_guide.html
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.
Looks like you missed these lines when you reverted all the other style changes.
| importmatplotlib.pyplotasplt | ||
| deftest_grouped_bar_single_hatch_str(): |
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.
These tests should be with the other bar tests rather than in their own file
matplotlib/lib/matplotlib/tests/test_axes.py
Line 2010 ine0f0ded
| deftest_bar_tick_label_single(): |
Though it might be worth it to factor all the bar tests out into their own file, but I think that's independent from this PR.
ilakkmanoharanOct 27, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Thank you for the comments@story645 :) Do you suggest that I need to add the newly added tests with other bar tests or is it okay for this merge?
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.
They need to go with the other tests in test_axes. Then this pull request still needs a code review and approval by 2 maintainers. The feedback you've been getting so far has been to get your PR into a state where it's feasible to review.
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.
@story645 Okay, I understand.
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.
Moved the grouped-bar hatch tests into lib/matplotlib/tests/test_axes.py alongside the existing bar tests, as suggested by@story645.
Verified locally — all grouped-bar tests (including hatch variants) pass on macOS.
The standalone test_grouped_bar_hatch.py file has been removed for consistency.
ilakkmanoharan commentedOct 27, 2025
I am working on a small project to debug PDF image comparison issues:https://github.com/ilakkmanoharan/matplotlib-pdf-diff-debug |
rcomer commentedOct 28, 2025
You have many style changes in the |
ilakkmanoharan commentedOct 29, 2025
@rcomer The style changes in _axes.pyi were necessary to align the stub with the updated runtime signature of Axes.grouped_bar. Without these adjustments, mypy-stubtest reports an inconsistency (missing parameter "hatch"), as the stub must exactly match the runtime definition for type-checking correctness. These changes ensure that the stub accurately reflects the implementation and prevents future CI failures. |
timhoffm commentedOct 29, 2025
You have changed 184 lines in _axes.pyi, most of which are unrelated to hatch. |
ilakkmanoharan commentedOct 29, 2025
9a6635d to6dfb1b0Compareilakkmanoharan commentedOct 29, 2025
5ab6af5 to3a00551Compare
rcomer left a comment
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 have not looked through the tests yet, as I think we need to agree the desired behaviour before reviewing those.
Uh oh!
There was an error while loading.Please reload this page.
| @@ -0,0 +1,11 @@ | |||
| :orphan: | |||
| grouped_bar hatch broadcasting | |||
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’m not sure "broadcasting" is a helpful term for the end user. They just need to know they can supply a list of hatches and they will be applied one hatch per dataset. An example would also help. Here is a similar entry for stackplot, which is nice and concisehttps://matplotlib.org/stable/users/prev_whats_new/whats_new_3.9.0.html#hatch-parameter-for-stackplot
Note also that this should be a what’s new entry, underdoc/release/next_whats_new/. Or possibly it will not be needed at all if this PR is finalised in time to go in the v3.11 release, sincegrouped_bar itself is new for v3.11.
lib/matplotlib/axes/_axes.py Outdated
| If not specified, the colors from the Axes property cycle will be used. | ||
| hatch : str or sequence of str, optional | ||
| Hatch pattern(s) to apply per dataset. If a single string is given, |
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.
Do we want to support a single string? We do not support a single color for this method. The main point of either colors or hatches is to distinguish between the datasets.
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.
Let's keep this narrow for now and only support sequences. The rare case of same-hatch can be realized on the user side with not too much effort (`hatch=['//'] * 5).
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.
Or even justhatch=['//'], if we keep theitertools.cycle.
lib/matplotlib/axes/_axes.py Outdated
| lefts= ( | ||
| group_centers-0.5*group_distance+margin_abs | ||
| +i* (bar_width+bar_spacing_abs) | ||
| ) |
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.
Looks like you missed these lines when you reverted all the other style changes.
| raiseValueError( | ||
| f"Expected{num_datasets} hatches, got{len(hatch)}" | ||
| ) | ||
| hatches=itertools.cycle(hatch) |
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.
If we have enforced the length of the list then I think we do not needitertools.cycle. But why do we enforce the length for hatches when we didn’t for colors?
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.
There are good arguments for and against enforcing length. Since we currently do not do this in most parts of the library, let's stick to that approach for now. Enforcing length is a separate discussion.
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.
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.
If you confirm that we can allow cycling for this PR, I’ll go ahead and make the required changes.
I’ve also opened an issue to discuss this behavior more broadly and reach a consensus, since it could be a useful improvement across style arguments:
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.
@ilakkmanoharan for this PR, please allow cycling and do not enforce the list length.
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.
ilakkmanoharan commentedNov 2, 2025
The only CI failure on ubuntu-24.04-arm (Python 3.12) is test_webagg, which times out after 120 s — a known flaky interactive-backend test on slow or headless ARM runners. |
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/axes/_axes.py Outdated
| # TODO: do we want to be more restrictive and check lengths? | ||
| colors=itertools.cycle(colors) | ||
| ifhatchisNoneorisinstance(hatch,str): |
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.
In#30683 (comment), we've agreed to limit ourselves to list of hatches and not support broadcasting a single hatch for now. Please correct, also the docstring above and the type stubs.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/axes/_axes.py Outdated
| iforientation=="vertical": | ||
| bc=self.bar(lefts,hs,width=bar_width,align="edge", | ||
| label=label,color=color,**kwargs) | ||
| label=label,color=color,hatch=hatch_pattern,**kwargs) | ||
| else: | ||
| bc=self.barh(lefts,hs,height=bar_width,align="edge", | ||
| label=label,color=color,**kwargs) | ||
| label=label,color=color,hatch=hatch_pattern,**kwargs) |
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.
Since we are passing a lot of common elements, I'd refactor to
common_kwargs = dict(align="edge", label=label, color=color, hatch=hatch_pattern)if orientation == "vertical": bc = self.bar(lefts, hs, width=bar_width, **common_kwargs, **kwargs)else: bc = self.barh(lefts, hs, height=bar_width, **common_kwargs, **kwargs)This reduces the likelihood of introducing errors later; and by that removes the need to run a separate test for orientation=horizontal.
ilakkmanoharanNov 5, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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.
It errored because you passed**kwargs twice. You insertedkwargs intocommon_kwargs and then passed both**common_kwargs and**kwargs tobar.
If you need debugging help, it is OK to ask within the PR.
lib/matplotlib/tests/test_axes.py Outdated
| deftest_grouped_bar_hatch_mixed_orientation(): | ||
| """Ensure hatch works correctly for both vertical and horizontal orientations.""" | ||
| fig, (ax1,ax2)=plt.subplots(1,2) | ||
| x=np.arange(3) | ||
| heights= [np.array([1,2,3]),np.array([2,1,2])] | ||
| hatches= ['//','xx'] | ||
| containers_v=ax1.grouped_bar( | ||
| heights,positions=x,hatch=hatches,orientation="vertical") | ||
| containers_h=ax2.grouped_bar( | ||
| heights,positions=x,hatch=hatches,orientation="horizontal") | ||
| forgi, (cv,ch)inenumerate( | ||
| zip(containers_v.bar_containers,containers_h.bar_containers)): | ||
| forrectincv: | ||
| assertrect.get_hatch()==hatches[gi] | ||
| forrectinch: | ||
| assertrect.get_hatch()==hatches[gi] |
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.
Can be left out if we refactor as above so that there are no separate code paths concerning hatching wrt. the orientation.
ilakkmanoharan commentedNov 3, 2025
…th runtime signature
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
27b9756 toa935895Compare…redundant orientation hatch test
story645 commentedNov 4, 2025
Superceded by#30726 |
PR summary
Closes#30673
Why is this change necessary?
Currently,
Axes.grouped_barsupports color broadcasting (one color per dataset) but not hatch broadcasting.Adding support for broadcasted
hatchvalues improves group distinction and makesgrouped_barconsistent with other multi-dataset plotting APIs such ashist.What problem does it solve?
Without this feature, all grouped bars share the same hatch pattern, reducing visual clarity when color is insufficient or unavailable (e.g., grayscale printing).
This PR allows one hatch per dataset, improving accessibility and plot readability.
What is the reasoning for this implementation?
The design mirrors existing color broadcasting:
This follows Matplotlib’s API design for argument broadcasting and maintains parity with other plotting functions.
Summary of changes
hatchparameter toAxes.grouped_bar.str | Sequence[str] | Noneinput types._axes.pyitype stub for the new argument.test_grouped_bar_hatch.pyto cover:ValueError).grouped_bardocstring to include hatch broadcasting.Example