Fix dash offset bug in Patch #23412
Fix dash offset bug in Patch #23412
Conversation
|
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? |
|
The whole setattr_cm is now a no-op and can go away. |
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. |
One may indeed think so, but please see the argument from @timhoffm why it is maybe/probably not: #22977 (comment) |
|
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 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).
|
|
Any feedback on the test ? @oscargus |
lib/matplotlib/tests/test_patches.py
Outdated
| 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) |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
Thanks for the comments on code readability. It's really useful.
|
@AnnaMastori Are you willing to follow up with Tim's comments? |
Yes, I will get to it ASAP. |
|
Thanks @AnnaMastori! |
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
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).