Factor out common code between Patch.draw and FancyArrowPatch.draw. #11741

anntzer · 2018-07-22T23:04:28Z

The code setting up the renderer and gc are nearly identical, so we can
factor it out as a helper contextmanager. The only difference is that
FancyArrowPatch then calls draw_path multiple times with various
arguments.

Note that the refactoring exposed the fact that FancyArrowPatches are
(incorrectly, IMO) ignoring joinstyles and capstyles (this is necessary
to make image test pass, but we probably will want to generate new
images with the "correct" (non-ignoring) behavior at some point).

PR Summary

PR Checklist

Has Pytest style unit tests
Code is PEP 8 compliant
New features are documented, with examples if plot related
Documentation is sphinx and numpydoc compliant
Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

jklymak · 2018-07-24T03:02:10Z

I don't follow context managers well enough to follow this code. Do you need to use a context manager to do this or can it just be factored out as a method or helper function? I don't understand why this would conceptually be a "context".

anntzer · 2018-07-24T07:56:30Z

Essentially, the context manager allows to refactor

<do init>
<do 1 using some vars set in init>
<do cleanup using some vars set in init>

<do init>
<do 2 using some vars set in init>
<do cleanup using some vars set in init>

into

@contextlib.contextmanager
def cm():
    <do init>
    yield <the vars>
    <do cleanup using some vars set in init>

with cm() as <the vars we need>:
    <do 1 using some vars set in init>

with cm() as <the vars we need>:
    <do 2 using some vars set in init>

(This can essentially be read as "run the context manager code, inserting the indented contents of the with-block at the place of the yield".)

I could instead provide a _setup() helper and a _cleanup() helper, but then I'd need to also pass the renderer and gc to the cleanup; or I could just inline and duplicate the (relatively short) cleanup code -- not a big deal, but the cm construct is exactly there for this purpose.

jklymak · 2018-07-24T13:08:11Z

No thats fine. I think I understand now. If you want to make it more clear for those of us w/ a more rudimentary understanding of the language feature set, you could comment the init and cleanup sections. You could also comment the partial, which I had to look up. However, feel free to use your judgement as to whether I am just hopelessly ignorant about the language feature set and everyone else is very familiar with functools and contextlib. In general, I think being explicit in shared code is helpful...

anntzer · 2018-07-24T13:25:55Z

Added a comment at the level of the yield, I hope it helps. Feel free to propose a rewording.

timhoffm · 2018-07-27T22:24:26Z

lib/matplotlib/patches.py

-        if self._edgecolor[3] == 0:
-            lw = 0
-        gc.set_linewidth(lw)
-        gc.set_dashes(self._dashoffset, self._dashes)


The context uses gc.set_dashes(0, self._dashes) instead.

Indeed, I had missed that. I restored the old behavior using setattr_cm (in Patch.draw), leaving a comment similar to the one for the capstyle in FancyArrowPatch.

timhoffm · 2018-07-27T22:31:46Z

lib/matplotlib/patches.py

-
-        gc.set_antialiased(self._antialiased)
-        self._set_gc_clip(gc)
-        gc.set_capstyle('round')


The context uses gc.set_capstyle(self._capstyle) instead. This is 'butt' by default (inherited from Patch). It's reasonable to draw the value from self._capstyle. Should the default for this value be 'round' to have backward compatibility or do you want to change it intentionally to be the same default as for Patch?

I forced it to "round" above (in the setattr_cm). Yes, ignoring the capstyle was probably a long-standing bug (the kind of things that such refactors uncover...) but changing that would result in changes in the baseline images (... that's how I realized the discrepancy). I'll leave this as it is until someone actually complains or we have some othe reason to change the baseline images.

timhoffm · 2018-07-27T22:37:54Z

lib/matplotlib/patches.py

-        renderer.close_group('patch')
-        self.stale = False
+            for p, f in zip(path, fillable):
+                draw_path(p, affine, self._facecolor if f else None)


doesn't need self._facecolor the same workaround for fully transparent values as above (self._facecolor if self._facecolor[3] else None).

Also in the original function rgbFace was used, which changed alpha=0 to alpha=None

rgbFace = self._facecolor if rgbFace[3] == 0: rgbFace = None # (some?) renderers expect this as no-fill signal

Fixed to

for p, f in zip(path, fillable): draw_path( p, affine, self._facecolor if f and self._facecolor[3] else None)

which should handle both issues.

timhoffm · 2018-07-27T22:54:00Z

lib/matplotlib/patches.py

+        """
+        ``draw()`` helper factored out for sharing with `FancyArrowPatch`.
+
+        Yields a callable that can be called as a bound ``draw_path`` method


Would this be more clear?

Yields a draw_path-like function that is bound to the GraphicsContext of the renderer. Technically, this is a partial function of draw_path with the first argument (the GraphicsContext) already filled in. The arguments of the returned function are the remaining arguements of draw_path.

reworded the docstring to something else...

timhoffm · 2018-07-27T22:58:44Z

lib/matplotlib/patches.py

-        if not self.get_visible():
-            return
+    @contextlib.contextmanager
+    def _prepare_path_drawer(self, renderer):


An alternative name could be _bind_draw_path_function. This would result in

with self._bind_draw_path_function(renderer) as draw_path: ...

A bit longish but maybe also a bit more telling. I don't have strong feelings on this. Leave it or use it.

anntzer · 2018-07-28T17:55:31Z

Comments addressed as a separate commit for ease of review, but would suggest squashing when merging.

timhoffm · 2018-07-28T22:37:09Z

Good to go. I'm not sure about the milestone. Is this still for 3.0 or should this go in 3.1?

Anyone can merge (and squash), once this is decided.

anntzer · 2018-08-19T19:40:26Z

Squashed and rebased, will self-merge into master (not 3.0) once CI passes per comment above.

The code setting up the renderer and gc are nearly identical, so we can factor it out as a helper contextmanager. The only difference is that FancyArrowPatch then calls draw_path multiple times with various arguments. Note that the refactoring exposed the fact that FancyArrowPatches are (incorrectly, IMO) ignoring joinstyles and capstyles (this is necessary to make image test pass, but we probably will want to generate new images with the "correct" (non-ignoring) behavior at some point).

anntzer · 2018-08-21T07:50:19Z

CI failure is spurious (apt-get complaining), self-merging per above.

jklymak approved these changes Jul 24, 2018

View reviewed changes

anntzer force-pushed the patchdraw branch from 054971d to f574cf2 Compare July 24, 2018 13:25

timhoffm reviewed Jul 27, 2018

View reviewed changes

anntzer force-pushed the patchdraw branch from cd979de to a15e366 Compare July 28, 2018 18:22

timhoffm approved these changes Jul 28, 2018

View reviewed changes

anntzer force-pushed the patchdraw branch from a15e366 to 311e95e Compare August 19, 2018 19:40

anntzer force-pushed the patchdraw branch from 311e95e to 5c564d4 Compare August 20, 2018 13:01

anntzer merged commit 6f37256 into matplotlib:master Aug 21, 2018

anntzer deleted the patchdraw branch August 21, 2018 07:50

QuLogic added this to the v3.1 milestone Aug 21, 2018

ImportanceOfBeingErnest mentioned this pull request Mar 10, 2019

joinstyle is not respected for FancyArrowPatch (either the path or the arrow heads) #13641

Closed

anntzer mentioned this pull request Sep 28, 2020

Line2D and PathPatch have different defaults for capstyle and joinstyle #18597

Open

anntzer mentioned this pull request Jul 11, 2022

Fix dash offset bug in Patch #23412

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out common code between Patch.draw and FancyArrowPatch.draw. #11741

Factor out common code between Patch.draw and FancyArrowPatch.draw. #11741

anntzer commented Jul 22, 2018

jklymak commented Jul 24, 2018

anntzer commented Jul 24, 2018

jklymak commented Jul 24, 2018 •

edited

Loading

anntzer commented Jul 24, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

timhoffm Jul 27, 2018

anntzer Jul 28, 2018

anntzer commented Jul 28, 2018

timhoffm commented Jul 28, 2018 •

edited

Loading

anntzer commented Aug 19, 2018

anntzer commented Aug 21, 2018

Factor out common code between Patch.draw and FancyArrowPatch.draw. #11741

Factor out common code between Patch.draw and FancyArrowPatch.draw. #11741

Conversation

anntzer commented Jul 22, 2018

PR Summary

PR Checklist

jklymak commented Jul 24, 2018

anntzer commented Jul 24, 2018

jklymak commented Jul 24, 2018 • edited Loading

anntzer commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anntzer commented Jul 28, 2018

timhoffm commented Jul 28, 2018 • edited Loading

anntzer commented Aug 19, 2018

anntzer commented Aug 21, 2018

jklymak commented Jul 24, 2018 •

edited

Loading

timhoffm commented Jul 28, 2018 •

edited

Loading