Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
afvincent merged 2 commits intomatplotlib:v2.0.xfromtacaswell:api_lw_scale_clipping
Feb 17, 2017

Conversation

tacaswell
Copy link
Member

No description provided.

@tacaswelltacaswell added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelFeb 6, 2017
@tacaswelltacaswell added this to the2.0.1 (next bug fix release) milestoneFeb 6, 2017
Copy link
Contributor

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

@afvincentafvincent 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 nowreally 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
MemberAuthor

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 fordotted 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

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 commentedFeb 7, 2017
edited
Loading

@efiring It may have been functional. It wasnot a dotted line. I'd be ok changing to a dashed style to keep the same aesthetic appearance.

@efiring
Copy link
Member

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

@dopplershiftdopplershift left a comment
edited
Loading

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 commentedFeb 7, 2017
edited
Loading

Here are some example images, all at 300 DPI.

importmatplotlib.pyplotaspltfig,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 commentedFeb 7, 2017
edited
Loading

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
MemberAuthor

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 commentedFeb 8, 2017
edited
Loading

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

@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 wasmax(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
MemberAuthor

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 commentedFeb 13, 2017
edited
Loading

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 itis/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
Copy link
MemberAuthor

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

@dopplershiftdopplershift changed the titleApi lw scale clipping[MRG+1] Api lw scale clippingFeb 17, 2017
@afvincent
Copy link
Contributor

LGTM.

@afvincentafvincent merged commit2558530 intomatplotlib:v2.0.xFeb 17, 2017
@afvincentafvincent changed the title[MRG+1] Api lw scale clipping[MRG+2] Api lw scale clippingFeb 17, 2017
@tacaswelltacaswell deleted the api_lw_scale_clipping branchFebruary 17, 2017 18:24
@afvincent
Copy link
Contributor

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

@QuLogicQuLogic changed the title[MRG+2] Api lw scale clippingApi lw scale clippingFeb 17, 2017
@afvincent
Copy link
Contributor

@tacaswell I just realized it now, but shouldn't this change be documented in anapi_changes or awhats_new entry?

@tacaswell
Copy link
MemberAuthor

In should!

tacaswell added a commit to tacaswell/matplotlib that referenced this pull requestMar 13, 2017
QuLogic added a commit that referenced this pull requestMar 18, 2017
has2k1 added a commit to has2k1/plotnine that referenced this pull requestMay 5, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@dopplershiftdopplershiftdopplershift approved these changes

@afvincentafvincentafvincent approved these changes

@efiringefiringAwaiting requested review from efiring

Assignees
No one assigned
Labels
Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Milestone
v2.0.1
Development

Successfully merging this pull request may close these issues.

4 participants
@tacaswell@afvincent@efiring@dopplershift

[8]ページ先頭

©2009-2025 Movatter.jp