Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork8.1k
Add hatchcolor parameter for Collections#29044
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
story645 commentedOct 30, 2024
Thanks for keeping at this, does#28104 need to be merged first? |
Impaler343 commentedOct 31, 2024
Yes, preferably, so that the changes for Patches can be accurately replicated for Collections |
r3kste commentedOct 31, 2024
There is currently one test which is failing and might need a rewrite: Previous BehaviorIn collections, hatchcolor is set along with edgecolor. In the cases where New BehaviorThe new implementation in this PR has hatchcolor separated from edgecolor and it uses the alpha value specified by the user, regardless of whether edgecolors is specified or not. Reason for New Behavior
In the above test, |
ab4081d to256d735CompareImpaler343 commentedJan 3, 2025
pinging@story645 for review |
r3kste commentedJan 3, 2025
The PDF test for |
c8d7b85 tob081bb9Compare
timhoffm left a comment• 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.
It's somewhat unfortunate that we have to put the parameter at the end of each function signature and make it optional on all lower-level functions. But I suppose that is the price we have to pay to keep API backward compatibility. I would have to check how public all this is and whether we could eventually migrate to a cleaner API.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
r3kste commentedJan 26, 2025
I think that we can't avoid this change, because the default behavior is somewhat inconsistent, as explained in thiscomment.
I think it might be better to update the images, because it looks like the test was previously testing the incorrect behavior. I think that it is better to have the test images reflect the correct behavior, as this test covers the behavior of hatchcolors with alpha values more comprehensively than if we change the test to explicitly set the hatch color to black with alpha = 1.0. When explicitly setting hatchcolor, I wasn't able to force alpha as 1.0 for hatchcolor without using diff --git a/lib/matplotlib/tests/test_axes.py b/lib/matplotlib/tests/test_axes.pyindex 38857e846c..5764d3fff6 100644--- a/lib/matplotlib/tests/test_axes.py+++ b/lib/matplotlib/tests/test_axes.py@@ -2651,6 +2651,9 @@ def test_contour_hatching(): cmap=mpl.colormaps['gray'], extend='both', alpha=0.5)+ for c in ax.collections:+ c._hatchcolors = mpl.colors.to_rgba_array('black', 1.0)+ @image_comparison( ['contour_colorbar'], style='mpl20', Here is why I think that the test was previously testing the incorrect behavior:
|
anntzer commentedFeb 14, 2025
I think that if we want to move this PR forward, the realistic short-term approach is to do API support sniffing via the empty call (and not even warn on third-parties for now, at least until we can agree on the long-term plan and likely also add support for multiple hatches). |
… suggested changes.
anntzer commentedFeb 17, 2025
Thanks@Impaler343 for adding the API sniffing. Unfortunately this is not enough, because you choose to just not pass hatchcolors at all if they are unsupported. While the idea of just drawing single-color hatches on 3rd-party backends may be reasonable (I'm fine with that), it means that these backends will try to consult rcParams["hatch.color"] to select a color; and since#28104 they can see the new unnormalized value "edge", for which there was no support before, and they will crash there. At least this value should be normalized (e.g. to the first edgecolor, I guess); ideally if hatchcolors is not supported, the draw_path_collection call should be expanded as in ContourSet.draw to draw one path at a time. |
anntzer commentedFeb 22, 2025
This works now on mplcairo. Thanks for going through all the work! |
r3kste commentedMar 15, 2025
@anntzer Are there any other changes expected? Or is this ready to be merged? |
Uh oh!
There was an error while loading.Please reload this page.
anntzer 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.
Just one last suggestion to improve on the future extensibility (approval is conditional on at least discussing it).
I still don't really like the API but it's also unrealistic to hope for a full rewrite here, so let's go with it.
r3kste commentedMar 21, 2025
I agree that this API will need to be changed in the future. @timhoffm any suggestions? |
dstansby 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.
Thanks for this PR - it looks great overall, and the examples were super nice to understand the new feature.
There's a few minor points I think can be improved, and some questions I had - I've left my comments inline in the code.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
| "screen",hatchcolors=self.get_hatchcolor() | ||
| ) | ||
| exceptTypeError: | ||
| hatchcolors_arg_supported=False |
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.
codecov is saying that these lines aren't covered (and several other lines in this file). Could you add some tests for these lines? They don't have to be figure tests, they can just test that the code works without erroring.
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 lines are run only in the case of a third-party backend. However, I am not sure how to test this within the current testing framework of matplotlib, without a third-party backend.
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.
You can make a mock third party backend to call the function - there are some examples of other mocks in the tests
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
lib/matplotlib/collections.py Outdated
| # The current new API of draw_path_collection() is provisional | ||
| # and will be changed in a future PR. | ||
| # Find whether renderer.draw_path_collection() takes hatchcolor parameter |
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.
| # Find whether renderer.draw_path_collection() takes hatchcolor parameter | |
| # Find whether renderer.draw_path_collection() takes hatchcolor parameter. | |
| # Since third-party implementations of draw_path_collection() may not be | |
| # introspectable, e.g. with inspect.signature, the only way is to try and | |
| # call this with the hatchcolors parameter. |
| "screen") | ||
| "screen",hatchcolors=self.get_hatchcolor() | ||
| ) | ||
| exceptTypeError: |
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's worth explicitly documenting this in the comment, see suggestion above.
lib/matplotlib/backend_bases.py Outdated
| defdraw_quad_mesh(self,gc,master_transform,meshWidth,meshHeight, | ||
| coordinates,offsets,offsetTrans,facecolors, | ||
| antialiased,edgecolors): | ||
| antialiased,edgecolors,hatchcolors=None): |
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.
Is this a red herring? From what I understand from#29044 (comment), making hatchcolors optional (or even kw-only) does not buy us anything in terms of better API compatibility: This is reimplemented by backends. Some may have implemented ahatchcolors parameter, others may not. It is only called from our matplotlib code, so we anyway have to deal with it being available or not (and if it's available, we always want to pass hatchcolors). There shouldn't be any third-party/user code needing to call this function.
If that's correct, an additional positional parameter would have the same effect. And then I'd prefer that because it keeps the whole function consistent.
ping@anntzer Am I correct here?
Edit: There seem to be very few "real" usages ofdraw_path_collection (I've tried to filter outforks or copies of our files):
https://github.com/search?q=%2F%5C.draw_path_collection%5C%28%2F+language%3APython+NOT+is%3Afork+NOT+repo%3Amatplotlib%2Fmatplotlib+NOT+path%3A**%2Fbackend_bases.py+NOT+path%3A**%2Fcollections.py++NOT+path%3A**%2Fpatheffects.py+NOT+path%3A**%2Fbackend_agg.py+NOT+path%3A**%2Fbackend_svg.py+NOT+path%3A**%2Fbackend_pdf.py++NOT+path%3A**%2Fbackend_ps.py+NOT+path%3A**%2Fbackend_macosx.py++NOT+path%3A**%2Fleft.py+NOT+path%3A**%2Fright.py++NOT+path%3A**%2Fmerge.py++NOT+path%3A**%2Fbase.py+NOT+path%3A**%2Fsite-packages%2F**&type=code
I think making this optional is meaningful for a start because it will support some very rare use cases. But in terms of API consistency, I'd like to move this to a regular parameter in the future, i.e.
- not make this kw-only right now
- some time later deprecate calling this function without
hatchcolors
anntzer commentedMar 24, 2025
re: making hatchcolors optional and API compatibility: |
timhoffm commentedMar 24, 2025
Thanks. I agreeoptional is set to begin with for compatibility. Questions are:
|
anntzer commentedMar 24, 2025
It makes it easier to later include parametersbefore it, e.g.
Let's not for now. As argued above I think the API introduced in this PR is actually not so great (but still the best that can be done without an in-depth reworking of draw_path_collection), so e.g. I'd rather not implement support for it at all in mplcairo (and instead rely on Matplotlib's fallback implementation) until a better API comes. Likewise I don't think we should encourage any third parties to implement it. |
9800478 to0c557e4Compare0c557e4 to2e4784bComparelib/matplotlib/collections.py Outdated
| path_ids=renderer._iter_collection_raw_paths( | ||
| transform.frozen(),ipaths,self.get_transforms()) | ||
| forxo,yo,path_id,gc0,rgbFaceinrenderer._iter_collection( | ||
| gc,list(path_ids),*args,hatchcolors=self.get_hatchcolor(), | ||
| ): | ||
| path,transform=path_id | ||
| ifxo!=0oryo!=0: | ||
| transform=transform.frozen() | ||
| transform.translate(xo,yo) | ||
| renderer.draw_path(gc0,path,transform,rgbFace) |
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.
For better backward-compatibility/performance, would it be reasonable to split the "backend does not support hatchcolors" into
if no_hatches_needed: renderer_draw_path_collection(...) # no hatch colors passedelse: # unroll the collection to draw paths in a loopI'm unclear how performance-critical this is and whether we can unversally force the collection unrolling on backends that do not support hatchcolors.
ping@anntzer
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.
Good catch. I agree that only going through the unrolled version when hatchcolors are actually passed would likely be much better performance-wise.
65d2818 intomatplotlib:mainUh oh!
There was an error while loading.Please reload this page.
timhoffm commentedMar 31, 2025
Thanks@r3kste for going through all the nitty gritty details with us! |




Uh oh!
There was an error while loading.Please reload this page.
PR summary
Follow on PR to#28104 helping to fix issues#26074 and#7059
Added
hatchcolorparameter for Collections, to be able to separately controledgecolors andhatchcolors.The fallback logic for
hatchcoloris identical to the previous PR, where it follows this precedence order:hatchcolorparameter ->hatch.colorrcParam -> inherit fromedgecolorsifhatch.colorisedge -> default topatch.edgecolorrcParam ifedgecolorsis not specified.hatchcolorparameter is specified, it will take precedence overhatch.colorrcParam andedgecolors.hatchcoloris not specified, it will try to usehatch.colorrcParam. If this rcParam is a valid color, it will be used for the hatches.hatch.colorrcParam isedge, the hatches will inherit the color fromedgecolorsif it is specified.edgecolorsis not specified, the hatches will default topatch.edgecolorrcParam.PR checklist