Fixed hatching in PatchCollection class #27937
Conversation
lib/matplotlib/collections.py
Outdated
|
|
||
| # Using the hatch of only the first patch | ||
| if patches: | ||
| kwargs['hatch'] = patches[0].get_hatch() |
There was a problem hiding this comment.
This is still problematic because it broadcasts the hatch of the first patch to all of them, if we are matching original then we need to keep track of which (if any) of the patches were originaly hatched.
|
Thank you for your work on this issue. It definitely needs tests and while having any hatch is a step in the right direction, I think using the first hatch for all of the patches is about as wrong as using no-hatches so I think we should have a complete solution here. If we can not broadcast hatch to each of of the patches (which may be a possibility given the vector nature of the collection artists) then we should clearly document that status-quo. |
|
Tried using an approach similar to the one used in drawing contours, drawing patches one at a time. This, however, takes away the speed of drawing as a collection. Tried messing with the C/C++ code and that did not end well. Seems like the function path_collection_generic() draws objects with similar properties only. Also added support for hatches in the form of a tuple. |
|
The RMS is very low(0.008) in the test that is failing, what to do? |
|
Could the test failure be related #10034? |
lib/matplotlib/backend_bases.py
Outdated
| @@ -990,7 +990,7 @@ def get_hatch(self): | |||
| def get_hatch_path(self, density=6.0): | |||
| """Return a `.Path` for the current hatch.""" | |||
| hatch = self.get_hatch() | |||
| if hatch is None: | |||
| if hatch is None or all(h is None for h in hatch): | |||
There was a problem hiding this comment.
| if hatch is None or all(h is None for h in hatch): | |
| if all(h is None for h in np.at_least1d(hatch)): |
h is not None will trigger checking the second condition, which will then try to iterate over a maybe not iterable hatch.
lib/matplotlib/backend_bases.py
Outdated
| @@ -990,7 +990,7 @@ def get_hatch(self): | |||
| def get_hatch_path(self, density=6.0): | |||
| """Return a `.Path` for the current hatch.""" | |||
| hatch = self.get_hatch() | |||
| if hatch is None: | |||
| if hatch is None or all(h is None for h in hatch): | |||
| return None | |||
| return Path.hatch(hatch, density) | |||
There was a problem hiding this comment.
will this need to be looped if passed a list of hatches?
lib/matplotlib/collections.py
Outdated
| @@ -171,6 +171,7 @@ def __init__(self, *, | |||
| # Flags set by _set_mappable_flags: are colors from mapping an array? | |||
| self._face_is_mapped = None | |||
| self._edge_is_mapped = None | |||
| self._match_original = False | |||
lib/matplotlib/collections.py
Outdated
| self.get_transforms(), offsets, offset_trf, | ||
| self.get_facecolor(), self.get_edgecolor(), |
There was a problem hiding this comment.
these return lists too, so why can't self.get_hatches, w/ changes as needed being implemented down in .draw_path_collection?
| If True, use the colors, linewidths, linestyles | ||
| and the hatch of the original patches. | ||
| If False, new colors may be assigned by |
There was a problem hiding this comment.
how are new colors assigned if not passed in as part of the original patches?
lib/matplotlib/collections.py
Outdated
| kwargs['linestyles'] = [p.get_linestyle() for p in patches] | ||
| kwargs['antialiaseds'] = [p.get_antialiased() for p in patches] | ||
| self._match_original = True | ||
| kwargs['facecolors'] = tuple([p.get_facecolor() for p in patches]) |
There was a problem hiding this comment.
why does it need to be a tuple?
| @@ -180,7 +180,7 @@ def __init__(self, hatch, density): | |||
|
|
|||
|
|
|||
| def _validate_hatch_pattern(hatch): | |||
| valid_hatch_patterns = set(r'-+|/\xXoO.*') | |||
| valid_hatch_patterns = set(r'-+|/\xXoO.*').union({None}) | |||
There was a problem hiding this comment.
why do you need to add None if next line you only validate if not none?
There was a problem hiding this comment.
In the case of a list of Nones, the set function makes it into a singleton set with None. So it fails the
if hatch is not None:condition
There was a problem hiding this comment.
Ok, but then I think you don't need the if hatch is not None anymore and can just go straight to validation
lib/matplotlib/collections.py
Outdated
| # Edgecolors are handled separately because are defaulted to None | ||
| # and the Hatch colors depend on them. | ||
| if all(p._original_edgecolor is not None for p in patches): | ||
| kwargs["edgecolors"] = tuple([p.get_edgecolor() for p in patches]) |
There was a problem hiding this comment.
how do you handle a patch collection where some edges are none and some aren't?
|
Sorry for the delay. We have been busy figuring out this PR as we didn't think this would get reviewed first. For now, we have tried to implement Weirdly, with the 'agg' backend, in some cases it works while in the others the code throws a segfault. We haven't been able to identify the cause for this behavior, so it would be really helpful if anyone could provide some insight onto why this might be happening. If the above implementation doesn't work we would have to go back to the previous implementation, where draw_path_collection is called multiple times within a loop. Meanwhile, it would be better if the changes made in #28104 gets reviewed, as it is complete and only needs tests as of now. |
Sorry, this one was easier to review 😅
please don't wait on a review to add tests as tests will make it easier to review -> easier to trace through code given a usage example |
PR summary
Closes #22654
Taking after @hhalwan's PR #22814, with a few of my own proposed changes and questions.
Have removed the determine_facecolor function as this is already checked for, by setting alpha to 0 if get_fill() is set to 0.
Have set edgecolor to black in case of no definition of edge-color in the patches, as it defaults to None, but shouldn't we be setting the edge-color to a color contrasting to the face-color? Is there a function that does this for us?
We can also re-route this PR by decoupling the hatch-color setting and edge-color setting(like in PR #26993), which might make the module clearer.
As we are choosing only the first patch's hatch, for all the patches if match_original is True, I don't think we need warnings as long as we update the API documentation. Thoughts?
Gives the needed output for:
#22654
PR checklist