-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Consolidate style parameter handling for plotting methods that call other plotting methods #30808
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
c624ca4
Consolidate style parameter handling for plotting methods that call o…
rcomer 292b3e5
simplify
rcomer 9de925e
Revert "simplify"
rcomer 9d77726
remove plot from whatsnew
rcomer 2b5ee9a
take hatch out of stackplot named parameters
rcomer 55d1a5e
Apply suggestions from timhoffm code review
rcomer ea8ac82
fix linting and stackplot test
rcomer c49771d
punctuation
rcomer c3f1633
group versionadded info
rcomer 11f44ee
remove named hatch param from pyplot.stackplot
rcomer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| Stackplot styling | ||
| ----------------- | ||
|
|
||
| `~.Axes.stackplot` now accepts sequences for the style parameters *facecolor*, | ||
| *edgecolor*, *linestyle*, and *linewidth*, similar to how the *hatch* parameter | ||
| is already handled. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| import collections.abc | ||
| import itertools | ||
|
|
||
| import numpy as np | ||
|
|
||
| import matplotlib.cbook as cbook | ||
| import matplotlib.colors as mcolors | ||
| import matplotlib.lines as mlines | ||
|
|
||
|
|
||
| def check_non_empty(key, value): | ||
| """Raise a TypeError if an empty sequence is passed""" | ||
| if (not cbook.is_scalar_or_string(value) and | ||
| isinstance(value, collections.abc.Sized) and len(value) == 0): | ||
| raise TypeError(f'{key} must not be an empty sequence') | ||
|
|
||
|
|
||
| def style_generator(kw): | ||
| """ | ||
| Helper for handling style sequences (e.g. facecolor=['r', 'b', 'k']) within plotting | ||
| methods that repeatedly call other plotting methods (e.g. hist, stackplot). Remove | ||
| style keywords from the given dictionary. Return the reduced dictionary together | ||
| with a generator which provides a series of dictionaries to be used in each call to | ||
| the wrapped function. | ||
| """ | ||
| kw_iterators = {} | ||
| remaining_kw = {} | ||
| for key, value in kw.items(): | ||
| if key in ['facecolor', 'edgecolor']: | ||
| if value is None or cbook._str_lower_equal(value, 'none'): | ||
| kw_iterators[key] = itertools.repeat(value) | ||
| else: | ||
| check_non_empty(key, value) | ||
| kw_iterators[key] = itertools.cycle(mcolors.to_rgba_array(value)) | ||
|
|
||
| elif key in ['hatch', 'linewidth']: | ||
| check_non_empty(key, value) | ||
| kw_iterators[key] = itertools.cycle(np.atleast_1d(value)) | ||
|
|
||
| elif key == 'linestyle': | ||
| check_non_empty(key, value) | ||
| kw_iterators[key] = itertools.cycle(mlines._get_dash_patterns(value)) | ||
|
|
||
| else: | ||
| remaining_kw[key] = value | ||
|
|
||
| def style_gen(): | ||
| while True: | ||
| yield {key: next(val) for key, val in kw_iterators.items()} | ||
|
|
||
| return remaining_kw, style_gen() |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,83 @@ | ||
| import pytest | ||
|
|
||
| import matplotlib.colors as mcolors | ||
| from matplotlib.lines import _get_dash_pattern | ||
| from matplotlib._style_helpers import style_generator | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('key, value', [('facecolor', ["b", "g", "r"]), | ||
| ('edgecolor', ["b", "g", "r"]), | ||
| ('hatch', ["/", "\\", "."]), | ||
| ('linestyle', ["-", "--", ":"]), | ||
| ('linewidth', [1, 1.5, 2])]) | ||
| def test_style_generator_list(key, value): | ||
rcomer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| """Test that style parameter lists are distributed to the generator.""" | ||
| kw = {'foo': 12, key: value} | ||
| new_kw, gen = style_generator(kw) | ||
|
|
||
| assert new_kw == {'foo': 12} | ||
|
|
||
| for v in value * 2: # Result should repeat | ||
| style_dict = next(gen) | ||
| assert len(style_dict) == 1 | ||
| if key.endswith('color'): | ||
| assert mcolors.same_color(v, style_dict[key]) | ||
| elif key == 'linestyle': | ||
| assert _get_dash_pattern(v) == style_dict[key] | ||
| else: | ||
| assert v == style_dict[key] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('key, value', [('facecolor', "b"), | ||
| ('edgecolor', "b"), | ||
| ('hatch', "/"), | ||
| ('linestyle', "-"), | ||
| ('linewidth', 1)]) | ||
| def test_style_generator_single(key, value): | ||
rcomer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| """Test that single-value style parameters are distributed to the generator.""" | ||
| kw = {'foo': 12, key: value} | ||
| new_kw, gen = style_generator(kw) | ||
|
|
||
| assert new_kw == {'foo': 12} | ||
| for _ in range(2): # Result should repeat | ||
| style_dict = next(gen) | ||
| if key.endswith('color'): | ||
| assert mcolors.same_color(value, style_dict[key]) | ||
| elif key == 'linestyle': | ||
| assert _get_dash_pattern(value) == style_dict[key] | ||
| else: | ||
| assert value == style_dict[key] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize('key', ['facecolor', 'hatch', 'linestyle']) | ||
| def test_style_generator_raises_on_empty_style_parameter_list(key): | ||
| kw = {key: []} | ||
| with pytest.raises(TypeError, match=f'{key} must not be an empty sequence'): | ||
| style_generator(kw) | ||
|
|
||
|
|
||
| def test_style_generator_sequence_type_styles(): | ||
| """ | ||
| Test that sequence type style values are detected as single value | ||
| and passed to a all elements of the generator. | ||
| """ | ||
| kw = {'facecolor': ('r', 0.5), | ||
rcomer marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 'edgecolor': [0.5, 0.5, 0.5], | ||
| 'linestyle': (0, (1, 1))} | ||
|
|
||
| _, gen = style_generator(kw) | ||
| for _ in range(2): # Result should repeat | ||
| style_dict = next(gen) | ||
| mcolors.same_color(kw['facecolor'], style_dict['facecolor']) | ||
| mcolors.same_color(kw['edgecolor'], style_dict['edgecolor']) | ||
| kw['linestyle'] == style_dict['linestyle'] | ||
|
|
||
|
|
||
| def test_style_generator_none(): | ||
| kw = {'facecolor': 'none', | ||
| 'edgecolor': 'none'} | ||
| _, gen = style_generator(kw) | ||
| for _ in range(2): # Result should repeat | ||
| style_dict = next(gen) | ||
| assert style_dict['facecolor'] == 'none' | ||
| assert style_dict['edgecolor'] == 'none' | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is the correct translation keeping existing behavior.
Semi-OT: I'm wondering whether the behavior is logically correct: If
patchesis empty, we do not advance the style. This means depending on data contents the assignment of styles to datasets may vary.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.
I tried removing the
if not patchloop, and the only test that failed was this onematplotlib/lib/matplotlib/tests/test_axes.py
Lines 5116 to 5122 in 89aa371
So I think we are not checking for an empty patch sequence, but rather a
Nonethat was created byzip_longestif there are more labels than datasets. I also tried creating histograms where the data are outside the bin range for 'bar', 'step' and 'stepfilled' types, and they all gave me an artist back, so I think there is always apatchfor 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).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.
bar()andgrouped_bar()require the same length:matplotlib/lib/matplotlib/axes/_axes.py
Lines 2601 to 2603 in 4aeb773
matplotlib/lib/matplotlib/axes/_axes.py
Line 3331 in 89aa371
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.
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.
Having said the above, the "unused labels" test came in at #28073, whereas the
if patchcheck goes all the way back to 30fddc3. So there may be reasons that are not covered by tests 🫣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.
We should do the labels check and possible logic changes in patches as a follow-up.