Separates edgecolor from hatchcolor #28104

Impaler343 · 2024-04-19T12:48:34Z

PR summary

Taking after @Vashesh08 's PR #26993
Should close #26074 and #7059
Implemented the fallback logic that @story645 mentioned in this comment
Cherry picked some initial commits from the Issue26074 branch written by Vashesh
Modified the hatchcolor setting to way similar to edgecolor setting.

Need to do some extensive testing on patches and collections
Need to write documentation for hatchcolor attribute, and also describe the behaviour change

PR checklist

"closes #0000" is in the body of the PR description to link the related issue
new and changed code is tested
[N/A] 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

r3kste · 2024-08-11T05:42:48Z

There are two tests that are currently failing and need a rewrite.

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

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

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

test_backend_bases.py::test_uses_per_path This requires a small change in the parameters passed to _iter_collection, corresponding to hatchcolors.

--- a/lib/matplotlib/tests/test_backend_bases.py
+++ b/lib/matplotlib/tests/test_backend_bases.py
@@ -35,7 +35,7 @@ def test_uses_per_path():
            rb._iter_collection(
                gc, range(len(raw_paths)), offsets,
                transforms.AffineDeltaTransform(master_transform),
-                   facecolors, edgecolors, [], [], [False],
+                   facecolors, edgecolors, [], [], [], [False],
                [], 'screen')]
        uses = rb._iter_collection_uses_per_path(
            paths, all_transforms, offsets, facecolors, edgecolors)

Impaler343 · 2024-08-11T05:44:06Z

Pinging @story645 for review

story645 · 2024-08-15T03:48:36Z

Sorry for delay, will try to review but please add some tests.

story645 · 2024-08-15T03:50:38Z

Also this should probably go in before #27937 b/c I agree that it'll probably clean that one up.

story645

I think this might need some discussion on how to manage since the changes dive pretty deep into the internals, but definitely add some tests cause that could help you debug the segfault.

lib/matplotlib/backend_bases.py

lib/matplotlib/collections.py

lib/matplotlib/backend_bases.py

lib/matplotlib/collections.py

lib/matplotlib/patches.py

src/_backend_agg.h

tacaswell · 2024-08-22T19:53:23Z

For this change we need to make sure both:

Anyone calling the backend methods without the new input value needs to not be broken (so the new keyword needs to be last and optional)
Anyplace we call the backend methods internally we need to be forgiving of backends that have not adopted the new signature (we should have an example of using inspect.signature to manage this)

This will also need a lot of documentation (both notifying third-party backends the expected API is changing) and announcing the new feature to users (with lots of examples).

lib/matplotlib/backend_bases.py

r3kste · 2024-08-30T16:02:37Z

Could I also go ahead with rewriting the two tests that are failing?

story645 · 2024-08-30T17:04:15Z

Could I also go ahead with rewriting the two tests that are failing?

~~why are they failing?~~ never mind, went and looked. So for this test:

TypeError: RendererBase._iter_collection() missing 1 required positional argument: 'offset_position'

It shouldn't be failing. This is what @tacaswell was talking about that you want to make sure the code is backwards compatible.

The test contour_hatch also seems like it should still pass, is there a reason it shouldn't?

Impaler343 · 2024-08-30T17:07:46Z

Could I also go ahead with rewriting the two tests that are failing?

why are they failing?

The initial comment explains the reason

story645 · 2024-08-30T17:14:27Z

Sorry, I updated while you posted. For the tests:

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.
In the above test, alpha is set to 0.5 and edgecolors is not specified. Ideally, the hatch should have an alpha value of 0.5, but it actually has the default alpha value of 1.0.

I think the user specified alpha should also be propagated to hatchcolors, w/ the same priority rules as color and edgecolor, which should then make the test pass.

This requires a small change in the parameters passed to _iter_collection, corresponding to hatchcolors.
these should be optional for backwards compatibility and therefore still pass

tacaswell · 2024-08-30T19:01:13Z

It may be simpler to work out what the non-vectorized version of this API looks like (so do just the classes in pathes.py) first and then do the collection-based ones in a follow on PR.

story645

couple of small comments but I'm really impressed w/ the work you've done here and how far this PR has come. 😄

doc/users/next_whats_new/separated_hatchcolor.rst

story645 · 2024-12-19T06:45:52Z

lib/matplotlib/patches.py

+            if self._edgecolor[3] == 0:  # fully transparent
+                return colors.to_rgba(mpl.rcParams['patch.edgecolor'])
+            return self.get_edgecolor()


so just checking that this will get followed up and just used get_edgecolor? cause like I mentioned, I'm uncomfortable with using transparent edge as the test

discussed this on the call today and is this being done to maintain backward compatibility if someone sets edgecolor to None and doesn't define hatchcolor or hatch.color to ensure that something gets drawn?

my thinking is that that should return a warning "Setting a hatch but hatch and edgecolor are none" but we shouldn't special case a fallback (that also may not work if patch.edgecolor is "none")

is this being done to maintain backward compatibility if someone sets edgecolor to None and doesn't define hatchcolor or hatch.color to ensure that something gets drawn?

Yes. This is for the final fallback where edgecolor is set to 'none', and hatchcolor is not set in which case patch.edgecolor rcParam is used and this condition is there to check whether edgecolor is None. Removing this condition causes test_artist::test_hatching to fail.

Ok, so we should discuss if this behavior should be deprecated (b/c it's an iffy fallback to rely on facecolor.rcparam being set to not None) now that we allow an explicit hatchcolor, but I think it's okay to push that to a follow up to this PR.

Is part of the problem here eager resolution of edgeolor so you can't check that edgecolor is None (hasn't been set)?

Yes, as hatchcolor must depend on the current edgecolor now, we are essentially special casing for when the hatchcolor came through as black from the rcParam in the previous implementation. I believe we can't really check if the edgecolor is None in a better place
I am for hatching with the rcParam when no hatchcolor is specified as it not only maintains backward compatibility but seems more natural for a user to not care about the hatchcolor, but the next best thing would be to raise a warning as mentioned above

I'm gonna move the discussion of deprecating to a new issue, but my concern is that the fallback on edgecolor='None' means:

if edgecolor='None' you'll backfill with 'patch.edgecolor' but that may also be None and this all happens silently

edgecolor=('green', 0) will also trigger this path and the person may have wanted the 'green' and were gonna up the alpha

lib/matplotlib/tests/test_patches.py

doc/users/next_whats_new/separated_hatchcolor.rst

lib/matplotlib/tests/test_patches.py

doc/users/next_whats_new/separated_hatchcolor.rst

lib/matplotlib/tests/test_patches.py

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

story645

slight wording nits but otherwise I think this is good to go and a feature I wanted a few weeks back

doc/users/next_whats_new/separated_hatchcolor.rst

galleries/examples/shapes_and_collections/hatchcolor_demo.py

story645 · 2024-12-24T23:40:03Z

lib/matplotlib/patches.py

+            if self._edgecolor[3] == 0:  # fully transparent
+                return colors.to_rgba(mpl.rcParams['patch.edgecolor'])
+            return self.get_edgecolor()


I'm gonna move the discussion of deprecating to a new issue, but my concern is that the fallback on edgecolor='None' means:

if edgecolor='None' you'll backfill with 'patch.edgecolor' but that may also be None and this all happens silently

edgecolor=('green', 0) will also trigger this path and the person may have wanted the 'green' and were gonna up the alpha

lib/matplotlib/patches.pyi

timhoffm · 2024-12-29T10:45:58Z

Thanks @Impaler343 for going with us through the lengthy discussion and design process.

github-actions bot added topic: rcparams topic: canvas and figure manager topic: collections and mappables labels Apr 19, 2024

Impaler343 marked this pull request as draft April 24, 2024 03:29

github-actions bot added the backend: agg label Jul 8, 2024

github-actions bot added backend: ps backend: pdf backend: svg topic: legend topic: styles labels Aug 10, 2024

Impaler343 force-pushed the hatchcolor branch from bcbf986 to 438c319 Compare August 10, 2024 15:54

Impaler343 marked this pull request as ready for review August 11, 2024 05:45

story645 self-assigned this Aug 14, 2024

r3kste mentioned this pull request Aug 21, 2024

Fixed hatching in PatchCollection class #27937

Open

4 tasks

story645 reviewed Aug 21, 2024

View reviewed changes

github-actions bot added the status: needs rebase label Aug 24, 2024

Impaler343 force-pushed the hatchcolor branch from 827e6b1 to 31dbe12 Compare August 25, 2024 04:13

github-actions bot removed the status: needs rebase label Aug 25, 2024

r3kste force-pushed the hatchcolor branch from 31dbe12 to 11e3fce Compare August 25, 2024 06:29

story645 reviewed Aug 25, 2024

View reviewed changes

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

story645 reviewed Dec 19, 2024

View reviewed changes

QuLogic reviewed Dec 19, 2024

View reviewed changes

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

lib/matplotlib/tests/test_patches.py Outdated Show resolved Hide resolved

lib/matplotlib/tests/test_patches.py Outdated Show resolved Hide resolved

story645 reviewed Dec 19, 2024

View reviewed changes

r3kste and others added 17 commits December 22, 2024 12:42

separated hatchcolor from edgecolor in patches

7b489af

fixed logic for inherit in collections

ed55f09

added a gallery example

3155bc0

made suggested changes

905ed86

reverted use of inherit

7942afd

fixed the logic for inheriting from edgecolor

219bb26

fixed docs and enhanced tests

8e732fa

Apply suggestions from code review

67a54c3

Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

made suggested changes

84e12fb

minor fix

5b3a1e6

use 'edge' instead of 'inherit'

8454dfc

fix rcsetup

a3c80e0

enhanced docs and added note for follow-up PR

253c7e4

Made suggested changes

0754734

made suggested changes

a74f2af

enhanced tests and minor fix in docs

91ae826

minor changes to docs

a23c9c5

Impaler343 force-pushed the hatchcolor branch from 72d47a7 to a23c9c5 Compare December 22, 2024 07:12

story645 approved these changes Dec 24, 2024

View reviewed changes

story645 mentioned this pull request Dec 25, 2024

[MNT]: deprecate auto hatch fallback to patch.edgecolor when edgecolor='None' #29380

Open

Changed wording and alt text

4dd0c2d

timhoffm reviewed Dec 25, 2024

View reviewed changes

lib/matplotlib/patches.pyi Outdated Show resolved Hide resolved

Minor Fix

6b54933

timhoffm approved these changes Dec 29, 2024

View reviewed changes

timhoffm merged commit c11175d into matplotlib:main Dec 29, 2024
37 of 39 checks passed

larsoner mentioned this pull request Jan 7, 2025

BUG: Fix regression with set_hatchcolor #29427

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Separates edgecolor from hatchcolor #28104

Separates edgecolor from hatchcolor #28104

Impaler343 commented Apr 19, 2024 •

edited

Loading

r3kste commented Aug 11, 2024

Previous Behavior

New Behavior

Reason for New Behavior

Impaler343 commented Aug 11, 2024

story645 commented Aug 15, 2024

story645 commented Aug 15, 2024

story645 left a comment

tacaswell commented Aug 22, 2024

r3kste commented Aug 30, 2024

story645 commented Aug 30, 2024 •

edited

Loading

Impaler343 commented Aug 30, 2024

story645 commented Aug 30, 2024

tacaswell commented Aug 30, 2024

story645 left a comment

story645 Dec 19, 2024

story645 Dec 19, 2024 •

edited

Loading

r3kste Dec 20, 2024

story645 Dec 22, 2024 •

edited

Loading

Impaler343 Dec 23, 2024

story645 Dec 24, 2024

story645 left a comment

story645 Dec 24, 2024

timhoffm commented Dec 29, 2024

Separates edgecolor from hatchcolor #28104

Separates edgecolor from hatchcolor #28104

Conversation

Impaler343 commented Apr 19, 2024 • edited Loading

PR summary

PR checklist

r3kste commented Aug 11, 2024

Previous Behavior

New Behavior

Reason for New Behavior

Impaler343 commented Aug 11, 2024

story645 commented Aug 15, 2024

story645 commented Aug 15, 2024

story645 left a comment

Choose a reason for hiding this comment

tacaswell commented Aug 22, 2024

r3kste commented Aug 30, 2024

story645 commented Aug 30, 2024 • edited Loading

Impaler343 commented Aug 30, 2024

story645 commented Aug 30, 2024

tacaswell commented Aug 30, 2024

story645 left a comment

Choose a reason for hiding this comment

story645 Dec 19, 2024

Choose a reason for hiding this comment

story645 Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

r3kste Dec 20, 2024

Choose a reason for hiding this comment

story645 Dec 22, 2024 • edited Loading

Choose a reason for hiding this comment

Impaler343 Dec 23, 2024

Choose a reason for hiding this comment

story645 Dec 24, 2024

Choose a reason for hiding this comment

story645 left a comment

Choose a reason for hiding this comment

story645 Dec 24, 2024

Choose a reason for hiding this comment

timhoffm commented Dec 29, 2024

Impaler343 commented Apr 19, 2024 •

edited

Loading

story645 commented Aug 30, 2024 •

edited

Loading

story645 Dec 19, 2024 •

edited

Loading

story645 Dec 22, 2024 •

edited

Loading