-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Stripey LineCollection
#24849
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Stripey LineCollection
#24849
Conversation
@@ -406,6 +410,17 @@ def draw(self, renderer): | |||
gc, paths[0], combined_transform.frozen(), | |||
mpath.Path(offsets), offset_trf, tuple(facecolors[0])) | |||
else: | |||
if self._gapcolor is 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.
Note that do_single_path_optimization
is only True
if all the lines are solid
matplotlib/lib/matplotlib/collections.py
Line 381 in fc5d316
all(ls[1] is None for ls in self._linestyles) and |
so
gapcolor
is only meaningful after this else
.
@rcomer for a new feature like this, its nice to either include example code in the description, or in documentation of the feature. I'm not quite following the API from the test images and the tests (though I only tried for a minute or so). |
Thanks @jklymak, I have updated the OP. |
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 seems fine to me, despite never using v/hlines ;-)
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.
Wording nits but I'm happy w/this. Thanks! (Also sorry for dragging this into the color validation arguments)
There can be overlaps between those two, which may result in | ||
artifacts when using transparency. | ||
|
||
This functionality is experimental and may change. |
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.
smallest little nit but I'd put only this very last sentence in the note and have everything else as just part of the docstring.
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.
The note here was lifted straight from the equivalent in Line2D
.
https://matplotlib.org/stable/api/_as_gen/matplotlib.lines.Line2D.html#matplotlib.lines.Line2D.set_gapcolor
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.
I think it should be changed there too but out of scope so I can put in that PR
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.
From what I remember of #23208, this went through a few iterations: started out with a warning about overlaps that don't look great with transparency. Then the warning was downgraded to a note, and the "experimental" bit added later. Given the amount of previous bikeshedding on this, I would be uncomfortable about changing it without a bit more discussion. That could happen in a follow-up PR, as you say.
lib/matplotlib/collections.py
Outdated
Used when gapcolor is set. Return the path and inverse pattern for | ||
non solid lines, so we can use these to fill the gaps. For solid lines | ||
we insert a path of nans so that no inverse line is drawn in that case. |
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.
Used when gapcolor is set. Return the path and inverse pattern for | |
non solid lines, so we can use these to fill the gaps. For solid lines | |
we insert a path of nans so that no inverse line is drawn in that case. | |
Returns the path and pattern for the gaps in the non-solid lines. | |
This path and pattern is the inverse of the path and pattern used to construct the non-solid lines. For solid lines, we set the inverse path to nans to prevent drawing an inverse line. |
Wordsmithing, take or leave
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.
Taken!
lib/matplotlib/collections.py
Outdated
non solid lines, so we can use these to fill the gaps. For solid lines | ||
we insert a path of nans so that no inverse line is drawn in that case. | ||
""" | ||
pairs = [ |
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.
Pairs is path_pattern?
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.
Do you mean I should change the variable name? Happy to do so if so.
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.
Was confirming that I understood what the code was doing but if I'm right than yes please
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.
Done
4f872b9
to
3b4bd83
Compare
Does anyone have an opinion on this Codecov failure? It seems unlikely to me that such a simple method could get broken. |
Thanks @rcomer ! |
PR Summary
Closes #24796.
Following #23208, which implemented the
gapcolor
property forLine2D
, this PR implementsgapcolor
forLineCollection
. So we can set a colour (or list of colours) to fill the gaps in dashed lines and make a striped effect. For exampleI marked this functionality as "experimental" to be consistent with
Line2D
.My current implementation works, but I'm pretty unsure about the way I've structured it: it doesn't seem great to have the
LineCollection
-specific handling withinCollection
. The only way I can see to avoid that is to defineLineCollection.draw
, but that would mean duplicating some code, which also wouldn't seem great.For reference, the test images look like this:
gapcolor='orange'
gapcolor=['r', 'k']
I'm happy to squash the commits down once we're happy with the implementation, but I suspect there will be plenty of changes before that...
PR Checklist
Documentation and Tests
pytest
passes)Release Notes
.. versionadded::
directive in the docstring and documented indoc/users/next_whats_new/
.. versionchanged::
directive in the docstring and documented indoc/api/next_api_changes/
next_whats_new/README.rst
ornext_api_changes/README.rst