Skip to content

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

Merged
merged 2 commits into from
Feb 17, 2017
Merged

Conversation

tacaswell
Copy link
Member

No description provided.

@tacaswell tacaswell added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Feb 6, 2017
@tacaswell tacaswell added this to the 2.0.1 (next bug fix release) milestone Feb 6, 2017
Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@afvincent afvincent left a 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.

@tacaswell
Copy link
Member Author

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.

@afvincent
Copy link
Contributor

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 dotted line style with default lw value. Besides one gets a significantly better approximation of “dots” with lw=0.8 in the case of grid lines if the lw clipping threshold is set equal to 1 (aspect ratio is then (1×1.1=)1.1 × 0.8), than with this threshold being 2.

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.

@efiring
Copy link
Member

efiring commented Feb 7, 2017

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.

@dopplershift
Copy link
Contributor

dopplershift commented Feb 7, 2017

@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.

@efiring
Copy link
Member

efiring commented Feb 7, 2017

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?

@dopplershift
Copy link
Contributor

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".

Copy link
Contributor

@dopplershift dopplershift left a 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.

@dopplershift
Copy link
Contributor

dopplershift commented Feb 7, 2017

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)

Minimum 2.0:
test

Minimum 1.0:
test

Minimum 0.5:
test

Make sure before judging that you view the full-size version, since your browser/github will be scaling the inline versions.

@dopplershift
Copy link
Contributor

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.

@afvincent
Copy link
Contributor

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:

  1. it may lose the user;
  2. can it be easily worked around? For example, can I, and would it be difficult to get a line with a (0.5, 10, 10, 0.5)-pt pattern style and a width of 0.5 pt, if this is what I really want?

Please note that these two remarks already apply with the current scaling ;)…

@dopplershift
Copy link
Contributor

dopplershift commented Feb 7, 2017

IMO, given:

  1. "useful" linewidth depends on the DPI
  2. Matplotlib is known for publication quality, which means figures at at least 300 DPI

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:

image

That's downright silly.

@tacaswell
Copy link
Member Author

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).

@afvincent
Copy link
Contributor

afvincent commented Feb 8, 2017

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”.

@tacaswell tacaswell force-pushed the api_lw_scale_clipping branch from 5672a90 to d2ba1c0 Compare February 8, 2017 17:29
@afvincent
Copy link
Contributor

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 max(2.0, lw)), aren't they? If it is actually true, does it means that one should scale the former on-off sequences in the rcParams (by a factor × 2/1.5) to keep a consistent behavior?

@tacaswell
Copy link
Member Author

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).

@afvincent
Copy link
Contributor

afvincent commented Feb 13, 2017

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: meeting <- phone call

@afvincent
Copy link
Contributor

Some image tests do not seem happy with the new version of the dash patterns ^^.

@tacaswell tacaswell force-pushed the api_lw_scale_clipping branch from 127b4c8 to c5a59e3 Compare February 17, 2017 04:11
@tacaswell
Copy link
Member Author

@dopplershift This should be ready to go as we discussed on Monday.

@dopplershift
Copy link
Contributor

@afvincent Can you approve/merge since things have changed significantly since your initial approval?

@dopplershift dopplershift changed the title Api lw scale clipping [MRG+1] Api lw scale clipping Feb 17, 2017
@afvincent
Copy link
Contributor

LGTM.

@afvincent afvincent merged commit 2558530 into matplotlib:v2.0.x Feb 17, 2017
@afvincent afvincent changed the title [MRG+1] Api lw scale clipping [MRG+2] Api lw scale clipping Feb 17, 2017
@tacaswell tacaswell deleted the api_lw_scale_clipping branch February 17, 2017 18:24
@afvincent
Copy link
Contributor

BTW @tacaswell , thanks for taking care of this PR :).

@QuLogic QuLogic changed the title [MRG+2] Api lw scale clipping Api lw scale clipping Feb 17, 2017
@afvincent
Copy link
Contributor

@tacaswell I just realized it now, but shouldn't this change be documented in an api_changes or a whats_new entry?

@tacaswell
Copy link
Member Author

In should!

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Mar 13, 2017
QuLogic added a commit that referenced this pull request Mar 18, 2017
has2k1 added a commit to has2k1/plotnine that referenced this pull request May 5, 2017
MPL changed how it draws dashed lines.

Culprit: matplotlib/matplotlib#8032
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants