FIX: Restore hatch color tracking edge color #7976

tacaswell · 2017-01-28T23:30:42Z

Addresses #7901

Still needs docs and a closer look at the test images that get updated.

tacaswell · 2017-01-28T23:33:55Z

Something is wrong with the hatch color of either collection or patches in svg (but not png).

NelleV · 2017-01-29T03:39:26Z

lib/matplotlib/backend_bases.py

+        Gets the color to use for hatching.
+        """
+        if hatch_color is None:
+            raise ValueError()


Can you add a message to the error, so that the user understands what's going on?

Also, don't you want to check that the hatch_color provided is a color? That should fix the value error message.

this is currently a debugging latch, will probably remove it before final review.

NelleV · 2017-01-29T03:40:54Z

lib/matplotlib/collections.py

+                    gc.set_hatch_color(self._hatch_color)
+                except AttributeError:
+                    # if we end up with a GC that does not have this method
+                    import warnings


Any reason not to move this import at the top of the file?

NelleV · 2017-01-29T03:41:39Z

lib/matplotlib/collections.py

+                except AttributeError:
+                    # if we end up with a GC that does not have this method
+                    import warnings
+                    warnings.warn("Your backend does not have support for "


"Your backend doesn't support setting the hatch color" is clearer for me (might be because I am a non native english speaker).

tacaswell · 2017-01-29T05:28:13Z

attn @jkseppan as I touched the innards of the pdf backend and attn @QuLogic as you were the last person to touch hatching.

tacaswell · 2017-01-29T05:42:29Z

An eye-ball check confirms that the eps backend is working correctly as well :)

tacaswell · 2017-01-29T06:04:41Z

ok, I think I am happy with where this landed (and it ended up being a lot less work (once I understood a few bits of code) than I had anticipated).

if an explicit edge color is set, use that for the hatch
if the default edge color (None -> 'none') then use rcParam['hatch.color']
bind rcparam at artist creation time (not at draw time)

Still needs some documentation work which I will do tomorrow.

anntzer · 2017-01-29T06:48:23Z

lib/matplotlib/backend_bases.py

@@ -1111,6 +1113,12 @@ def get_hatch_color(self):
        """
        return self._hatch_color

+    def set_hatch_color(self, hatch_color):
+        """
+        Gets the color to use for hatching.


anntzer · 2017-01-29T06:50:30Z

lib/matplotlib/collections.py

+                gc.set_hatch_color(self._hatch_color)
+            except AttributeError:
+                # if we end up with a GC that does not have this method
+                warnings.warn("Your backend does not support setting the "


Could you define set_hatch_color in the base graphicscontext class as a noop that prints the warning? (would avoid the repetition below).

It is defined as a functional function it the base class, this is in case there are backends in the wild that do not derive from our base class.

Could we suggest them to derive from our base class if they don't have set_hatch_color?

Why? On the draw method side we are ok duck-typing and the gc objects should belong to the backends so they should free to implement how ever they want.

Just that it'd be an easy fix to get this working.

In this particular case, it seems better practice than duck typing :)

NelleV

(I am still reviewing this pull request, but I think I'll need some time to get around the code.)

NelleV · 2017-01-29T18:22:46Z

lib/matplotlib/backends/backend_pdf.py

@@ -2210,14 +2210,14 @@ def alpha_cmd(self, alpha, forced, effective_alphas):
        name = self.file.alphaState(effective_alphas)
        return [name, Op.setgstate]

-    def hatch_cmd(self, hatch):
+    def hatch_cmd(self, hatch, hatch_color):


Can you explain this extra argument instead of using self._hatch_color?

Because of the way the pdf backend deals with the fact that internally pdf's have a stack-based graphics context so when we change the gc in the draw methods, the delta method is used to determine what is different between the 'current' gc and the 'gc' that was just passed in. The way that method works is by checking a bunch of internal state and if it needs to be updated emit some commands to the pdf file. hatch_cmd is what generates the hatch commands. To tell if we need to change the hatch we need to check both _hatch and _hatch_color and then all of the parameters you check are passed into the command function (see L2323 ish). And the instance of the gc that the command methods are bound to is the 'old' gc, not the 'new' gc so we have to inject it this way.

It is possible that the pdf backend could be cleaned up, but that is a much bigger project.

sorry this is rambley, but it took me a while to sort this out last night.

I think I understand…

I feel the same way 😅

Also, this is made more complicated by how we implement hatching in pdf: it replaces the fill color with a named pattern. I think the backend_pdf changes are correct, and you added tests for the new functionality so we should be fine.

NelleV · 2017-01-29T18:24:16Z

lib/matplotlib/collections.py

+                gc.set_hatch_color(self._hatch_color)
+            except AttributeError:
+                # if we end up with a GC that does not have this method
+                warnings.warn("Your backend does not support setting the "


Could we suggest them to derive from our base class if they don't have set_hatch_color?

tacaswell · 2017-01-29T18:49:16Z

force-pushed to remove some test-thrashing + add docs.

NelleV

This looks ok to me, but I have to say that I am very happy that there are tests on this section of the code. It also seems very convoluted, so an additional careful review is in order.

tacaswell · 2017-01-29T19:38:19Z

doc/api/api_changes.rst

+to color the hatches.
+
+In the future there may also be ``hatch_linewidth`` and
+``hatch_density`` related methods added.  It is encouraged, but not


@NelleV Added the 'please derive from us' verbiage here.

Are you canceling by hand the old travis-ci tests or have we activated the new beta feature to cancel tests on PRs if a newer version exists?

I am canceling them by hand.

dstansby · 2017-01-30T14:23:59Z

doc/users/dflt_style_changes.rst

+
+The color of the lines in the hatch is now determined by
+
+ - If an edge color is explicitly, use that for the hatch color


'is explicitly set'

dopplershift

Pending CI success.

tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Jan 28, 2017

tacaswell added this to the 2.0.1 (next bug fix release) milestone Jan 28, 2017

NelleV reviewed Jan 29, 2017

View reviewed changes

tacaswell force-pushed the fix_hatch_color branch 2 times, most recently from bdb3679 to 7196cd5 Compare January 29, 2017 05:11

tacaswell changed the title ~~FIX: Restore hatch color tracking edge color~~ [MRG] FIX: Restore hatch color tracking edge color Jan 29, 2017

anntzer reviewed Jan 29, 2017

View reviewed changes

NelleV reviewed Jan 29, 2017

View reviewed changes

tacaswell force-pushed the fix_hatch_color branch from 6eb84ef to c8aa6bf Compare January 29, 2017 18:49

FIX: Restore hatch color tracking edge color

f6b4a32

tacaswell force-pushed the fix_hatch_color branch from c8aa6bf to f6b4a32 Compare January 29, 2017 18:50

NelleV approved these changes Jan 29, 2017

View reviewed changes

NelleV changed the title ~~[MRG] FIX: Restore hatch color tracking edge color~~ [MRG+1] FIX: Restore hatch color tracking edge color Jan 29, 2017

DOC: add whats_new and api_changes entries for gc hatch methods

a58b0d1

tacaswell commented Jan 29, 2017

View reviewed changes

dstansby reviewed Jan 30, 2017

View reviewed changes

dstansby mentioned this pull request Jan 30, 2017

Hatching doesn't respect alpha #7992

Closed

tacaswell mentioned this pull request Feb 3, 2017

Several hatching improvements #7421

Merged

DOC: add missing word

acda4ac

dopplershift approved these changes Feb 6, 2017

View reviewed changes

QuLogic changed the title ~~[MRG+1] FIX: Restore hatch color tracking edge color~~ FIX: Restore hatch color tracking edge color Feb 6, 2017

QuLogic merged commit d63c95d into matplotlib:v2.0.x Feb 6, 2017

tacaswell deleted the fix_hatch_color branch February 6, 2017 22:14

tacaswell mentioned this pull request Feb 14, 2017

Allow multiple hatch colors in same figure #7901

Closed

dopplershift mentioned this pull request Feb 15, 2017

Matplotlib hatch color broken #8076

Closed

jchutrue mentioned this pull request Mar 22, 2017

add hatch color to epochplot nelpy/nelpy#153

Open

tacaswell mentioned this pull request Apr 10, 2017

Hatches filling an empty color contour not saved in pdf #8431

Closed

tacaswell mentioned this pull request Apr 8, 2020

PDF backend can now do alpha on hatches. #17049

Draft

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: Restore hatch color tracking edge color #7976

FIX: Restore hatch color tracking edge color #7976

tacaswell commented Jan 28, 2017

tacaswell commented Jan 28, 2017

NelleV Jan 29, 2017

NelleV Jan 29, 2017

tacaswell Jan 29, 2017

NelleV Jan 29, 2017

NelleV Jan 29, 2017

tacaswell commented Jan 29, 2017

tacaswell commented Jan 29, 2017

tacaswell commented Jan 29, 2017

anntzer Jan 29, 2017

anntzer Jan 29, 2017

tacaswell Jan 29, 2017

NelleV Jan 29, 2017

tacaswell Jan 29, 2017

NelleV Jan 29, 2017

NelleV Jan 29, 2017

NelleV left a comment

NelleV Jan 29, 2017

tacaswell Jan 29, 2017

NelleV Jan 29, 2017

tacaswell Jan 29, 2017

jkseppan Jan 29, 2017

NelleV Jan 29, 2017

tacaswell commented Jan 29, 2017

NelleV left a comment

tacaswell Jan 29, 2017

NelleV Jan 29, 2017

tacaswell Jan 29, 2017

dstansby Jan 30, 2017

dopplershift left a comment


		The color of the lines in the hatch is now determined by

		- If an edge color is explicitly, use that for the hatch color

FIX: Restore hatch color tracking edge color #7976

FIX: Restore hatch color tracking edge color #7976

Conversation

tacaswell commented Jan 28, 2017

tacaswell commented Jan 28, 2017

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

tacaswell commented Jan 29, 2017

tacaswell commented Jan 29, 2017

tacaswell commented Jan 29, 2017

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

NelleV left a 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

tacaswell commented Jan 29, 2017

NelleV left a 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

dopplershift left a comment

Choose a reason for hiding this comment