Add hatchcolor parameter for Collections #29044

Impaler343 · 2024-10-30T18:44:51Z

PR summary

Follow on PR to #28104 helping to fix issues #26074 and #7059
Added hatchcolor parameter for Collections, to be able to separately control edgecolors and hatchcolors.

The fallback logic for hatchcolor is identical to the previous PR, where it follows this precedence order:

hatchcolor parameter -> hatch.color rcParam -> inherit from edgecolors if hatch.color is edge -> default to patch.edgecolor rcParam if edgecolors is not specified.

If hatchcolor parameter is specified, it will take precedence over hatch.color rcParam and edgecolors.
If hatchcolor is not specified, it will try to use hatch.color rcParam. If this rcParam is a valid color, it will be used for the hatches.
If hatch.color rcParam is edge, the hatches will inherit the color from edgecolors if it is specified.
If edgecolors is not specified, the hatches will default to patch.edgecolor rcParam.

Need to rewrite the failing test(s)
Need to write tests
What's New note
Example demos

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
Plotting related features are demonstrated in an example
New Features and API Changes are noted with a directive and release note
Documentation complies with general and docstring guidelines

story645 · 2024-10-30T20:00:29Z

Thanks for keeping at this, does #28104 need to be merged first?

Impaler343 · 2024-10-31T02:37:40Z

Yes, preferably, so that the changes for Patches can be accurately replicated for Collections

r3kste · 2024-10-31T03:43:34Z

There is currently one test which is failing and might need a rewrite: lib/matplotlib/tests/test_axes.py::test_contour_hatching

Previous Behavior

In collections, hatchcolor is set along with edgecolor.

In the cases where edgecolors is explicitly specified by the user, hatchcolor is set to the first color in edgecolors and it also uses the alpha value specified by the user. However, if edgecolors is not specified, hatchcolor uses mpl.rcParams['hatch.color'] for the color, but it doesn't use the alpha value specified by the user.

New Behavior

The 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

alpha works on hatchcolor when edgecolors is specified. Therefore, I believe that it should also work when edgecolors is not specified.

In the above test, alpha is set to 0.5. Ideally, the hatch should have an alpha value of 0.5, but it has the default alpha value of 1.0.

Impaler343 · 2025-01-03T12:19:45Z

pinging @story645 for review

r3kste · 2025-01-03T12:37:40Z

The PDF test for test_axes.py::test_contour_hatching isn't failing, so the baseline PDF is left untouched. I suspect that this is because alpha on hatches on PDF backend isn't completely implemented and it looks like #17049 aims to fix this.

timhoffm

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.

lib/matplotlib/tests/baseline_images/test_axes/contour_hatching.png

doc/users/next_whats_new/separated_hatchcolor.rst

r3kste · 2025-01-26T07:41:34Z

AFAICS you haven't modified the test. This would mean, whe have a change of default behavior. Can that be avoided?

I think that we can't avoid this change, because the default behavior is somewhat inconsistent, as explained in this comment.

(2) I suggest to modifying the test instead (likely by setting an explicit hatch color) so that the tests images do not need update. Generally, we try to use as few images as possible and changes them as a rarely as possible, because the images bloat the git repo.

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 _hatchcolors attribute.

diff --git a/lib/matplotlib/tests/test_axes.py b/lib/matplotlib/tests/test_axes.py
index 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:
For a stackplot, with alpha = 0.3

	Previous Behavior	New Behavior (PR)
`edgecolor = 'black'`
`edgecolor = None`

timhoffm · 2025-02-13T16:23:04Z

Using a GraphicsContext sounds reasonable. But that's a change I would also combine with introducing more formal API versioning.

story645 · 2025-02-13T19:23:27Z

Do these changes need to be applied back to the changes in #28104 and should factoring out into a global context object + versioning be their own PR(s)?

timhoffm · 2025-02-13T23:03:51Z

No, #28104 is not affected, because it's not changing the backend API.

anntzer · 2025-02-14T15:16:46Z

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 · 2025-02-17T10:39:40Z

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 · 2025-02-22T16:49:03Z

This works now on mplcairo. Thanks for going through all the work!
I haven't carefully reviewed the implementation yet but the general approach looks fine to me. Perhaps add a note that the current new API is provisional, as I expect that it will change in the future at least to support multiple hatches as well? (I would rather only change things on mplcairo's side once multiple hatches are supported too.)

r3kste · 2025-03-15T03:48:39Z

@anntzer Are there any other changes expected? Or is this ready to be merged?

lib/matplotlib/backend_bases.py

anntzer

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.

…gument

r3kste · 2025-03-21T07:35:11Z

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.

I agree that this API will need to be changed in the future.

@timhoffm any suggestions?

dstansby

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.

doc/users/next_whats_new/separated_hatchcolor.rst

galleries/examples/shapes_and_collections/hatchcolor_demo.py

lib/matplotlib/backend_bases.py

lib/matplotlib/tests/test_backend_bases.py

dstansby · 2025-03-21T18:56:24Z

lib/matplotlib/collections.py

+                    "screen", hatchcolors=self.get_hatchcolor()
+                )
+            except TypeError:
+                hatchcolors_arg_supported = False


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.

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.

You can make a mock third party backend to call the function - there are some examples of other mocks in the tests

lib/matplotlib/collections.py

lib/matplotlib/backend_bases.pyi

lib/matplotlib/tests/test_axes.py

doc/users/next_whats_new/separated_hatchcolor.rst

galleries/examples/shapes_and_collections/hatchcolor_demo.py

timhoffm · 2025-03-24T15:44:17Z

lib/matplotlib/collections.py

+            # 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


Suggested change

# 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.

timhoffm · 2025-03-24T15:45:03Z

lib/matplotlib/collections.py

-                    "screen")
+                    "screen", hatchcolors=self.get_hatchcolor()
+                )
+            except TypeError:


It's worth explicitly documenting this in the comment, see suggestion above.

timhoffm · 2025-03-24T16:01:23Z

lib/matplotlib/backend_bases.py

@@ -252,7 +258,7 @@ def draw_path_collection(self, gc, master_transform, paths, all_transforms,

    def draw_quad_mesh(self, gc, master_transform, meshWidth, meshHeight,
                       coordinates, offsets, offsetTrans, facecolors,
-                       antialiased, edgecolors):
+                       antialiased, edgecolors, hatchcolors=None):


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 a hatchcolors 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 of draw_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 @dstansby @r3kste Does that make sense?

anntzer · 2025-03-24T17:37:44Z

re: making hatchcolors optional and API compatibility:
My point of view is that the renderer API (draw_foo) is public and it is legitimate for 3rd-parties to call it directly (e.g. they can create their own artist subclasses that implement their own draw() and call draw_foo() from there. However, I guess it could also be reasonable(?), and possibly helpful for extensibility, to declare that these methods are not for 3rd-party direct use, and that third-party artists must go through a builtin artist's draw to invoke the renderer's drawing functions. But that's definitely not a policy we have stated so far.

timhoffm · 2025-03-24T18:26:14Z

Thanks. I agree optional is set to begin with for compatibility.

Questions are:

Do we want/gain anything with keyword-only? - I tend towards no.
Do we want to evolve the API to non-optional through regular deprecation machinery? - I tend towards yes.

anntzer · 2025-03-24T19:32:06Z

Do we want/gain anything with keyword-only? - I tend towards no.

It makes it easier to later include parameters before it, e.g. hatches is something that's clearly missing and would be expected to come first.

Do we want to evolve the API to non-optional through regular deprecation machinery? - I tend towards yes.

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.

…tchcolors arg

timhoffm · 2025-03-28T23:42:48Z

lib/matplotlib/collections.py

+                    path_ids = renderer._iter_collection_raw_paths(
+                        transform.frozen(), ipaths, self.get_transforms())
+                    for xo, yo, path_id, gc0, rgbFace in renderer._iter_collection(
+                        gc, list(path_ids), *args, hatchcolors=self.get_hatchcolor(),
+                    ):
+                        path, transform = path_id
+                        if xo != 0 or yo != 0:
+                            transform = transform.frozen()
+                            transform.translate(xo, yo)
+                        renderer.draw_path(gc0, path, transform, rgbFace)


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 passed else: # unroll the collection to draw paths in a loop

I'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

Good catch. I agree that only going through the unrolled version when hatchcolors are actually passed would likely be much better performance-wise.

timhoffm · 2025-03-31T10:01:07Z

Thanks @r3kste for going through all the nitty gritty details with us!

github-actions bot added topic: rcparams backend: agg backend: ps backend: pdf backend: svg topic: legend topic: collections and mappables topic: styles topic: canvas and figure manager labels Oct 30, 2024

story645 added the status: waiting for other PR label Oct 31, 2024

github-actions bot added the status: needs rebase label Oct 31, 2024

Impaler343 mentioned this pull request Dec 19, 2024

Separates edgecolor from hatchcolor #28104

Merged

6 tasks

Impaler343 marked this pull request as ready for review January 3, 2025 11:58

r3kste force-pushed the collections-hatchcolor branch from ab4081d to 256d735 Compare January 3, 2025 12:17

github-actions bot removed topic: rcparams topic: styles status: needs rebase labels Jan 3, 2025

github-actions bot added the status: needs rebase label Jan 4, 2025

timhoffm removed the status: waiting for other PR label Jan 5, 2025

r3kste force-pushed the collections-hatchcolor branch from c8d7b85 to b081bb9 Compare January 8, 2025 15:52

github-actions bot added Documentation: examples files in galleries/examples and removed status: needs rebase labels Jan 8, 2025

timhoffm reviewed Jan 25, 2025

View reviewed changes

lib/matplotlib/tests/baseline_images/test_axes/contour_hatching.png Outdated Show resolved Hide resolved

timhoffm reviewed Jan 25, 2025

View reviewed changes

doc/users/next_whats_new/separated_hatchcolor.rst Show resolved Hide resolved

pass hatchcolor arg only if it is supported by the renderer. and made…

7e46707

… suggested changes.

Fix hatchcolors argument support in third-party backends

c5e9583

Add note about provisional API for draw_path_collection()

d2b6268

anntzer reviewed Mar 16, 2025

View reviewed changes

lib/matplotlib/backend_bases.py Outdated Show resolved Hide resolved

anntzer approved these changes Mar 16, 2025

View reviewed changes

Refactor draw_path_collection to make hatchcolors a keyword-only ar…

fee895b

…gument

dstansby reviewed Mar 21, 2025

View reviewed changes

Made suggested changes

b9ac0fb

timhoffm reviewed Mar 24, 2025

View reviewed changes

Impaler343 force-pushed the collections-hatchcolor branch from 9800478 to 0c557e4 Compare March 28, 2025 15:54

added test and improve comments for third party backend support of ha…

2e4784b

…tchcolors arg

r3kste force-pushed the collections-hatchcolor branch from 0c557e4 to 2e4784b Compare March 28, 2025 15:55

timhoffm reviewed Mar 28, 2025

View reviewed changes

skip unnecessary path iteration when hatchcolors is not passed/needed

3b43bc6

timhoffm approved these changes Mar 31, 2025

View reviewed changes

timhoffm added this to the v3.11.0 milestone Mar 31, 2025

timhoffm merged commit 65d2818 into matplotlib:main Mar 31, 2025
39 of 42 checks passed

anntzer mentioned this pull request Apr 7, 2025

[Bug]: Missing backcompat for backends not supporting hatchcolors in draw_quad_mesh #29883

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add hatchcolor parameter for Collections #29044

Add hatchcolor parameter for Collections #29044

Impaler343 commented Oct 30, 2024 •

edited

Loading

story645 commented Oct 30, 2024

Impaler343 commented Oct 31, 2024

r3kste commented Oct 31, 2024

Impaler343 commented Jan 3, 2025

r3kste commented Jan 3, 2025

timhoffm left a comment •

edited

Loading

r3kste commented Jan 26, 2025

timhoffm commented Feb 13, 2025

story645 commented Feb 13, 2025

timhoffm commented Feb 13, 2025

anntzer commented Feb 14, 2025

anntzer commented Feb 17, 2025

anntzer commented Feb 22, 2025

r3kste commented Mar 15, 2025

anntzer left a comment

r3kste commented Mar 21, 2025

dstansby left a comment

dstansby Mar 21, 2025

r3kste Mar 22, 2025

story645 Mar 23, 2025

timhoffm Mar 24, 2025

timhoffm Mar 24, 2025

timhoffm Mar 24, 2025

anntzer commented Mar 24, 2025

timhoffm commented Mar 24, 2025

anntzer commented Mar 24, 2025

timhoffm Mar 28, 2025

anntzer Mar 29, 2025 •

edited

Loading

timhoffm commented Mar 31, 2025

-            # 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.

Add hatchcolor parameter for Collections #29044

Add hatchcolor parameter for Collections #29044

Conversation

Impaler343 commented Oct 30, 2024 • edited Loading

PR summary

PR checklist

story645 commented Oct 30, 2024

Impaler343 commented Oct 31, 2024

r3kste commented Oct 31, 2024

Previous Behavior

New Behavior

Reason for New Behavior

Impaler343 commented Jan 3, 2025

r3kste commented Jan 3, 2025

timhoffm left a comment • edited Loading

Choose a reason for hiding this comment

r3kste commented Jan 26, 2025

timhoffm commented Feb 13, 2025

story645 commented Feb 13, 2025

timhoffm commented Feb 13, 2025

anntzer commented Feb 14, 2025

anntzer commented Feb 17, 2025

anntzer commented Feb 22, 2025

r3kste commented Mar 15, 2025

anntzer left a comment

Choose a reason for hiding this comment

r3kste commented Mar 21, 2025

dstansby left a comment

Choose a reason for hiding this comment

dstansby Mar 21, 2025

Choose a reason for hiding this comment

r3kste Mar 22, 2025

Choose a reason for hiding this comment

story645 Mar 23, 2025

Choose a reason for hiding this comment

timhoffm Mar 24, 2025

Choose a reason for hiding this comment

timhoffm Mar 24, 2025

Choose a reason for hiding this comment

timhoffm Mar 24, 2025

Choose a reason for hiding this comment

anntzer commented Mar 24, 2025

timhoffm commented Mar 24, 2025

anntzer commented Mar 24, 2025

timhoffm Mar 28, 2025

Choose a reason for hiding this comment

anntzer Mar 29, 2025 • edited Loading

Choose a reason for hiding this comment

timhoffm commented Mar 31, 2025

Impaler343 commented Oct 30, 2024 •

edited

Loading

timhoffm left a comment •

edited

Loading

anntzer Mar 29, 2025 •

edited

Loading