| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
| """ | ||
|
|
||
| if offcolor is not None: | ||
| if not is_color_like(offcolor) and offcolor != 'auto': |
There was a problem hiding this comment.
Is there a reason to not use mcolors._check_color_like(color=color) as is done in set_color?
Sorry, something went wrong.
There was a problem hiding this comment.
Looks like set_color changed since the original PR. I completely missed that and will change it.
Sorry, something went wrong.
| ---------- | ||
| drawstyle : {'default', 'steps', 'steps-pre', 'steps-mid', \ | ||
| 'steps-post'}, default: 'default' | ||
| 'steps-post'}, default: 'default' |
There was a problem hiding this comment.
This should not be here as it is a continuation of the previous line.
Sorry, something went wrong.
|
|
||
| if linewidth is None: | ||
| linewidth = rcParams['lines.linewidth'] | ||
|
|
There was a problem hiding this comment.
Please do not remove empty lines.
Sorry, something went wrong.
|
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(..) |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
Perhaps an extra line or two in Customizing dashed line styles? |
Sorry, something went wrong.
Sounds like a great idea! |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| 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) |
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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).
Sorry, something went wrong.
There was a problem hiding this comment.
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:
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
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:
matplotlib/lib/matplotlib/lines.py
Line 57 in 8e2987b
I opened #23220 to track this.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| @_api.define_aliases({ | ||
| "antialiased": ["aa"], | ||
| "color": ["c"], | ||
| "offcolor": ["oc"], |
There was a problem hiding this comment.
I'm not sure this should get an alias. It doesn't seem obvious to me what "oc" means at first glance.
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks, I had wondered if this is high profile enough to warrant an alias.
Sorry, something went wrong.
There was a problem hiding this comment.
We can always add one later.
Sorry, something went wrong.
|
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.) |
Sorry, something went wrong.
The new test image does show some overlaps between the red/black sections when they join on corners 😞 |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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:
Sorry, something went wrong.
| @@ -0,0 +1,19 @@ | |||
| Stripey Lines | |||
There was a problem hiding this comment.
I think "Striped" with a d sounds better, but that is just a personal preference. Might be regional dialect?
Sorry, something went wrong.
There was a problem hiding this comment.
"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!
Sorry, something went wrong.
| In some cases the two line colors may overlap, which could lead to | ||
| unwanted effects if also setting ``alpha``. |
There was a problem hiding this comment.
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
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| assert_array_equal(line3.get_marker().vertices, triangle1.vertices) | ||
|
|
||
|
|
||
| @image_comparison(['striped_line.png'], remove_text=True, style='mpl20') |
There was a problem hiding this comment.
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=' '.
Sorry, something went wrong.
There was a problem hiding this comment.
@oscargus asked for the legend so I guess he wanted to check the legend lines are drawn correctly.
Sorry, something went wrong.
There was a problem hiding this comment.
Correct! Completely happy without text!
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| y = x**3 | ||
|
|
||
| fig, ax = plt.subplots() | ||
| ax.plot(x, y, linestyle='--', color='orange', second_color='blue', |
There was a problem hiding this comment.
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:
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:
Sorry, something went wrong.
There was a problem hiding this comment.
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)
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
@rcomer Thanks for picking this up! This is a really nice feature. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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:
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.
Would the new gallery example fit better under "Lines, bars and markers" rather than "Pyplot"?
PR Checklist
Tests and Styling
Documentation