Fix dash offset bug in Patch by AnnaMastori · Pull Request #23412 · matplotlib/matplotlib

AnnaMastori · 2022-07-11T01:41:11Z

PR Summary

Pass the given offest in the draw method of the Patch class. In this way, the Patch class no longer ignores the dash offset. (closes #22977 )

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

jklymak · 2022-07-11T06:28:37Z

This sounds like a breaking change. Do you have a test or a what's new image to explain why this is the right thing to do?

anntzer · 2022-07-11T06:56:10Z

The whole setattr_cm is now a no-op and can go away.
See discussion in particular at #22977 (comment). I agree this qualifies more as a bugfix than as an API changes (it would only affect users who 1) explicitly set a dashoffset to a patch and 2) expected it to be ignored), but at least a whatsnew entry would be nice.
The current behavior came in in https://github.com/matplotlib/matplotlib/pull/6710/files#diff-03b74bfefbb7d4eca2804f274825899d191f8bacdbe99ea973d88925f15d1169R525 (@tacaswell).
Also, when I last looked at this (#11741), I noted that ignoring the dashoffsets was necessary to make tests pass; it would be nice to figure out why this is no longer the case.

AnnaMastori · 2022-07-11T22:57:00Z

The whole setattr_cm is now a no-op and can go away. See discussion in particular at #22977 (comment). I agree this qualifies more as a bugfix than as an API changes (it would only affect users who 1) explicitly set a dashoffset to a patch and 2) expected it to be ignored), but at least a whatsnew entry would be nice. The current behavior came in in https://github.com/matplotlib/matplotlib/pull/6710/files#diff-03b74bfefbb7d4eca2804f274825899d191f8bacdbe99ea973d88925f15d1169R525 (@tacaswell). Also, when I last looked at this (#11741), I noted that ignoring the dashoffsets was necessary to make tests pass; it would be nice to figure out why this is no longer the case.

I am not very familiar with context managers but I guess that the removal of the setattr_cm should not be addressed within this PR. If that is not the case, I can remove it.

oscargus · 2022-07-12T08:10:45Z

This sounds like a breaking change

One may indeed think so, but please see the argument from @timhoffm why it is maybe/probably not: #22977 (comment)

oscargus · 2022-07-12T08:17:25Z

I think a test is required here, as suggested in #22977 (comment). One way is to make an image comparison test, see e.g.

matplotlib/lib/matplotlib/tests/test_patches.py

Lines 131 to 132 in fe870f5

    
           @check_figures_equal(extensions=['png']) 
        
           def test_rotate_rect_draw(fig_test, fig_ref):

where, roughly, relevant parts of the code in #22977 (comment) is used for the reference image and then do the similar thing but with the new feature, (i.e., replacing the last linestyle with linestyle = (6, [6, 6]),, (probably the first can be linestyle = (0, [6, 6]), for both cases).

AnnaMastori · 2022-07-14T13:43:10Z

Any feedback on the test ? @oscargus

timhoffm · 2022-07-17T23:45:21Z

+    edgecolor = 'b'
+    linestyle = (0, [6, 6])
+    linestyle_hacked = (0, [0, 6, 6, 0])
+    rect_ref = Rectangle(loc, width, height, edgecolor=edgecolor,
+                                                linestyle=linestyle)
+    rect_ref2 = Rectangle(loc, width, height, edgecolor=edgecolor,
+                                        linestyle=linestyle_hacked)


I first thought: Why do we need two reference rectangles at all? We have two test rectangles that are carefully crafted to lie on top of each other, but have shifted dashes. Can't we have one single solid rectangle as reference?

Apparently the two dashed lines don't add up to a fully solid line because of antialiasing.

If we can't get a solid rectangle anyway, let's not even try: I propose to leave the rectangles on top of each other, but use different colors.

Suggested change

edgecolor = 'b'

linestyle = (0, [6, 6])

linestyle_hacked = (0, [0, 6, 6, 0])

rect_ref = Rectangle(loc, width, height, edgecolor=edgecolor,

linestyle=linestyle)

rect_ref2 = Rectangle(loc, width, height, edgecolor=edgecolor,

linestyle=linestyle_hacked)

rect_ref = Rectangle(loc, width, height, linewdith=3,

edgecolor='b','linestyle=(0, [6, 6]))

# fill the line gaps using a linestyle (0, [0, 6, 6, 0]), which is

# equivalent to (6, [6, 6]) but has 0 dash offset

rect_ref2 = Rectangle(loc, width, height, linewidth=3,

edgecolor='r', linestyle=(0, [0, 6, 6, 0]))

Note that I have inlined the edgecolor and linestyle variables. They are only used once and IMHO defining them explicitly leads to another indirection and additional lines, which don't help readability. I've also increased the linewidth to 3, which makes the dash pattern better visible.

Thanks for the comments on code readability. It's really useful.

tacaswell · 2022-08-03T21:29:57Z

@AnnaMastori Are you willing to follow up with Tim's comments?

AnnaMastori · 2022-08-04T22:33:55Z

@AnnaMastori Are you willing to follow up with Tim's comments?

Yes, I will get to it ASAP.

timhoffm · 2022-08-05T20:17:49Z

Thanks @AnnaMastori!

* Add passed offset in draw method of Patch * Add entry to whats new about dash offset * Remove trailing spaces * Add test for dash offset in Patch * Refactor test_dash_offset_patch_draw * Correct mention of code object * Fix indentation in draw method of Patch

…bjects Pin f06c2c3abdaf4b90285ce5ca7fedbb8ace715911 and its regression selectors for reproducible runs. Source: matplotlib/matplotlib#23412

Add passed offset in draw method of Patch

43bff96

AnnaMastori changed the title ~~Add passed offset in draw method of Patch~~ Fix dash offset bug in Patch Jul 11, 2022

AnnaMastori added 2 commits July 11, 2022 22:12

Add entry to whats new about dash offset

ac4ba14

Remove trailing spaces

18bf3b1

oscargus added the status: needs tests label Jul 12, 2022

Add test for dash offset in Patch

b3cdcdf

oscargus approved these changes Jul 15, 2022

View reviewed changes

oscargus removed the status: needs tests label Jul 15, 2022

timhoffm reviewed Jul 17, 2022

View reviewed changes

QuLogic reviewed Jul 21, 2022

View reviewed changes

Comment thread lib/matplotlib/patches.py Resolved

tacaswell added this to the v3.6.0 milestone Aug 3, 2022

AnnaMastori added 3 commits August 5, 2022 18:16

Refactor test_dash_offset_patch_draw

d810794

Correct mention of code object

b9edc6f

Fix indentation in draw method of Patch

4ebe03a

timhoffm approved these changes Aug 5, 2022

View reviewed changes

timhoffm merged commit d7ffff2 into matplotlib:main Aug 5, 2022

Conversation

AnnaMastori commented Jul 11, 2022

PR Summary

PR Checklist

Uh oh!

jklymak commented Jul 11, 2022

Uh oh!

anntzer commented Jul 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnnaMastori commented Jul 11, 2022

Uh oh!

oscargus commented Jul 12, 2022

Uh oh!

oscargus commented Jul 12, 2022

Uh oh!

AnnaMastori commented Jul 14, 2022

Uh oh!

Uh oh!

timhoffm Jul 17, 2022

Choose a reason for hiding this comment

Uh oh!

AnnaMastori Aug 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tacaswell commented Aug 3, 2022

Uh oh!

AnnaMastori commented Aug 4, 2022

Uh oh!

timhoffm commented Aug 5, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

anntzer commented Jul 11, 2022 •

edited

Loading