Skip to content

Conversation

@rcomer
Copy link
Member

@rcomer rcomer commented Dec 4, 2025

PR summary

hist, stackplot and grouped_bar all loop through datasets and call another plotting method repeatedly. This means they have a common requirement to support sequences for parameters such as facecolor and hatch.

hist already has handling for this, which I have factored out into a new helper function. I wasn't sure where to put the helper, so for now it is in its own new private module. I tried putting it in _axes, _api and cbook but all those options gave me circular import problems. The tests from #29719 are now reimagined as cheaper tests on the helper.

Using the helper in stackplot closes #30804.

Using the helper in grouped_bar closes #30673, so this would supersede #30726. I copied the grouped_bar test from there so added @ilakkmanoharan as a co-author. Note that, contrary to agreement in #30726, this does support a single hatch string for grouped_bar. I still don't think a single hatch makes sense for any of these methods, but it is already supported for both hist and stackplot and I do not feel strongly enough to deprecate the existing support or to throw a special case exception in grouped_bar.

grouped_bar example usage:

import matplotlib.pyplot as plt

categories = ['A', 'B']
datasets = {
    'dataset 0': [1, 11],
    'dataset 1': [3, 13],
    'dataset 2': [5, 15],
}

fig, ax = plt.subplots()
ax.grouped_bar(datasets, tick_labels=categories, hatch=['/','+', '.'], facecolor='gray', edgecolor=['r', 'b', 'g'])
ax.legend()
plt.show()
image

Note the helper does not handle color because the methods already have bespoke handling for colors. E.g. stackplot maps colors to facecolor, hist with "step" type maps colors to edgecolor. What should happen if color is in **kwargs probably needs individual consideration for each method.

PR checklist

if colors is None:
colors = [axes._get_lines.get_next_color() for _ in y]

kwargs['hatch'] = hatch
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unsure whether to do this or to remove hatch from the named parameters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really important, but I'm inclined to remove the explicit hatch parameter. The code is simpler without and there's no reason to name this explicitly, but not other style parameters.

@rcomer rcomer added this to the v3.11.0 milestone Dec 4, 2025
@rcomer rcomer marked this pull request as draft December 5, 2025 11:49
@rcomer rcomer marked this pull request as ready for review December 5, 2025 13:53
Comment on lines 21 to 26
ax.stackplot(x, y1, y2,
facecolor=['tab:red', 'tab:blue'],
edgecolor=['black', 'gray'],
linestyle=[':', '--'],
linewidth=[2, 3],
hatch=['.', '*'])
Copy link
Member

@timhoffm timhoffm Dec 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is structurally reasonable to also iterate edge-related parameters, I don't see a practical use case because the horizontal edges overlap, so that you get no clear picture / artifacts if they are different.

I would only do an example if we can come up with an example that may be actually used in practice.
We also don't have to showcase every aspect of the feature, e.g. different line widths should be extermely rare.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I played around a bit, but didn't produce a plot I actually liked, so just removed it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rcomer FYI: There may be arcane use cases for line styling in stackplots, e.g. one could add small finite linewidths in #22393 to compensate antialiasing artifacts of adjacent polygon edges with the same color. So it is good to have these options. But this use case is too special to be mentioned as an example.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Comments are small suggestions.

'edgecolor': next(edgecolors),
'facecolor': next(facecolors),
})
kwargs.update(next(style_gen))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the correct translation keeping existing behavior.

Semi-OT: I'm wondering whether the behavior is logically correct: If patches is empty, we do not advance the style. This means depending on data contents the assignment of styles to datasets may vary.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried removing the if not patch loop, and the only test that failed was this one

def test_hist_unused_labels():
# When a list with one dataset and N elements is provided and N labels, ensure
# that the first label is used for the dataset and all other labels are ignored
fig, ax = plt.subplots()
ax.hist([[1, 2, 3]], label=["values", "unused", "also unused"])
_, labels = ax.get_legend_handles_labels()
assert labels == ["values"]

So I think we are not checking for an empty patch sequence, but rather a None that was created by zip_longest if there are more labels than datasets. I also tried creating histograms where the data is outside the bin range for 'bar', 'step' and 'stepfilled' types, and they all gave my an artist back, so I think there is always a patch for a dataset.

This could all be a bit more obvious though, and I wondering why we don't just enforce that the number of labels is the same as the number of datasets (or perhaps n_labels <= n_datasets).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bar() and grouped_bar() require the same length:

if len(patch_labels) != len(x):
raise ValueError(f'number of labels ({len(patch_labels)}) '
f'does not match number of bars ({len(x)}).')

assert len(labels) == num_datasets

We should do the same for stackplot. If people do not want to label selected elements they should mark this explicitly in the list of labels.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having said the above, the "unused labels" test came in at #28073, whereas the if patch check goes all the way back to 30fddc3. So there may be reasons that are not covered by tests 🫣

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should do the labels check and possible logic changes in patches as a follow-up.

rcomer and others added 7 commits December 7, 2025 11:28
…ther plotting methods

Co-authored-by: Ilakkuvaselvi Manoharan <ilakkmanoharan@gmail.com>
This reverts commit ab39326.
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
rcomer and others added 2 commits December 7, 2025 12:12
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Stackplot does not pass facecolor(s) correctly to fill_between [ENH]: Add custom hatch styling to grouped_bar

2 participants