Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fixed hatching in PatchCollection class#27937
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 havingany 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. |
Impaler343 commentedApr 17, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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? |
story645 commentedMay 7, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Could the test failure be related#10034? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hey, sorry for the delay here. Big thing is please add some tests to verify that the code is doing what you think it's supposed to.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| ifhatchisNoneorall(hisNoneforhinhatch): | |
| ifall(hisNoneforhinnp.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
where is this set?
lib/matplotlib/collections.py Outdated
| self.get_transforms(), offsets, offset_trf, | ||
| self.get_facecolor(), self.get_edgecolor(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
these return lists too, so why can't self.get_hatches, w/ changes as needed being implemented down in .draw_path_collection?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Makes sense
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why do you need to add None if next line you only validate if not none?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the case of a list of Nones, the set function makes it into a singleton set with None. So it fails the
ifhatchisnotNone:
condition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, but then I think you don't need theif 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 |
Looking through old PRs, any chance you'll be able to finish this up? |
Have been swamped recently, will be able to pick this up in a week |
I was asked to review this, and I have to admit I don't know very much about the collection classes. I would like to request a high-level overview. What is your diagnosis of the bug? The way hatches now work with collections is wrong somehow, but is the current implementation not doing what the documentation says it should do, or is the documented behaviour less useful than it could be? In which circumstances does the bug occur? Can you make a test case that currently does not pass but will pass once the bug is fixed? |
story645 commentedMay 18, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@jkseppan I'm so sorry, I gave you the wrong link and meant#29392 which you have reviewed (thank you) This PR is waiting on sorting out the proposal for a new GraphicsContext object#29044 (comment) |
@story645 No problem at all! That other PR does seem closer to code that I have authored or edited. I still think a high-level overview would be useful here (and the test cases you suggested might well serve the same goal) but perhaps this has been discussed in another place. |
Uh oh!
There was an error while loading.Please reload this page.
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