Skip to content

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

Merged
merged 33 commits into from
Feb 8, 2016
Merged

Style changes omnibus PR #5774

merged 33 commits into from
Feb 8, 2016

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Dec 31, 2015

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.

@mwaskom
Copy link

mwaskom commented Jan 21, 2016

Is this the right place to comment on style changes, or is there a discussion thread somewhere else?

@mdboom
Copy link
Member Author

mdboom commented Jan 21, 2016

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.

@mwaskom
Copy link

mwaskom commented Jan 21, 2016

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")
Copy link
Member

Choose a reason for hiding this comment

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

This change seems unnecessary.

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

Yea, just the saving.

@mdboom
Copy link
Member Author

mdboom commented Jan 21, 2016

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
Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch.

@QuLogic
Copy link
Member

QuLogic commented Jan 21, 2016

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 lib/matplotlib/tests/baseline_images/test_legend/framealpha.png where it reaches the edge of the legend.

Also, maybe the what's new page should mention the classic style?

@@ -1,4 +1,4 @@
"""
`"""
Copy link
Member

Choose a reason for hiding this comment

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

A new minor typo.

@Tillsten
Copy link
Contributor

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 :(.

@Tillsten
Copy link
Contributor

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],
Copy link
Member

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?

Copy link
Member

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.

@mdboom
Copy link
Member Author

mdboom commented Jan 25, 2016

@Tillsten wrote:

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.

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
Copy link
Member

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.

@WeatherGod
Copy link
Member

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.

@mdboom
Copy link
Member Author

mdboom commented Jan 26, 2016

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.

Makes sense. Easy enough to put it back to the old colors.

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.

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.

Is the current rendering of the docs with this PR available somewhere? Would be interesting to look at the gallary.

Not presently, but I'll put it up somewhere to make that easier to do.

@tacaswell
Copy link
Member

There are some changes to the inset locator demo to disable the auto-ticking which are on master but not in this PR.

@mdboom
Copy link
Member Author

mdboom commented Jan 26, 2016

Thanks for all the helpful feedback, @WeatherGod -- and sorry for not pushing and sending you on some duplicate work.

@WeatherGod
Copy link
Member

That test image you uploaded still fails (~5 RMS)

@mdboom
Copy link
Member Author

mdboom commented Jan 28, 2016

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.

@jenshnielsen
Copy link
Member

@mdboom That should be on the 2.x branch now since #5934 was merged

@zblz
Copy link
Member

zblz commented Jan 31, 2016

@mdboom: Has the proposal in #4730 (using labels 0.1, 1, and 10 in log-scaled axes) been accepted as a default change? I cannot find an implementation with rcParams, but I could whip one up.

@efiring
Copy link
Member

efiring commented Feb 8, 2016

@zblz: In answer to your question, #4730 is not accepted as a default, but will be welcomed as an option with rcParams.

@tacaswell
Copy link
Member

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.

tacaswell added a commit that referenced this pull request Feb 8, 2016
API: Style changes omnibus PR
@tacaswell tacaswell merged commit 673f6c5 into matplotlib:v2.x Feb 8, 2016
@QuLogic
Copy link
Member

QuLogic commented Feb 9, 2016

Now that this PR is merged, where do we give feedback? I see some bugs in the doc build already...

@tacaswell
Copy link
Member

New issues is probably easiest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.