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

RFC: Color schemes #908

Merged
merged 11 commits into from
Oct 6, 2016
Merged

RFC: Color schemes #908

merged 11 commits into from
Oct 6, 2016

Conversation

shashi
Copy link
Collaborator

@shashi shashi commented Oct 4, 2016

This adds the ability to

  1. Set color scales in Theme (discrete_color_scale, continuous_color_scale)
  2. Ability to set a theme with set_theme(::Theme), or with set_theme(theme_name::Symbol) given get_theme(::Type{Val{theme_name}}) returns a Theme
  3. A style function which will modify the current theme instead of creating a new one with the default default values.
  4. Set theme using GADFLY_THEME env variable

and adds a dark theme:

image

image

image

image

image

image

And is intended for use in Atom

image

@pkofod
Copy link

pkofod commented Oct 4, 2016

Is this a theme you cooked up yourself, or is it the atom color scheme, or?

@tlnagy
Copy link
Member

tlnagy commented Oct 4, 2016

🎉🎉🎉🎉

I guess the main things remaining are to update the docs. I think the most important files are docs/src/man/themes.md and docs/src/tutorial.md. Great work.

Copy link
Member

@tlnagy tlnagy left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple minor nitpicks

try
set_theme(Symbol(theme))
catch err
warn("Error loading Gadlfy theme $theme (set by GADFLY_THEME env variable)")
Copy link
Member

Choose a reason for hiding this comment

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

Gadfly misspelled

:shape => Scale.shape_discrete(),
:group => Scale.group_discrete(),
:label => Scale.label()))
const default_aes_scales = Dict{Symbol, Dict}(
Copy link
Member

Choose a reason for hiding this comment

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

Is the @compat not necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not anymore since we depend on Julia 0.4+

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good point. I forgot we dropped 0.3

end
b
function $(name)($(inherit_parameters_expr), b::$name)
$(new_with_defaults)
end
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@tlnagy
Copy link
Member

tlnagy commented Oct 5, 2016

Also, it looks like Travis is failing due to things unrelated to this PR. I'm looking at you RData 🙄. It looks like this is most likely FileIO's fault: JuliaIO/FileIO.jl#85

@shashi
Copy link
Collaborator Author

shashi commented Oct 5, 2016

@pkofod I kinda made it up...

@tlnagy I've made more changes, now it we have a theme_stack where you can push and pop themes from as you call plot to apply different themes. See the documentation... I have some questions about it, I will be making some inline comments.

@@ -1492,7 +1492,7 @@ const contour = ContourStatistic
function default_scales(::ContourStatistic)
return [Gadfly.Scale.z_func(), Gadfly.Scale.x_continuous(),
Gadfly.Scale.y_continuous(),
Gadfly.Scale.color_continuous_gradient()]
Gadfly.get_scale(:numerical, :color)]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure why the other Scales here also should not come from Gadfly.get_scale, doing that only for the color scales might seem a little odd.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, shouldn't these all be get_scale calls?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll leave the others unchanged for now, because its seems this will in fact be faster than a dict lookup. We should consider this if we are doing a larger simplification of these things.

plot(x=rand(10), y=rand(10),
style(major_label_font="CMU Serif",minor_label_font="CMU Serif",
major_label_font_size=16pt,minor_label_font_size=14pt))
end
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How do I hide the output of this? (This is the same as the previous one)

Copy link
Member

Choose a reason for hiding this comment

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

As in, you don't want it to plot? You can append # hide (spaces are important) to the end of any line you don't want displayed. If you have multiple lines then a @setup block might be useful. See https://juliadocs.github.io/Documenter.jl/latest/man/syntax.html#@example-block-1

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I want the code to appear, but not the output...

Copy link
Member

Choose a reason for hiding this comment

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

ah, there are two different ways of doing it. It depends whether you want the code to be displayed and get executed or just displayed.

To display and execute:

```@example 1
plot(...)
nothing # hide
```

just display, no execution

```julia
code
```

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, thanks. I think 1st one will also test that it works, so will use that.

style(major_label_font="CMU Serif",minor_label_font="CMU Serif",
major_label_font_size=16pt,minor_label_font_size=14pt)) do

plot(x=rand(10), y=rand(10))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need to hide this as well.

@ViralBShah
Copy link
Collaborator

That color scheme will look really awesome with Juno!

Copy link
Member

@tlnagy tlnagy left a comment

Choose a reason for hiding this comment

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

👍

plot(x=rand(10), y=rand(10),
style(major_label_font="CMU Serif",minor_label_font="CMU Serif",
major_label_font_size=16pt,minor_label_font_size=14pt))
end
Copy link
Member

Choose a reason for hiding this comment

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

As in, you don't want it to plot? You can append # hide (spaces are important) to the end of any line you don't want displayed. If you have multiple lines then a @setup block might be useful. See https://juliadocs.github.io/Documenter.jl/latest/man/syntax.html#@example-block-1

@tlnagy
Copy link
Member

tlnagy commented Oct 5, 2016

I think this PR is getting close. Main thing is to get JuliaIO/FileIO.jl#85 fixed because I want CI tests passing before merging.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 65.405% when pulling faf7846 on shashi:colorscheme into 22d4659 on dcjones:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 65.405% when pulling faf7846 on shashi:colorscheme into 22d4659 on dcjones:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 65.405% when pulling faf7846 on shashi:colorscheme into 22d4659 on dcjones:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 65.405% when pulling faf7846 on shashi:colorscheme into 22d4659 on dcjones:master.

@tlnagy
Copy link
Member

tlnagy commented Oct 5, 2016

The tests look good! (except for on 0.4, but that's due to Media/Juno having issues)

@coveralls
Copy link

coveralls commented Oct 6, 2016

Coverage Status

Coverage increased (+0.06%) to 65.897% when pulling 7e3da5f on shashi:colorscheme into 22d4659 on dcjones:master.

@shashi
Copy link
Collaborator Author

shashi commented Oct 6, 2016

Okay, added a couple of tests. Added a bit more documentation. I did a Documenter generation and everything is great. Merging.

@shashi shashi merged commit 8dda492 into GiovineItalia:master Oct 6, 2016
@shashi shashi deleted the colorscheme branch October 6, 2016 15:08
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