Skip to content
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

Adding two new styles to mplstyles #3190

Merged
merged 5 commits into from Jul 12, 2014

Conversation

CamDavidsonPilon
Copy link
Contributor

  1. The first, 538.mplstyle is a replication of the plots used on FiveThirtyEight.com, Nate Silver's blog. I wrote an blog article on the replication here (with some samples).
  2. The second, bmh.mplstyle, is a what is used in Bayesian Methods for Hackers. I think it also won the PlotOrNot contest from last year ¯_(ツ)_/¯?

cc @ChrisBeaumont

@tacaswell
Copy link
Member

Can you squash these down to two commits?

@CamDavidsonPilon
Copy link
Contributor Author

@tacaswell done! (and learned some new git too)

axes.color_cycle: 348ABD, A60628, 7A68A6, 467821, D55E00, CC79A7, 56B4E9, 009E73, F0E442, 0072B2

legend.fancybox: True
figure.figsize : 11, 8figure.dpi : 100
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Error here, I'll fix

Update 538.mplstyle

Update bmh.mplstyle

fixing line in bmh.mplstyle
@tacaswell tacaswell added this to the v1.4.0 milestone Jul 6, 2014
@tacaswell
Copy link
Member

This is the first round of style sheets we have gotten so there is no precedent, but I see no reason not to merge them for 1.4.0. @tonysyu you have comments?

@tonysyu
Copy link
Contributor

tonysyu commented Jul 6, 2014

Thanks for pinging me @tacaswell. The styles themselves are quite nice, and it'd be great to have more stylesheets included. I'm not crazy about the naming: 538 would probably be better as 'fivethirtyeight' (to match all their branding) and 'bmh' is a bit too terse for me, but I don't have a better suggestion.

Actually, one constructive criticism: I don't think most public stylesheets should have DPI specified. We should prefer the user's personal DPI preference, and possibly their figure size preference as well. The exception would be for specific publication-specific stylesheets, which may have exacting specifications.

@CamDavidsonPilon
Copy link
Contributor Author

@tonysyu, I've addressed those issues: removed dpi/figsize settings, and renamed to fivethirtyeight

@Tillsten
Copy link
Contributor

Tillsten commented Jul 6, 2014

Adding a section with the available styles to https://github.com/matplotlib/matplotlib/blob/master/doc/users/style_sheets.rst would be wonderful. Even better with a picture per style.

@CamDavidsonPilon
Copy link
Contributor Author

mmm, would that be better on the matplotlib website? I see those docs purely for creating new styles, not seeing what's available.

@tacaswell
Copy link
Member

Those docs are what are used to generate the website.

@CamDavidsonPilon
Copy link
Contributor Author

Best place to put the images in /doc/_static? Thoughts on the size of the images?

@tonysyu
Copy link
Contributor

tonysyu commented Jul 6, 2014

Actually, I think a plot script, rather than an image, would be preferable. Currently, there are style examples in the gallery here:
https://github.com/matplotlib/matplotlib/tree/master/examples/style_sheets

@tonysyu
Copy link
Contributor

tonysyu commented Jul 6, 2014

I should note that the plot should be fairly simple; something closer to plot_dark_background.py in that section, rather than the other two. Long term, it might be nice to have style sheets run with some "standard" plot for easy comparison (and less code).

@tacaswell
Copy link
Member

Ideally we would have one chunk of code that would just loop over all of the style sheets an generate a figure. That requires understanding a bit more of the sphinx build system than I currently do.

@tonysyu
Copy link
Contributor

tonysyu commented Jul 6, 2014

Sorry, I wasn't very clear earlier: That's essentially what I was getting at; I was just pointing out that it would be nice to come up with a plot that exercises most of the configurable parameters in a stylesheet.

Generating all the figures shouldn't need to involve any Sphinx machinery: If you just plot multiple figures from a single script, they're rendered in the gallery as separate plots; e.g.

http://matplotlib.org/examples/color/colormaps_reference.html

@CamDavidsonPilon
Copy link
Contributor Author

Kk, I've adding two scripts to generate pretty images using the new styles


x = np.linspace(0,10)

figure(figsize=(6,4))
Copy link
Member

Choose a reason for hiding this comment

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

missing plt

adding np and plt into script
@CamDavidsonPilon
Copy link
Contributor Author

bump

plt.hist(beta(10,10,size=10000),histtype="stepfilled", bins=25, alpha=0.8, normed=True)
plt.hist(beta(4,12,size=10000),histtype="stepfilled", bins=25, alpha=0.7, normed=True)
plt.hist(beta(50,12,size=10000),histtype="stepfilled", bins=25, alpha=0.8, normed=True)
plt.hist(beta(6,55,size=10000),histtype="stepfilled", bins=25, alpha=0.8, normed=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

These lines raise some flake8 noise. Maybe you could define a small hist function to remove some repetition; e.g.

def step_hist(x):
    plt.hist(x, histtype="stepfilled", bins=25, alpha=0.8, normed=True)

Also, make sure there's a space after your commas.

@tonysyu
Copy link
Contributor

tonysyu commented Jul 8, 2014

Thanks for the quick updates, but I think it would be easier if you ran some sort of code checker. For example, flake8 reports the following on the plot_bmh.py script:

E302 expected 2 blank lines, found 1
E231 missing whitespace after ','
E231 missing whitespace after ','
E231 missing whitespace after ','
E231 missing whitespace after ','
E501 line too long (89 > 79 characters)
E231 missing whitespace after ','
E231 missing whitespace after ','
E231 missing whitespace after ','
E231 missing whitespace after ','

I know it seems a bit nitpicky, but many developers have code checkers connected to their editors that complain when there are PEP8 violations. BTW, I'm not a core developer, so I have no power to accept or reject this PR, so feel free to ignore me ;)

@tacaswell
Copy link
Member

+1 on what Tony said (except ignoring him)

@CamDavidsonPilon
Copy link
Contributor Author

hokay, I've pep'ed and flake'd both files

@CamDavidsonPilon
Copy link
Contributor Author

thoughts, @tacaswell?

@tonysyu
Copy link
Contributor

tonysyu commented Jul 11, 2014

👍

@tacaswell
Copy link
Member

I would like feed back from at least one more core dev and then it will get merged (most likely tomorrow).

@tacaswell
Copy link
Member

As a side note (that I did not notice before) is that you should always create a feature branch to work on, rather that making new commits on master. The reason is that when this is merged you will need to use a force push to make your master on gh track the upstream master again.

tacaswell added a commit that referenced this pull request Jul 12, 2014
Adding two new styles to mplstyles
@tacaswell tacaswell merged commit 44b3634 into matplotlib:master Jul 12, 2014
@CamDavidsonPilon
Copy link
Contributor Author

very true, thanks for informing me =)

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

Successfully merging this pull request may close these issues.

None yet

5 participants