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

Added support for color libraries #5

Closed
wants to merge 13 commits into from

Conversation

mkborregaard
Copy link
Member

This adds support to color libraries. To use this in Plots would require some changes in Plots.jl

@mkborregaard
Copy link
Member Author

This addresses this issue: #4
I have refactored the color gradient support slightly to use color collections or libraries to match color gradient names. This allows PlotUtils to support many more color collections out of the box without namespace clashes, making the color capabilities in PlotUtils versatile enough to be a standalone color gradient support.

To change as little of the existing code as possible, I have chosen an approach where the global _gradients is changed to a flag signalling the current color library. Using a new color library is as simple as switching that flag with set_color_library!(color_library::Symbol).
I believe it should be possible by Plots to use a color_library keyword argument by simply calling set_color_library at the beginning of doing the plot, then restoring the library after plotting.

I have also added the colorbrewer (http://colorbrewer2.org) palettes which are iconic in cartography (they are the de facto standard for maps but are used for many other purposes as well). I do not depend on Tim Renner's and Tim Holy's ColorBrewer.jl package, in that it simply wraps the colorbrewer palettes in a format useful for Gadfly. Instead I downloaded the palettes directly from the source.

Note that this may be slightly breaking, in that I have put the cmocean colors in the :cmocean library. Thus any code that uses these colors from the last 22 days would throw an error. This is an obvious break, is unlikely to affect package code, and solvable by adding a single command to scripts.

@mkborregaard
Copy link
Member Author

mkborregaard commented Feb 21, 2017

I have worked on this now for some time, and I think it is ready, as far as I can tell. I have had to rebase a few times so it would be nice to have it merged, if the verdict is positive.

As I write above, this adds support for color libraries in Plots. This allows potentially expanding the number of color gradients and the control with them. If this gets merged I will update the Plots docs with a description.

This provides access to helper functions:

using Plots
clibraries()
  5-element Array{Symbol,1}:
   :Plots_internal
   :matplotlib    
   :cmocean       
   :colorbrewer   
   :default    # this is added by PlotThemes, the default is currently set to :matplotlib

cgradients()
  4-element Array{Symbol,1}:
   :inferno
   :plasma 
   :viridis
   :magma  

 cgradients(:cmocean)
  18-element Array{Symbol,1}:
   :phase  
   :speed  
   .
   .
   :thermal
   :delta  

You can get any color gradient by cgrad(:RdBu, color_library = :colorbrewer), but the recommended way to use it is

set_clibrary(:colorbrewer)
histogram2d(randn(1000), randn(1000), c = :Spectral_r)
#or
histogram2d(randn(1000), randn(1000), c = :divergent)

skaermbillede 2017-02-21 kl 15 48 30

All libraries have defined a default gradient, and can also define, e.g. a default sequential or default divergent gradient.
The arrangement into color libraries is contentious of course - I have put the matplotlib as the defaults and moved those that aren't tailored for scientific color reproduction out in Plots_internal.

There are some breaking elements in this: the cmocean is moved out as a separate library to reduce clutter, and the Plots_internal have been moved out of default, - though Plots could take those and merge them in a default color library as well.

Also, PlotThemes should use the functionality here, so it would require some changes. @pkofod would you review? Given that @tbreloff is really busy these days and this is an important change it would be good if you could take some time to review this.

@tbreloff
Copy link
Member

I really like the idea of color libraries for organization and exploration purposes, but I think it's a mistake to require it for actually selecting a gradient/cmap. A better approach would be to search through all libraries for the gradient name, and throw a warning if there are more than one for a given cgrad name.

A solution might be to just store a reverse mapping of cgrad --> library or something like that.

tl;dr One should be able to choose a gradient without knowing that color libraries exist.

@mkborregaard
Copy link
Member Author

Let me be sure I understand what you mean:
With this PR, it is still possible to do plot(x, c = :inferno) without thinking about colorlibraries (because matplotlib is default). But are you requesting that plot(x, c = :algae) be possible too, without changing the library to :cmocean?

@tbreloff
Copy link
Member

But are you requesting that plot(x, c = :algae) be possible too, without changing the library to :cmocean?

Yes. Definitely. 💯 🎉 👍 🥇

@pkofod
Copy link

pkofod commented Feb 21, 2017

I will review, but I am battling contractors who didn't do their job right, so my time is a bit limited as well.

@tbreloff
Copy link
Member

@pkofod Sounds intense! Good hunting. (so say we all)

@mkborregaard
Copy link
Member Author

mkborregaard commented Feb 21, 2017

Thanks @pkofod ! Yes I know what you mean. I have to take 3 months break from Julia due to two huge deadlines coming up with my work (plus I need to do a little fieldwork in the mountains of Tanzania). So I am trying to divest myself from all my open things this week :-)

@pkofod
Copy link

pkofod commented Feb 21, 2017

Thanks @pkofod ! Yes I know what you mean. I have to take 3 months break from Julia due to two huge deadlines coming up with my work (plus I need to do a little fieldwork in the mountains of Tanzania). So I am trying to divest myself from all my open things this week :-)

Oh, okay. I will try to get back to this asap.

@mkborregaard
Copy link
Member Author

I didn't mean to stress you, just talking about my personal priorities. I will take the time to go through your response carefully whenever you are able to find the time to do it.

@mkborregaard
Copy link
Member Author

mkborregaard commented Feb 21, 2017

OK, the newest version allows heatmap(x, c = :algae) without specifying the :cmocean library. I am not sure this doesn't defeat the purpose of the PR a little bit, so that this is just @tbreloff 's very polite way of saying he disagrees with the direction :-) It also means that it won't be breaking AFAICT (PlotThemes probably still needs updating though), so that is a good thing.
On the other hand, it still does deal with namespace clashes (multiple gradients with the same name raise warnings), and it should make it easier to add new color libraries. A package could also depend on PlotUtils (if they don't mind doing that) and use the tools to generate its own color library and setting it as the default.
Perhaps the more advanced use of defaults is really the most useful element.

@tbreloff
Copy link
Member

this is just @tbreloff 's very polite way of saying he disagrees with the direction :-)

😏

Perhaps the more advanced use of defaults is really the most useful element.

Personally I think a programmatic way of exploring the options is great.

Side note: it would be great to have a Plots method to "plot a color library" like: #2 (comment)

@tbreloff
Copy link
Member

Also, it seems like you're updating code that is outside the scope of this PR, likely due to bad git rebase/merge. That needs to be fixed before any merge.

@mkborregaard
Copy link
Member Author

Yes, I have really had trouble with git for this one :-( Can you point to the code you are thinking of?

@tbreloff
Copy link
Member

For example, you appear to be adding adapted_grid

@mkborregaard
Copy link
Member Author

That is really strange. I remember the PR had gotten stale, so I rebased by merging (or believe I merged) JuliaPlots/master into my branch – adapted_grid came with it, so I just thought that had been added to master in the meantime. I guess that code should not be there, then.

@mkborregaard
Copy link
Member Author

It is this commit: mkborregaard@fefcbf3 I seem to not be able to revert it.

@tbreloff
Copy link
Member

Not sure exactly what happened, but I wonder if it makes sense to re-branch from the current master, and copy/paste from this PR's diff? Then you can get exactly what you changed and not all the conflicts.

@mkborregaard
Copy link
Member Author

git_2x

@mkborregaard
Copy link
Member Author

#7

@mkborregaard
Copy link
Member Author

Closing in favour of #7

@mkborregaard mkborregaard deleted the color_libraries branch March 2, 2017 21:51
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.

3 participants