Skip to content

Add a common example to compare style sheets #6476

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 13 commits into from
Oct 21, 2016

Conversation

afvincent
Copy link
Contributor

This new example aims at producing a reference to compare the different styles, like for example colormaps_reference.py does for the colormaps. A separate figure is plotted for each available style sheet, so this may result in a big number of figures.

The different subplots give an idea of the chosen style in various situations. There are taken from other examples in the style_sheets section, some of them slightly adapted. The sample data are "randomly" produced with a constant seed passed to the pseudo-random number generator instance.

@afvincent
Copy link
Contributor Author

Here are some examples of the figure, respectively with the grayscale, ggplot and dark_background style:
grayscale.pdf
ggplot.pdf
dark_background.pdf

@afvincent
Copy link
Contributor Author

Note to myself: it may be clever to add x- and y-label for at least one of the subplots.

@afvincent
Copy link
Contributor Author

Added some demo of the axes labels in the first subplot.
classic

Besides, I hope I didn't miss any of the PEP8 errors.

@afvincent
Copy link
Contributor Author

The PEP8 tests are now fine. However, since my last commit fe0abf7 that only corrected PEP8 typos, test_backend_pdf.test_grayscale_alpha is failing on Travis and I don't understand why. And about the other failure on AppVeyor, I don't even clearly understand where it fails…

@tacaswell tacaswell added this to the 2.0 (style change major release) milestone May 26, 2016
@afvincent
Copy link
Contributor Author

Apparently rcParams['axes.color_cycle'] is deprecated so I switched to rcParams['axes.prop_cycle'].by_key()['color'] to force the colors of the circle patches. However, I'm not sure I've totally understood how to use the cycler…

I also corrected a typo when determining max_idx.

"""
list_of_colors = plt.rcParams['axes.prop_cycle'].by_key()['color']
max_idx = min(nb_samples, len(list_of_colors))
for color in list_of_colors[0:max_idx]:
Copy link
Member

Choose a reason for hiding this comment

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

I think something like this would work (which granted exercises more than color, but that seems like a feature).

for sty, j in zip(plt.rcParams['axes.prop_cycle'], range(nb_samples)):
    ax.add_patch(Circle(prng.normal(scale=3, size=2),
                      radius=1.0, **sty))

Cyclers are iterables that yield dictionaries exactly for **ing into functions like this.

@tacaswell
Copy link
Member

See http://matplotlib.org/cycler/

cycler started life as a mpl PR that was deemed to have use outside of the library (which it does, we use it a couple of different places at my day job). For this exact use case it is a bit overkill, for multi-style (ex 'lw', 'ls', 'color', 'marker') it is pretty useful.

@tacaswell
Copy link
Member

also 👍 on this!

@afvincent
Copy link
Contributor Author

@tacaswell : thanks for the doc on cycler, I was unsuccessfully looking for this yesterday. I must have googled the wrong keywords… Furthermore, I didn't know that zip truncates the returned list of tuples to the size of the shortest argument, so indeed it will avoid bothering with max_idx.

However, in this specific example, I'd rather do something like

for sty_dict, j in zip(plt.rcParams['axes.prop_cycle'], range(nb_samples)):
    ax.add_patch(Circle(prng.normal(scale=3, size=2),
                        radius=1.0, color=sty_dict['color']))

rather than **ing. At least if one wants to demonstrate only the color cycle in the subplot with the circle patches. I am a bit afraid of style sheets that might add other properties to rcParams['axes.prop_cycle'] than just the color cycle : what about **ing kwargs that are not supported by patches.Circle (like marker)?

def plot_scatter(ax, prng, nb_samples=200):
""" Scatter plot.

NB: `plt.scatter` doesn't use default colors.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is true any more.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, #6291

@afvincent
Copy link
Contributor Author

@QuLogic : thank you for all the comments and remarks! I did the suggested modifications.

@tacaswell : normally, rcParams['axes.color_cycle] is not used anywhere, replaced by all the new way of selecting colors ("Cn", rcParams['axes.prop_cycle]). I also changed plot_colored_circles with what I suggested earlier, rather than exactly what you proposed (about using the cycler). But if you think your proposition is better, no problem I'll modify the function accordingly.

From the plotting point of view, I added a 2nd cloud of points in the scatter subplot: (i) to convince myself that scatter was indeed using the current color cycle, (ii) because it adds some insight on how each style helps to discriminate different clusters of points. For this 2nd point, see for example

classic

where the green cluster is a bit hard to distinguish from the blue one, while with this other style

dark_background

the two clusters appear much more clearly.

@WeatherGod
Copy link
Member

The subplot spacing still needs to be adjusted. Don't know if that should be in the script, or elsewhere, though.

@@ -25,8 +24,8 @@ def plot_colored_sinusoidal_lines(ax):
"""
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wouldn't “Plot sinusoidal lines with colors following the style color cycle.” be more correct?

@afvincent
Copy link
Contributor Author

A grep tells me that (currently) the fivethirtyeight and the classic styles tweak some of the rcParams related to figure.subplot.*. If I impose some of these rcParams in the script (and try to find some values that look nice on all the currently available styles), it will affect the behavior of these styles, won't it? Is it what we want? For the moment, the script avoids to overwride anything that may be tuned by a style, in order for the comparison betweeen the different styles to be the fairest possible.

@afvincent afvincent self-assigned this Jul 16, 2016
@afvincent
Copy link
Contributor Author

Anyway, I've just seen some typos + that the script is using plot instead of scatter to do a scatter plot, so I will try to update the script tomorrow.

@jenshnielsen
Copy link
Member

@afvincent What is the status on this one? Looks like something that would be nice to land for 2.0

@afvincent
Copy link
Contributor Author

@WeatherGod : the script now uses tight_layout, which fixes most of the subplot spacing issues. The only remaining one I noticed is the suptitle being sometimes (just) a bit too close from the top center subplot.

@jenshnielsen : there is still one issue that I am struggling with. I would llike to use scatter instead of plot in the function plot_scatter (which seems more logical to me). Unfortunately, when I replace the current line (l. 17) ax.plot(x, y, ls='none', marker=marker) with ax.scatter(x, y, marker=marker), I get the following traceback :

In [2]: %tb
---------------------------------------------------------------------------
ValueError                                Traceback (most recent call last)
/home/adrien/matplotlib/lib/matplotlib/backends/backend_agg.pyc in draw(self)
    462 
    463         try:
--> 464             self.figure.draw(self.renderer)
    465         finally:
    466             RendererAgg.lock.release()

/home/adrien/matplotlib/lib/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
     66     def draw_wrapper(artist, renderer, *args, **kwargs):
     67         with with_rasterized(artist, renderer):
---> 68             return draw(artist, renderer, *args, **kwargs)
     69 
     70     draw_wrapper._supports_rasterization = True

/home/adrien/matplotlib/lib/matplotlib/figure.pyc in draw(self, renderer)
   1260 
   1261         mimage._draw_list_compositing_images(
-> 1262             renderer, self, dsu, self.suppressComposite)
   1263 
   1264         renderer.close_group('figure')

/home/adrien/matplotlib/lib/matplotlib/image.pyc in _draw_list_compositing_images(renderer, parent, dsu, suppress_composite)
    137     if not_composite or not has_images:
    138         for zorder, a in dsu:
--> 139             a.draw(renderer)
    140     else:
    141         # Composite any adjacent images together

/home/adrien/matplotlib/lib/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
     66     def draw_wrapper(artist, renderer, *args, **kwargs):
     67         with with_rasterized(artist, renderer):
---> 68             return draw(artist, renderer, *args, **kwargs)
     69 
     70     draw_wrapper._supports_rasterization = True

/home/adrien/matplotlib/lib/matplotlib/axes/_base.pyc in draw(self, renderer, inframe)
   2377             renderer.stop_rasterizing()
   2378 
-> 2379         mimage._draw_list_compositing_images(renderer, self, dsu)
   2380 
   2381         renderer.close_group('axes')

/home/adrien/matplotlib/lib/matplotlib/image.pyc in _draw_list_compositing_images(renderer, parent, dsu, suppress_composite)
    137     if not_composite or not has_images:
    138         for zorder, a in dsu:
--> 139             a.draw(renderer)
    140     else:
    141         # Composite any adjacent images together

/home/adrien/matplotlib/lib/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
     66     def draw_wrapper(artist, renderer, *args, **kwargs):
     67         with with_rasterized(artist, renderer):
---> 68             return draw(artist, renderer, *args, **kwargs)
     69 
     70     draw_wrapper._supports_rasterization = True

/home/adrien/matplotlib/lib/matplotlib/collections.pyc in draw(self, renderer)
    819     def draw(self, renderer):
    820         self.set_sizes(self._sizes, self.figure.dpi)
--> 821         Collection.draw(self, renderer)
    822 
    823 

/home/adrien/matplotlib/lib/matplotlib/artist.pyc in draw_wrapper(artist, renderer, *args, **kwargs)
     66     def draw_wrapper(artist, renderer, *args, **kwargs):
     67         with with_rasterized(artist, renderer):
---> 68             return draw(artist, renderer, *args, **kwargs)
     69 
     70     draw_wrapper._supports_rasterization = True

/home/adrien/matplotlib/lib/matplotlib/collections.pyc in draw(self, renderer)
    312 
    313         if do_single_path_optimization:
--> 314             gc.set_foreground(tuple(edgecolors[0]))
    315             gc.set_linewidth(self._linewidths[0])
    316             gc.set_linestyle(self._linestyles[0])

/home/adrien/matplotlib/lib/matplotlib/backend_bases.pyc in set_foreground(self, fg, isRGBA)
   1027             self._rgb = fg
   1028         else:
-> 1029             self._rgb = colors.to_rgba(fg)
   1030 
   1031     def set_graylevel(self, frac):

/home/adrien/matplotlib/lib/matplotlib/colors.pyc in to_rgba(c, alpha)
    137         rgba = _colors_full_map.cache[c, alpha]
    138     except (KeyError, TypeError):  # Not in cache, or unhashable.
--> 139         rgba = _to_rgba_no_colorcycle(c, alpha)
    140         try:
    141             _colors_full_map.cache[c, alpha] = rgba

/home/adrien/matplotlib/lib/matplotlib/colors.pyc in _to_rgba_no_colorcycle(c, alpha)
    196         c = c[:3] + (alpha,)
    197     if any(elem < 0 or elem > 1 for elem in c):
--> 198         raise ValueError("RGBA values should be within 0-1 range")
    199     return c
    200 

ValueError: RGBA values should be within 0-1 range

It is a bit weird as it seems to occur only with seaborn-colorblind and seaborn-dark-palette. I rapidly had a look to the 2 style sheets : the color declarations seem to be correct hexadecimal code. So my current workaround is to rely on plot with 'none' line style instead of using scatter

PS : there are many more styles now than when I wrote this script the first time. The number of figures generated will be rather big : should we worry about that?

@afvincent
Copy link
Contributor Author

afvincent commented Sep 5, 2016

My last commit (610ce04) reverts some of my previous changes in the docstrings. I don't know why I was sure the docstrings were supposed to be written """Does something…""" rather than """Do something…""", but reading again PEP 257 and Numpy documentation guide style, I realized I had remembered them totally backwards… Sorry for the noise!

@story645
Copy link
Member

story645 commented Sep 5, 2016

I'm wondering if how it's currently laid out, it might end up being a little difficult for people to compare...What about laying it out in a table, as such

stylesheet name  (possibly rotated) | line plot+annotation | scatter plot | heat map| bar

And yes, I kinda think you can safely reduce the number of plots if broad style changes are what you're going for.

@afvincent
Copy link
Contributor Author

@story645 I have just experimented a bit with your suggestion about a tabular layout : one issue that arises is the figure-related rcParams (in particular the face color for style with dark background). I will try to make a draft with one figure per face color value today.

@afvincent
Copy link
Contributor Author

Sorry, this PR had fallen below my radar…

I switched to a single row of demo plots as @story645 suggested, and indeed it looks nice (though I have kept all the previous demo plots). Actually, I have decided to stick to one figure per style, instead of one figure per figure face color: it makes the code simpler to read, and some style share the same face color but differ on other figure parameters (so this approach was not totally consistent).

Here are some examples showing how it currently looks like:
classic_row
grayscale_row
seaborn-paper_row

Is it a problem if I try to rebase the branch of this PR? It seems like I started working on this before top and right ticks were set to not visible by default… Beside some auto ticks are a bit weird but they may have been enhanced since last May.

@NelleV Is the script new docstring OK for you :)?

@NelleV
Copy link
Member

NelleV commented Oct 16, 2016

FYI, I rebase the code of my branches all the time, so don't hesitate :)

@afvincent afvincent force-pushed the common_example_for_style_sheets branch from 187ce10 to cea7f28 Compare October 16, 2016 16:11
@afvincent
Copy link
Contributor Author

Simple rebase. (Unfortunately I might have missed to pass some option to have the possibility of squashing some minor commits.) As expected, the ticks behave better with the current master. Here is now the kind of comparison that can be done with this new example script:
classic_row
bmh_row
ggplot_row
grayscale_row
seaborn_row
seaborn-paper_row

One last thing I am wondering about the gallery behavior: the figures are displayed in the order they are created, aren't they? If that the case, maybe I should use sorted(plt.style.available) instead of simply plt.style.available, at least to group all the seaborn-related styles together?

@story645
Copy link
Member

Agree that like styles should be grouped together, but I think classic should always be on top. Also 😄 about the new layout.

@afvincent
Copy link
Contributor Author

afvincent commented Oct 16, 2016

Oops: the complete commit message for 2af4445 should be "Fix default value of the named parameter in 'plot_figure'". As I am not sure how ugly it is to amend a pushed commit, I let it as it is for the moment: if it was not something to do, just tell me and I will look how to take care of it through git. Anyway, this commit is pretty minor: I just realized that the keyword argument default value was not so clever anymore, as the style context manager is now outside plot_figure

More importantly, with commit c2347d1, styles are now used more or less in alphabetical order: only default and classic are given specific roles (= the 2 first positions). NB: I have added default eventhough it is not in plt.style.available, because I find it rather useful to know what the defaults look like compared to the other styles.

I am still opened to remarks and suggestions, in case this example needs some additional polishing step.

Edit: typo, plt.style.defaultplt.style.available

Style sheets reference
======================

This scripts demonstrates the different available style sheets on a
Copy link
Member

@NelleV NelleV Oct 17, 2016

Choose a reason for hiding this comment

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

There is a typo: scripts -> script

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed: fixed in 97224f6.

Copy link
Member

@NelleV NelleV left a comment

Choose a reason for hiding this comment

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

Thanks!

@NelleV NelleV changed the title Add a common example to compare style sheets [MRG+1] Add a common example to compare style sheets Oct 18, 2016
@afvincent
Copy link
Contributor Author

I am not very good at reading CI failure reports but I think the Appveyor one is of the usual “502 gateway”-type, while the Travis one:

Warning, treated as error:
/home/travis/build/matplotlib/matplotlib/doc/thirdpartypackages/index.rst:20: WARNING: duplicate label hl_plotting, other instance in /home/travis/build/matplotlib/matplotlib/doc/mpl_toolkits/index.rst

is unrelated to this PR and was fixed by #7304 .

@NelleV
Copy link
Member

NelleV commented Oct 19, 2016

While I am more than happy to merge with an appveyor failure, I am less happy about the travis error. Do you mind rebasing?

@afvincent afvincent force-pushed the common_example_for_style_sheets branch from 97224f6 to 09e1f3a Compare October 19, 2016 22:17
@afvincent
Copy link
Contributor Author

afvincent commented Oct 19, 2016

@NelleV I've just rebased my branch. Let's see if Travis is still complaining.

Edit: Travis seems happy now :).

@NelleV NelleV merged commit 7bce844 into matplotlib:master Oct 21, 2016
NelleV added a commit that referenced this pull request Oct 21, 2016
Add a common example to compare style sheets
@QuLogic
Copy link
Member

QuLogic commented Oct 22, 2016

Backported to v2.x via 992b5ee.

@QuLogic QuLogic changed the title [MRG+1] Add a common example to compare style sheets Add a common example to compare style sheets Oct 22, 2016
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.

8 participants