FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

ENH: enable stripey lines by rcomer · Pull Request #23208 · matplotlib/matplotlib · GitHub

Repository navigation

ENH: enable stripey lines - #23208

Merged
timhoffm merged 1 commit into
matplotlib:mainfrom
rcomer:stripey-lines
Jun 23, 2022
Merged

timhoffm merged 1 commit into
matplotlib:mainfrom
rcomer:stripey-lines

Conversation

rcomer commented Jun 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

PR Summary

Closes #18351 and closes #19053, which had been orphaned, so this replaces. This PR implements a keyword to set a second colour for a line, and create a stripey effect.

The first two commits are cherry-picked from #19053, with some tweaks to address conflicts with the #21050 changes. I'm happy to squash everything down to one commit, but thought I'd leave them separate for now so that my changes are more transparent.

Two outstanding questions:

  1. As per Add the ability to plot striped lines #19053 (review), is offcolor the best name for this new keyword? At Add the ability to plot striped lines #18351, alt_color, coloralt and gapcolor were suggested. coloralt would be consistent with markerfacecoloralt, though gapcolor would seem to me the most consistent with things like facecolor and edgecolor. Since the keyword will only have an effect for dashed styles, I wonder if it should be something like dash_gapcolor for consistency with dash_capstyle and dash_joinstyle.

  2. Would the new gallery example fit better under "Lines, bars and markers" rather than "Pyplot"?

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

Comment thread lib/matplotlib/lines.py Outdated
"""

if offcolor is not None:
if not is_color_like(offcolor) and offcolor != 'auto':

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is there a reason to not use mcolors._check_color_like(color=color) as is done in set_color?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Looks like set_color changed since the original PR. I completely missed that and will change it.

Comment thread lib/matplotlib/lines.py Outdated
----------
drawstyle : {'default', 'steps', 'steps-pre', 'steps-mid', \
'steps-post'}, default: 'default'
'steps-post'}, default: 'default'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This should not be here as it is a continuation of the previous line.

Comment thread lib/matplotlib/lines.py

if linewidth is None:
linewidth = rcParams['lines.linewidth']

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Please do not remove empty lines.

oscargus commented Jun 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Thanks for reviving this!

I think it would be nice if the test image included a legend and a custom dash.

Edit, you mentioned this: At some stage, this will have to be squashed to get rid of the unclean error (some file was added in one commit and removed in another), but that can be done while merging.

Edit: Yes, it would be better to add it in some other example and ideally use something like:

fig, ax = plt.subplots()
ax.plot(..)

oscargus commented Jun 5, 2022

Copy link
Copy Markdown
Member

Oh, and I think this warrant an entry in doc/users/next_whats_new so users can benefit from this! Ideally, a short description of the feature with a small example, see e.g. https://github.com/matplotlib/matplotlib/blob/main/doc/users/next_whats_new/custom_cap_widths.rst

rcomer commented Jun 5, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Yes, it would be better to add it in some other example

Perhaps an extra line or two in Customizing dashed line styles?

oscargus commented Jun 6, 2022

Copy link
Copy Markdown
Member

Yes, it would be better to add it in some other example

Perhaps an extra line or two in Customizing dashed line styles?

Sounds like a great idea!

oscargus commented Jun 6, 2022

Copy link
Copy Markdown
Member

And I agree with you that gapcolor seems like the best option here, but I would probably not risk changing it before someone else has a say as well. If noone chips in before Thursday, I will discuss it at the developer meeting.

(Feel free to join, it is open, there is also a "new contributor meeting" tomorrow: https://hackmd.io/Rvc8Dl2RSi-AsFDgj2rdLw
Although that is not in the calendar: https://calendar.google.com/calendar/u/0/embed?src=79hk8jhvlks8jn8ds4ri1e6q4g@group.calendar.google.com&ctz=America/New_York&pli=1 )

rcomer commented Jun 6, 2022

Copy link
Copy Markdown
Member Author

Thanks @oscargus for your review. I think I have addressed everything except the parameter name. I recycled the old gallery example into the "what's new" entry.

Comment thread lib/matplotlib/lines.py Outdated
if self.is_dashed() and self._offcolor is not None:
lc_rgba = mcolors.to_rgba(self._offcolor, self._alpha)
gc.set_foreground(lc_rgba, isRGBA=True)
gc.set_dashes(0, None)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don't think this is enough. This will just draw a solid line in the background and doesn't look good if you set an alpha on the line. I think you'll need to unwrap the dash pattern and do the inverse dashed pattern.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good point! It also strikes me that the dash cap style can cause some issues here. But I guess that one can probably live with that having a round dash cap style with alpha gives a less than ideal result?

The option would be, I guess, to introduce an "inverted" round cap style. Also, projecting caps will require a bit of extra work here, but should be more easily doable since one knows the additional overlap (linewidth/2).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

So my initial attempt at this failed badly because I assumed that offset would add an extra "gap" to the start of the sequence, when in fact it seems to remove a section from the start instead. I couldn't find this documented anywhere: this docstring links to a "blue book" that might help, but that pdf seems to be only available on certain high security systems.

I've sanity checked this by adapting the linestyle demo, plotting with and without an initial offset of 1, so I know it works for a variety of dash styles:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

It also turned out that the line underneath was problematic even without setting the alpha: the previous test image showed outlines around the lines which disappeared when I updated the main code:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I couldn't find this documented anywhere

I was under the same impression as you. That is clearly something that can be documented more clearly. Although now that I know it, I can sort of understand what offset means, and explain this line in the code:

offset %= dsum

I opened #23220 to track this.

story645 Jun 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

when in fact it seems to remove a section from the start instead. I couldn't find this documented anywhere: this docstring links to a "blue book" that might help, but that pdf seems to be only available on certain high security systems.

https://www.adobe.com/jp/print/postscript/pdfs/PLRM.pdf page 667

The offset operand can be thought of as the “phase” of the dash pattern relative to
the start of the path. It is interpreted as a distance into the dash pattern (measured
in user space units) at which to start the pattern. Before beginning to stroke a
path, the stroke operator cycles through the elements of array, adding up dis-
tances and alternating dashes and gaps as usual, but without generating any out-
put. When the accumulated distance reaches the value specified by offset, it begins
stroking from the starting point of the path, using the dash pattern from the point
that has been reached. Each subpath of a path is treated independently; the dash
pattern is restarted and the offset reapplied at the beginning of each subpath.

Comment thread lib/matplotlib/lines.py Outdated
@_api.define_aliases({
"antialiased": ["aa"],
"color": ["c"],
"offcolor": ["oc"],

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'm not sure this should get an alias. It doesn't seem obvious to me what "oc" means at first glance.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks, I had wondered if this is high profile enough to warrant an alias.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We can always add one later.

oscargus commented Jun 7, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

The capstyle discussion also made me realize that it could be good to, once the basic case is handled, to change to test to use alpha, and multiple points in the plot, ideally making the two lines cross so that the effect of alpha (of one) can be evaluated.

(Multiple points as I cannot really realize what would happen when the lines change direction if there is a gap around there.)

rcomer commented Jun 8, 2022

Copy link
Copy Markdown
Member Author

Multiple points as I cannot really realize what would happen when the lines change direction if there is a gap around there.

The new test image does show some overlaps between the red/black sections when they join on corners 😞

oscargus commented Jun 8, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

when they join on corners

Ahh, that seems hard to solve... However, personally, I still think that there are cases where this is useful so it shouldn't be a showstopper. I am not sure how things like these are handled historically, but I would almost suggest that one adds some sort of "warning" in the docs that this may currently sometimes lead to unwanted effects when using alpha so one better check the results or something along those lines.

greglucas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this looks good and is a useful addition. There are some things that could be improved (caps, alpha-overlap), but I also think those could be quite challenging to handle robustly and can be implemented in follow-up PRs as improvements (I don't think they're worth blocking this over).

I think now you have the hard part left which is choosing the wording:

  • offcolor vs gapcolor vs something-else
  • stripey vs striped

Comment thread lib/matplotlib/lines.py
@@ -0,0 +1,19 @@
Stripey Lines

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think "Striped" with a d sounds better, but that is just a personal preference. Might be regional dialect?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

"Stripey" feels more natural to me but I don't have a strong opinion. Since the original PR and the issue used "striped" that's at least 3 votes to 1!

Comment thread lib/matplotlib/lines.py Outdated
Comment on lines +1064 to +1070
In some cases the two line colors may overlap, which could lead to
unwanted effects if also setting ``alpha``.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this is a fact of the way this is implemented with two separate lines, so the renderer doesn't know they are meant to be the same object and so then it blends the transparencies.

Another option for the striped lines may be to do something similar to the multi-colored line example...
https://matplotlib.org/stable/gallery/lines_bars_and_markers/multicolored_line.html

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The multicolored line is somewhat different from dashes because it assigns values/borders in data coordinates. I.e. you can zoom in to one color segment. With dashes however, we don't want the users to zoom in to a single dash. The dash size is independed of the data coordiantes.

Anyway, I've proposed to decleare this feature experimental so that we still have the freedom to come up with a different implementation.

rcomer force-pushed the stripey-lines branch 2 times, most recently from f153664 to 168535c Compare June 10, 2022 06:09

timhoffm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Given that there could be issues with the rendering and we may want to change some things (e.g. maybe force a flat capstyle?), I suggest to declare this feature experimental so that we still have the feedom to change all aspects based on usage experience.

It would also be nice to mention this functionality in set_dashes.

Comment thread lib/matplotlib/lines.py Outdated
assert_array_equal(line3.get_marker().vertices, triangle1.vertices)


@image_comparison(['striped_line.png'], remove_text=True, style='mpl20')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Interestingly, remove_text does not apply to legend. (Should be investigated at some point).

Still, I propose not to pick up text in the reference image. Do you want the legend entries as a test that the lines are drawn correctly there? If not remove the legend, if yes use label=' '.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@oscargus asked for the legend so I guess he wanted to check the legend lines are drawn correctly.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Correct! Completely happy without text!

Comment thread lib/matplotlib/lines.py Outdated

rcomer commented Jun 12, 2022

Copy link
Copy Markdown
Member Author

Hmmmm. The "Compare" on my latest force-push is showing a bunch of irrelevant changes. I don't know why that should be, as the overall change still looks fine.

y = x**3

fig, ax = plt.subplots()
ax.plot(x, y, linestyle='--', color='orange', second_color='blue',

timhoffm Jun 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

TL;DR I think we should go with gapcolor.

I'm very sorry for going back and forth, but I just realized how this parameter fits into the whole color story.

Seeing this makes me wonder whether second_color is actually a good name Because for

    ax.plot(x, y, linestyle='o', color='orange', second_color='blue',

color affects the marker but second_color does not. This is a certain asymmetry.

Actually the color mechanism is somewhat complicated:

Visual part of Line2D artist explicit parameter Affected by color
marker face markerfacecolor x
additional marker face (for half-filled markers) markerfacecoloralt -
marker edge markeredgecolor x
line - x
this PR: additional line part (for dashed lines) second_color -

Observations:

  • color is a global parameter that can affect multiple visual parts. The behavior is reaonable: Color all visible parts with this color". (It's correct that this does not apply to the additional marker face of half-filled markers, because that would make them fully filled.)
  • Conceptually the gapcolor (I call it like this for now) is similar to markerfacecoloralt - it should and isn't affected by color.
  • We don't have an explicit linecolor parameter. That's also ok. Think: We start with a base color and one can optionally change all marker parameters on top of this.

When only thinking about the line part color + second_color is a perfectly good API. However, taking the whole picture into account, it's not specific enough. IMHO the name has to contain the visual part that is addressed. My suggestions in order of preference:

  • gapcolor
  • second_line_color (very clear, but very long).
  • linecoloralt (rhymes with markerfacecoloralt, but a bit awkward to have ...alt without a linecolor and I don't think we need to add that).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Way too late to change I know, but I don't think it would be bad to have a linecolor alias b/c in the marker+line case, is kinda confusing that the line color is color but the markercolor is markerfacecolor + markeredgecolor (also that there's no markercolor)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think if we had second_line_color then a user might expect there to be a line_color. Someone (@efiring I think) mentioned that “alt color” has problematic meaning in some places.

So gapcolor also seems best to me.

jklymak requested a review from timhoffm June 23, 2022 08:32

jklymak commented Jun 23, 2022

Copy link
Copy Markdown
Member

@timhoffm are you ok with this one?

timhoffm merged commit c19ffdf into matplotlib:main Jun 23, 2022

Copy link
Copy Markdown
Member

@rcomer Thanks for picking this up! This is a really nice feature.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add the ability to plot striped lines

8 participants


Back | FazBrowse Home | New Git URL