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

Scale.alpha_continuous & Scale.alpha_discrete #1257

Merged
merged 1 commit into from
Mar 5, 2019

Conversation

Mattriks
Copy link
Member

@Mattriks Mattriks commented Mar 3, 2019

  • I've updated the documentation to reflect these changes
  • I've added an entry to NEWS.md
  • I've added and/or updated the unit tests
  • I've run the regression tests
  • I've built the docs and confirmed these changes don't cause new errors

Same as #1252, but now with alpha info in the docstrings for Geom.point, Geom.polygon and Geom.ribbon

@codecov-io
Copy link

codecov-io commented Mar 3, 2019

Codecov Report

Merging #1257 into master will decrease coverage by 0.24%.
The diff coverage is 95.83%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1257      +/-   ##
=========================================
- Coverage   85.74%   85.5%   -0.25%     
=========================================
  Files          35      35              
  Lines        4119    4139      +20     
=========================================
+ Hits         3532    3539       +7     
- Misses        587     600      +13
Impacted Files Coverage Δ
src/data.jl 0% <ø> (ø) ⬆️
src/aesthetics.jl 77.96% <ø> (ø) ⬆️
src/Gadfly.jl 74.81% <ø> (-0.49%) ⬇️
src/theme.jl 65.51% <0%> (-4.66%) ⬇️
src/geom/ribbon.jl 100% <100%> (ø) ⬆️
src/geom/point.jl 100% <100%> (ø) ⬆️
src/scale.jl 94.56% <100%> (+0.49%) ⬆️
src/geom/polygon.jl 92.5% <100%> (+1.07%) ⬆️
src/guide/keys.jl 91.13% <100%> (ø) ⬆️
src/guide.jl 84.39% <100%> (ø) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 82922a3...99f54e4. Read the comment docs.

Copy link
Member

@bjarthur bjarthur left a comment

Choose a reason for hiding this comment

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

looks great! thanks for fixing the git problems. just a few comments.

src/scale.jl Outdated
@@ -313,7 +325,7 @@ anything in the data that's not respresented in `levels` will be set to
default order.

See also [`group_discrete`](@ref), [`shape_discrete`](@ref),
[`size_discrete`](@ref), and [`linestyle_discrete`](@ref).
[`size_discrete`](@ref), and [`linestyle_discrete`](@ref), and [`alpha_discrete`](@ref).
Copy link
Member

Choose a reason for hiding this comment

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

the "and" before linestyle_discrete could be removed as it was in #1252

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

for (k,g) in enumerate(ug)
i = groups.==[g]
polys[k] = polygon_points(aes_x[i], aes_y[i], geom.preserve_order)
colors[k] = first(aes_color[i])
line_styles[k] = mod1(first(aes_linestyle[i]), linestyle_palette_length)
line_styles[k] = mod1(first(aes_linestyle[i]), linestyle_palette_length)
alphas[k] = first(alpha_discrete ? theme.alphas[aes_alpha[i]] : aes_alpha[i])
Copy link
Member

Choose a reason for hiding this comment

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

what git weirdness is causing the line_style[k] line to be highlight as a change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Weird - no idea.

@@ -10,41 +10,42 @@ RibbonGeometry(default_statistic=Gadfly.Stat.identity(); fill=true, tag=empty_ta
Geom.ribbon

Draw a ribbon at the positions in `x` bounded above and below by `ymax` and
`ymin`, respectively. Optionally draw multiple ribbons by grouping with `color`.
`ymin`, respectively. Optionally draw multiple ribbons by grouping with `color` and `alpha` or `linestyle`.
Copy link
Member

Choose a reason for hiding this comment

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

what does "color and alpha or linestyle" mean? the boolean logic is unclear

Copy link
Member Author

Choose a reason for hiding this comment

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

I've clarified this. It differs whether the ribbon is filled or not filled:

geom.fill ? compose!(ctx, Compose.polygon(polys, geom.tag), fill(colors), fillopacity(alphas)) :
compose!(ctx, Compose.line(lines, geom.tag), fill(nothing), stroke(colors), strokedash(linestyles))

Theme(default_color="green", discrete_highlight_color=c->"gray",
point_size=2mm, alphas=[0.0,0.2,0.4,0.6,0.8,1.0])
)
hstack(p1,p2)
Copy link
Member

Choose a reason for hiding this comment

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

both of these tests are near identical to the docs, the only difference being the hard-coded random numbers. seems redundant. ideally i like to see real data, say from RDatasets, used in the docs that is particularly pretty, and very simple made up cases for the tests with focus on execution speed (we time out sometimes) and code coverage. this is a bit of a nit-pick, but can you at least make the docs and the tests less redundant?

Copy link
Member Author

Choose a reason for hiding this comment

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

I like this example because it illustrates e.g. "color aesthetic, fixed alpha" and "alpha aesthetic, fixed color". If I used an example from RDatasets to illustrate the alpha aesthetic, I would use something like the Diamonds dataset (53940 points). In my next PR I'm going to halve the time it takes to plot that: Geom.point currently suffers from using a new context for every point.

@bjarthur bjarthur merged commit cfc5c85 into GiovineItalia:master Mar 5, 2019
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.

svgjs, hstack and opacity opacity in the color scale
3 participants