-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
First attempt for individual hatching styles for stackplot #27158
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
Conversation
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 opening your first PR into Matplotlib!
If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
Thanks for the fast work @nbarlowATI and for checking this works for the example in the issue. I'm unsure whether passing a single string will work at the moment. Could you check what happens if you pass This will also need some tests. Please see here for general guidance on testing. You might also like to look at the tests added at #24470, where a similar change was made for the Please mark this as "ready for review" when you are ready (things marked as draft often get overlooked). In the meantime, if you need help, feel free to ask questions here. Or you may prefer to ask them in our incubator gitter room. |
lib/matplotlib/stackplot.py
Outdated
@@ -76,6 +83,14 @@ def stackplot(axes, x, *args, | |||
else: | |||
colors = (axes._get_lines.get_next_color() for _ in y) | |||
|
|||
if hatch: | |||
if isinstance(hatch, Iterable): |
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.
be careful because hatches can be multi-char strings, and strings are Iterable.
I think what you may want is
if isinstance(hatch, (str, None)):
hatch = itertools.cycle([hatch])
else:
hatch = itertools.cycle(hatch)
I think that should handle even the None case (also " "
is not a valid hatch pattern and is causing warnings)
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.
Thanks, and good point! You're right about the strings being iterables, and I think your logic is the right choice (though I had to modify slightly because None
doesn't count as a type so can't be used in isinstance
).
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.
Ahh, str | None
would work (but is py3.10+ only so we can't use it).
builtins.NoneType
or type(None)
would also work, but is used less commonly.
Type hints allow None
as shorthand, but not isinstance
, apparently.
It'll also need a whats new example - what you have here is fine and you can also use the what's new in #24470 as an example. ETA: also I'm psyched, this is great and we should vectorize all the things 😄 |
I was thinking perhaps this change is a good chance to introduce the following API (provided this is clear and acceptable):
|
I understand why you're asking for this but I'm concerned this is a bit magic cause this is not a thing we do for any variables (list of color vary by density). Also if there was support for this, it'd have to work for all the places that take a list of hatches. And I wonder if this would be better achieved via custom hatch objects?
|
No problem. I was just throwing the idea for the debate. I'm very much in favour of continuous improvement so even if there's a chance my proposal would be considered for implementation but hindered the current work, it's probably best to get out the current change (support for list of hatches) and then go back to the other ideas in the next iterations... |
I added a test in |
last thing left is fixing the stubtest by adding this variable - which I think is matplotlib/lib/matplotlib/streamplot.pyi Line 11 in e501543
|
@@ -3792,12 +3792,15 @@ def spy( | |||
|
|||
# Autogenerated by boilerplate.py. Do not edit as changes will be lost. |
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.
As the comment says, the signatures here are autogenerated
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.
Ah yes, sorry! I think I ran boilerplate.py myself in the hope of fixing the stub github action, as I didn't know about the .pyi file... Should I revert this commit, or will the autogeneration take care of it anyway?
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 think you are supposed run boilerplate.py yourself, and this comment is meant to indicate that you shouldn't manually edit the type hints here.
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 think Ruth is right given I edited this file in my PR too and we never call boilerplate.py as part of our build/install process. Sorry & I'll open up a follow up issue on documenting how this is used!
lib/matplotlib/stackplot.py
Outdated
A sequence of hatching styles (see reference in | ||
https://matplotlib.org/devdocs/gallery/shapes_and_collections/hatch_style_reference.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.
A sequence of hatching styles (see reference in | |
https://matplotlib.org/devdocs/gallery/shapes_and_collections/hatch_style_reference.html) | |
A sequence of hatching styles. See :ref:`sphx_gallery_shapes_and_collections_hatch_style_reference.py |
Personal bias against parentheticals & we always cross-reference when possible so that the link goes to the tutorial in the same version of the docs
https://sphinx-gallery.github.io/stable/advanced.html#cross-referencing
But also double check that link by building the doc cause I may have done something wonky
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.
OK makes sense. For me, the above ':ref:' link didn't quite work in my locally built version of the docs, but I copied something very similar (using ':doc:') from the pie chart docstring, that seems to work.
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 your patience w/ my "oh, another thing" 😅
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.
No problem (I'm a big fan of Columbo!), and thanks for your help! :)
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.
Yay, it's got code and tests and typing and a whats new 🥳
Slight optional wording suggestion, but I think it's good to go. Also do you want to rebase or do you want one of us to?
Thanks! Re: rebasing, it would be great if one of you could do it - I'm away for the next few days (but I could also do it when I'm back...) |
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.
Thanks for your work on this @nbarlowATI. I just have one minor comment.
This will also need squashing down to one commit if you are confident to do so, otherwise we can handle that on merge. Edit: oops I see you already discussed that. |
Hi, I'm back online now, and happy to have a go at the rebase. Just to confirm, is the idea to both:
|
This one please! Since there are no conflicts between your branch and We have some guidance here if you need it |
Don't add complication you don't need of course, but |
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.
Thanks @nbarlowATI and congratulations on your first PR in Matplotlib! We hope to hear from you again.
PR summary
Closes #27146 this modifies
stackplot.py
to treat thehatch
argument tostackplot
in the same way ascolors
, i.e. if a list is provided, it will cycle through those hatch styles for the different data in the stack.I believe the existing behaviour is preserved - if a single style (e.g. "x") is provided, this will be used for all elements of the stack, while if no "hatch" argument is passed, it will apply hatch style " " to all elements (though perhaps this isn't the best way of doing this?)
Using the example code in the original Issue from @pawjast :
I get the following:

Let me know if this looks OK, and if so I can make a start on updating documentation and tests.
PR checklist