-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Style changes omnibus PR #5774
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
Style changes omnibus PR #5774
Conversation
Is this the right place to comment on style changes, or is there a discussion thread somewhere else? |
If you have specific issues related to these changes feel free to make them here, but the ship has largely sailed on subjective decision making. |
bon voyage, then |
plt.hist2d(data[:, 0], data[:, 1], | ||
bins=100, norm=mcolors.PowerNorm(gamma)) | ||
|
||
plt.subplots_adjust(hspace=0.39) | ||
plt.subplots_adjust(hspace=0.8) | ||
plt.savefig("test.png") |
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.
This change seems unnecessary.
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.
You mean just the saving, not the spacing change, right? I agree about the saving -- I think that's just left in accidentally.
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.
Yea, just the saving.
Sorry if I seemed prickly -- that wasn't my intention. The longer story is that we have already put out the request for suggested style changes (through promotion at conferences, the mailing list and a banner on the website) -- that generated many PRs from which Thomas and I selected the best options, taking into account "best for the common case" issues, and consistency etc. This PR is the culmination of that and is largely in the round of finding bugs that it reveals, fixing poor interactions between choices etc., but the big design stuff is mostly over so that we can make a 2.0 release without too much further bikeshedding. All that said, we sincerely hope that the style infrastructure will make it easier for those the disagree with the defaults to easily override them. |
@@ -256,7 +256,7 @@ legend.fancybox : False # if True, use a rounded box for the | |||
# legend, else a rectangle | |||
legend.loc : upper right | |||
legend.isaxes : True # this option is internally ignored | |||
legend.numpoints : 2 # the number of points in the legend line | |||
legend.numpoints : 1 # the number of points in the legend 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.
The classic style shouldn't be changing, right?
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.
Good catch.
As I understand it, all the test images should still be using the classic style. Something appears to be wrong with the handle length in legends as they seem to have widened in several images. It is most evident in Also, maybe the what's new page should mention the classic style? |
@@ -1,4 +1,4 @@ | |||
""" | |||
`""" |
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.
A new minor typo.
I would really like to see more changes to make the legend not so spacious, the defaults are quite unuseable with more than two entries. Also i really dislike the rounded corners, which is a change i never saw discussed anywhere :(. |
I would also like to see the default markersize decreased. |
|
||
# this option is internally ignored - it never served any useful purpose | ||
'legend.isaxes': [True, validate_bool], | ||
|
||
# the number of points in the legend line | ||
'legend.numpoints': [2, validate_int], | ||
'legend.numpoints': [1, validate_int], | ||
# the number of points in the legend line for scatter | ||
'legend.scatterpoints': [3, validate_int], |
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.
Is this also going to change (to go along with numpoints
), or is that undecided?
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.
yeah, I thought this is what was meant by changing the number of points in the legend.
@Tillsten wrote:
Thanks for the feedback. As things get integrated, it may be that this does or does not make sense. Unfortunately, the official window to provide style suggestions has closed, but we're obviously still open to feedback about things in the existing changes that don't interact well or make specific kinds of common plots much worse. |
#grid.linestyle : : # dotted | ||
#grid.linewidth : 0.5 # in points | ||
#grid.alpha : 1.0 # transparency, between 0.0 and 1.0 | ||
#grid.color : '#b0b0b0' # grid color |
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.
can't use pound sign in rcfile.
A comment about the inset locator demo. With the new colors, it is hard to see the inset lines. Personally, I liked the demo the way it was. The new colors there are very dark. Also, the ticks in that demo just seems "weird" to me. There is no x tick on the left corners, but there is one on the right corner. Is the current rendering of the docs with this PR available somewhere? Would be interesting to look at the gallary. |
Makes sense. Easy enough to put it back to the old colors.
Good point. I don't think it's a bug -- it's just the new view limiting coming into play. But we could hardcode the limits or change the locator to fix the weirdness.
Not presently, but I'll put it up somewhere to make that easier to do. |
There are some changes to the inset locator demo to disable the auto-ticking which are on master but not in this PR. |
Thanks for all the helpful feedback, @WeatherGod -- and sorry for not pushing and sending you on some duplicate work. |
That test image you uploaded still fails (~5 RMS) |
Yeah -- I think #5932 fixes that correctly. I'll just wait for that to merge and rebase. |
Merging this as-is so we can see the built docs. The 3.5 failure looks like travis flakyness, but I will fix in a follow on PR if that is really broken. |
API: Style changes omnibus PR
Now that this PR is merged, where do we give feedback? I see some bugs in the doc build already... |
New issues is probably easiest. |
This is the amalgamation of many style changes slated for v2.0, all put together so we can see how these changes interact and work as a whole.