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

Miscellaneous style fixes #1518

Merged
merged 11 commits into from Jun 21, 2017

Conversation

Projects
None yet
2 participants
@jlstevens
Copy link
Contributor

jlstevens commented Jun 5, 2017

This PR is for style changes and fixes that are bound to break the display tests. We could use this PR to update styles in general for 1.8.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

@jbednar @philippjfr Following on from our earlier discussion, I've come to a decision as to what I think we should do about colormaps: I would bundle a bunch of popular/useful colormaps in util/colormap.

My reasoning is as follows:

  • Although it is annoying to have to include data like this, holoviews is about to get smaller once the plotting backends are split out.
  • These common/popular colormaps do belong in core: these will be the colormaps that we want to work always regardless of active backend and what other plotting libs may or may not be installed.
  • Colormaps can be snapshotted as they are forever immutable once in use (changing existing colormaps would cause chaos!)
  • We can include our favorite colorcet maps (such as fire which we want as the default!) as well as popular colormaps used by matplotlib (e.g viridis). This subdirectory should include a README with a list of the included colormaps and where they came from. How big is the whole of colorcet? If it isn't too big I wouldn't be against including it in its entirety.
  • This approach will let us establish the colormap format that will have to be supported by all the plotting backends.
  • We can offer a tool to load these bundled colormaps that also includes a list of all the matplotlib colormap names. That way, if a non-bundled matplotlib colormap is requested, it can try to conditionally import from matplotlib and convert to the bundled format (and warn that matplotlib is not installed if unavailable). I think other than colorcet (colormaps done properly!) and matplotlib (longtime standard), there are no other appropriate/well-known sources of colormaps but this tool could use this approach for any other colormap source.

What do you think? I imagine a structure like this:

util/colormap
 |_ colorcet/
 |_ matplotlib/
  |_ bokeh/          # Does bokeh have any unique colormaps?
 |_ README.rst  # Lists what is included.
 |_ __init__py          # Includes utility to load colormaps

I think __init__.py could also include any of our existing cross-backend code relating to colormaps and palettes.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 5, 2017

We can include our favorite colorcet maps (such as fire which we want as the default!) as well as popular colormaps used by matplotlib (e.g viridis). This subdirectory should include a README with a list of the included colormaps and where they came from. How big is the whole of colorcet? If it isn't too big I wouldn't be against including it in its entirety.

Why not just require colorcet then? Seems like the better solution.

This approach will let us establish the colormap format that will have to be supported by all the plotting backends.

This approach will let us establish the colormap format that will have to be supported by all the plotting backends.

Not sure what that means to be honest, colormaps are generally just stored as a list of colors and all plotting backends I know of support this format.

That way, if a non-bundled matplotlib colormap is requested, it can try to conditionally import from matplotlib and convert to the bundled format (and warn that matplotlib is not installed if unavailable).

Again, why convert to some other format? Matplotlib accepts matplotlib colormaps obviously, and bokeh and plotly both accepts lists of colors which a matplotlib colormap can easily be converted to on the fly.

I think my preference is to require colorcet and consistently support the three new matplotlib colormaps which are also included in bokeh (viridis, magma and inferno).

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

Why not just require colorcet then? Seems like the better solution.

Fine by me. Included in core or in the plotting backends?

Not sure what that means to be honest, colormaps are generally just stored as a list of colors and all plotting backends I know of support this format.

Hex color strings, RGB tuples, RGBA tuples, Nx3 numpy array? There are lots of ways to list colors. If all the plotting backends you know all accept all of the possible formats, then I take your point though I suspect there are some ways of specifying colormaps accepted by some libraries but not others. Anyway, the plan doesn't hinge on this particular comment.

... and bokeh and plotly both accepts lists of colors which a matplotlib can easily be converted to on the fly.

Easily yes so why not put that in core in the same place where we ship the bundled colormaps?

... I think my preference is to require colorcet and consistently support the three new matplotlib colormaps which are also included in bokeh (viridis, magma and inferno).

I'm not sure that is enough though requiring colorcet does seem fine to me.

We want to remove the matplotlib dependency bokeh has wrt colormaps and we used to support any matplotlib colormap with bokeh. We should have some sort of warning if a matplotlib colormap is referenced without matplotlib installed.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 5, 2017

Fine by me. Included in core or in the plotting backends?

Probably the plotting backends I suppose, no strong opinion though.

Hex color strings, RGB tuples, RGBA tuples, Nx3 numpy array?

As far as I'm aware the first three are supported by both matplotlib and bokeh, Nx3 arrays don't seem like a sensible format to list colors as in a file.

Easily yes so why not put that in core in the same place where we ship the bundled colormaps?

Sure, color conversion utilities should live in the main holoviews repo.

I'm not sure that is enough though requiring colorcet does seem fine to me.

Sure, I'd be open to make a shortlist of matplotlib colormaps to include somewhere, not sure where to draw the line though.

We should have some sort of warning if a matplotlib colormap is referenced without matplotlib installed.

Definitely, it should warn and fall back to something else gracefully I suppose.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

I think we are mostly agreed now and just need to decide which matplotlib ones to include.

Definitely, it should warn and fall back to something else gracefully I suppose.

I'm not sure we can sensible fallback. We can at least generate a descriptive exception that explains why the colormap is not available (as opposed to just a matplotlib import error).

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 5, 2017

I think we are mostly agreed now and just need to decide which matplotlib ones to include.

Right, I'm not sure there's any we need in particular. The categorical ones (Set1-3 and other brewer palettes) are the ones I can think of atm but those are also included in bokeh in some form so maybe we can just make those behave the same by some other means.

I'm not sure we can sensible fallback. We can at least generate a descriptive exception that explains why the colormap is not available (as opposed to just a matplotlib import error).

I don't think a styling error should ever cause a hard exception if possible, styling is secondary to the data and as long as it warns sensibly you should get a plot regardless.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

I don't think a styling error should ever cause a hard exception if possible, styling is secondary to the data and as long as it warns sensibly you should get a plot regardless.

Sure. As long as you fallback to a very different colormap. If you fall back from fire to hot people might not notice and will end up interpreting their data wrong. I don't quite see how you would pick an appropriate fallback. Maybe if it is grayscale pick one with color and if it is color fallback to grayscale?

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 5, 2017

Sure. As long as you fallback to a very different colormap. If you fall back from fire to hot people might not notice and will end up interpreting their data wrong. I don't quite see how you would pick an appropriate fallback. Maybe if it is grayscale pick one with color and if it is color fallback to grayscale?

I don't think the fallbacks should be intelligent in any way, it should simply fall back to a default colormap that we have defined and know exists. If a user assumes they got the correct colormap despite a warning stating the opposite that's their fault really.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

I don't think the fallbacks should be intelligent in any way, it should simply fall back to a default colormap that we have defined and know exists.

If 'fire' becomes our default colormap then that would be it. That policy should mostly be ok though I guess that depends on how many similar looking colormaps (e.g 'hot') are not common between our plotting backends. As long as most of the common colormaps that look similar to 'fire' are always supported then there is less potential for confusion (even with a warning).

To summarize what we have decided:

  • We need to depend on colorcet for fire.
  • We need to offer a way to load colormaps in core HoloViews that issues a decent warnings and falls back to 'fire' from colorcet (new default).

If that is true and you are sure we really don't need to include any other matplotlib colormaps, then we don't really need a colormap subdirectory in util either. We just need a simple utility placed somewhere sensible.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

@philippjfr Added hv.config with a style_17 parameter. I think this PR would be a good place to test what things look like when switching to the matplotlib 2.0 styles.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 5, 2017

Just to make the link to the issue: this PR is helping address #823 .

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 15, 2017

The background color change is expected by PR #1537 - we want to remove this cell:

# HIDDEN CELL!
%output backend='matplotlib'
# New style should follow this by default
%opts Spikes [bgcolor='white']
%opts Curve [bgcolor='white']

@jlstevens jlstevens force-pushed the style_changes branch from a9d60d1 to a01fee3 Jun 15, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 15, 2017

Finally set the default colormap to 'fire'!

Also, issue #1006 might help inform any other style changes we want to make (e.g disable show_grid by default?)

@jlstevens jlstevens referenced this pull request Jun 15, 2017

Merged

Getting started guide #1537

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 20, 2017

@jbednar @philippjfr Getting this PR merged ASAP is now top priority. No point having gifs with outdated styles of gifs with new styles that users don't see!

Plan:

  • Use holoviews.rc set via HOLOVIEWSRC on travis to set style_17=True on Travis. Updating tests and switching to mpl2 on travis should be a second step.
  • Philipp recommends setting show_frame=False by default for Area, Bars, ErrorBars, Spread, Scatter, Spread.
  • Anything else we definitely agree to do right away?

I'll start trying to get the tests green on travis in this PR so we can merge these style fixes.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 20, 2017

Looks like I might have gotten travis working the way I want after on the first go editing travis.yaml! There is a first time for everything. :-)

I'll try the suggested show_frame change now. In #823 Jim also wrote:

> My first proposed bokeh style change: make errorbars thicker by default:
I agree. I also favor removing the grid lines by default, and making the default sizes comparable between the two backends.

Not sure about trying to match sizes between backends but should we try to set the errorbar line width and disabling grid lines?

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 20, 2017

and making the default sizes comparable between the two backends.

Not sure about this either, I deliberately made bokeh a bit larger, because axes, legends, colorbars can eat into the actual plot space and because interactive plots have to be slightly larger to be usable.

@jlstevens jlstevens force-pushed the style_changes branch from 250e5d3 to 3c2a469 Jun 20, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 20, 2017

@jbednar @philippjfr The style_17 flag name may be a bit of a lie as we probably updated display tests for various things since then. Got a better suggestion for a name?

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 20, 2017

@philippjfr I recommend merging once the tests are green so we can get the most obvious changes in. We can always add further changes in a subsequent PR once we decide about other details e.g gridlines and show_frame.

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 20, 2017

That said, a website build with the new style would be good to make sure we are happy with the current set of changes!

@jlstevens jlstevens force-pushed the style_changes branch from 3c2a469 to b7c5f3c Jun 20, 2017

@jlstevens jlstevens force-pushed the style_changes branch from b7c5f3c to 3deb3a8 Jun 20, 2017

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 20, 2017

Tests are going green which means style_17=True is working - or at least display output hasn't changed for this particular PR. I can add more style changes as long as they are quick and uncontroversial!

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 21, 2017

I'll be pushing a fix to the color cycle, restoring the short 5 color cycle we have been using. An issue about extending the color cycle has been filed (#1591).

@jlstevens

This comment has been minimized.

Copy link
Contributor Author

jlstevens commented Jun 21, 2017

@philippjfr I think these are enough changes that we should merge now and then use these in our rebased documentation PR.

@philippjfr

This comment has been minimized.

Copy link
Contributor

philippjfr commented Jun 21, 2017

Okay, the new styles look a whole lot better. I'll merge now and we can make tweaks later if we need to.

@philippjfr philippjfr merged commit 5560a50 into master Jun 21, 2017

4 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 79.812%
Details
s3-reference-data-cache Tests passing no test data changes required.
Details

@philippjfr philippjfr deleted the style_changes branch Jun 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.