-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Api lw scale clipping #8032
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
Api lw scale clipping #8032
Conversation
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.
LGTM
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.
It seems reasonable to decrease the line width clipping threshold to 1. At least with default line width, the dotted pattern should now really look “dotted” (i.e. square). And it will be almost square with lw=0.8, which is the default value for grid lines if I am correct. TBH, I do not remember why the threshold was set to such a “high” value in #6547 (I mean, 2 instead of 1.5 for example). Maybe @efiring would know.
However, I am not sure this change will be sufficient to address #7991, as one of the issues seems to be an “excessive” frequency of the dots that leads to a dotted line looking a bit like a straight line.
We made the default 'dotted' pattern to be equal on/off. There is just no way to have a line of ~1px width, square marks, and 50/50 on/off that does not blend into a solid line. |
Maybe I was not clear: what I meant was simply that with the lw clipping threshold of lw=2, then the dots were elongated since their aspect ratio was (2×1.1=)2.2 × 1.5, while with a threshold ≤ 1.5, they are almost real squares: (1.5×1.1=)1.65 × 1.5. So I think adopting a lw clipping threshold below 1.5 is a good thing for this reason: one gets (almost) “dots” when asking for About the problem raised in #7991 , it is more focused on the case of grid lines, which are a “subset” of lines. I think that #8040 may be a suitable solution for every body. Most user will not bother tweaking grid lines and will rely on the new solid line style introduced with mpl 2.0, while the ones who prefer the old style will be able to adapt it in rcParams. |
Looking at the boxplot replacement (the first image file that is changed) shows why the threshold was set the way it was: the dots in the new version are so small that I don't think they are very functional, while the old version was highly functional. |
@efiring It may have been functional. It was not a dotted line. I'd be ok changing to a dashed style to keep the same aesthetic appearance. |
If you want to go for linguistic purity over graphical functionality, why have a threshold at all? Maybe the threshold should be applied to the spacing, not to the dots? |
It's not "linguistic purity"--it's called not confusing or surprising our users. I'm not interested in semantic arguments--If I ask for a dotted line, I don't expect to see a dashed line. What matplotlib was displaying before was definitively "dashed", and it's the kind of thing that leads people to lose time wondering why matplotlib insisted on dashes when they asked for dotted. The new behavior, while visually less pleasing, at least gives the user what they asked for and leads to the path of "that's too small, maybe I should use a thicker line". |
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'm actually going to change and suggest we go smaller, say 0.5--or even remove the threshold. Unless there's a bug, the useful minimum width depends on the DPI.
Here are some example images, all at 300 DPI. import matplotlib.pyplot as plt
fig, ax = plt.subplots()
ax.grid(linestyle='dotted')
fig.savefig('test.png', dpi=300) Make sure before judging that you view the full-size version, since your browser/github will be scaling the inline versions. |
And if I go to 600 DPI and turn off the linewidth threshold, I can get 0.25 linewidth to look fine dotted and can even make out individual dots at a linewidth of 0.1. |
I agree that having a “dotted” line style that is more “short-dashed” than dotted appears to be rather confusing: they are several issue tickets that noticed it (and we are only talking of the people coming to GitHub). On the other hand, it is true that the threshold of lw=1 seems to be more or less the boundary between “kind of functional” and “not so functional“ with the defaults (but it is also DPI-dependent, so…). The idea of having some kind of threshold applied only to the off ink parts like @efiring suggested (if I understood correctly) is something that I was thinking about too, and I think it would be worth trying it. Nevertheless, we may want to be extra careful with all the “intelligence” we put in this scaling because:
Please note that these two remarks already apply with the current scaling ;)… |
IMO, given:
We should not be thresholding the linewidth for scaling the dash/dot pattern. Provide sane defaults, but don't stand in the way of giving the user what they want--and right now, the library stands in the way. Here's the closest I can get to a thin dotted line at 300 DPI on current master: That's downright silly. |
I am convinced, will get this fixed some time tonight (will probably get to my destination before the tests finish running on my laptop to get update the images). |
Would there be an interest to implement something inspired by what @rougier suggested in #7087, i.e. adding a few new named line styles? I am especially thinking about “loose” versions of the current non solid styles (e.g. "--:loose" or "dotted:loose" for a pattern like (1.1, 3.3) instead of (1.1, 1.1)). This way, we could provide “sane” defaults for people who do not (want to) know what values to use in order to get less dense dashed or dotted line styles. I would be happy to give it a shot in a separate PR if somebody thinks that it would be worth it (or at least nice trying ;) ). Edit: I opened a separate PR (#8048) to demonstrate what I have in mind about supplementary line styles to easily get “loosely dashed lines”. |
5672a90
to
d2ba1c0
Compare
Just a thought. If one removes the line width-controlled dash scaling, then all the non solid patterns with the default lw = 1.5 are changed (as previously the scaling threshold was |
If we change the patterns or the scaling factor, then we change everything else. @dopplershift convinced me that the clipping was a mistake to begin with and we should take this chance to get get rid of it (which means this needs a bunch of docs added). |
I agree with getting rid of the clipping, for the reasons @dopplershift explained. I was simply concerned that we could change the default pattern, as it is/was impacted by the clipping (default lw is 1.5 and clipping threshold was 2), without noticing it. If I am correct, such a change does not really get tested within our test suite as most of the examples (i.e. all of them but the four ones that you updated) use the “classic” style, don't they? Anyway, could this be talked about during the next Monday phone call? I think this is something that definitively needs careful attention as it will impact most of the graphs. By the way, I think that the ideas in #8040 (that makes possible to globally fine tune the grid line style) and in #8048 (that introduces new (“loose”) line styles, besides the current ones) may be solutions to help the users that would “suffer” from the removing of the clipping. But I guess I am biaised here ;). Edit: |
Some image tests do not seem happy with the new version of the dash patterns ^^. |
127b4c8
to
c5a59e3
Compare
@dopplershift This should be ready to go as we discussed on Monday. |
@afvincent Can you approve/merge since things have changed significantly since your initial approval? |
LGTM. |
BTW @tacaswell , thanks for taking care of this PR :). |
@tacaswell I just realized it now, but shouldn't this change be documented in an |
In should! |
DOC: missing API docs from #8032
MPL changed how it draws dashed lines. Culprit: matplotlib/matplotlib#8032
No description provided.