Unify scale_info and scales #180
Conversation
This makes it possible to apply scale defaults after collapsing the
scale objects, instead of before. Also, this commit adds a distinction
between scale names (like "x", "y", "fill", "foo", "bar") and
visual properties that the scales can apply to ("x", "y", "fill", but
not "foo" and "bar"). Note that these properties are different from
props, which also can include "x2", "y2", etc.
| # for the property and name. If we get something like "blah", then use that | ||
| # for the name, but use prop_name for the property. | ||
| property <- prop_scale(prop, default_scale = propname_to_scale(prop_name)) | ||
| if (property %in% valid_scales) { |
There was a problem hiding this comment.
Having to use list of valid scale names seems a bit strange
There was a problem hiding this comment.
Yeah, that was probably the hackiest part of the whole pull request. It was necessary for these to work:
# Using custom scale name for a known property (fill)
df <- data.frame(x = 1:5, y = 5:1, z = runif(5))
df %>% ggvis(x = ~x, y = ~x, fill = prop(~z, scale = "blah")) %>%
layer_points()
# Use props like x2, y2
df %>% ggvis(x = ~x, y = ~x, x2 = ~x+1, y2 = ~x+1) %>%
layer_rects()There was a problem hiding this comment.
Can't you figure it out by inspect properties of the prop?
There was a problem hiding this comment.
Maybe... Here are some cases to consider:
# 1
ggvis(x = ~y)
# 2
ggvis(x2 = ~y)
# 3
ggvis(x = prop(~y, scale = "x"))
# 4
ggvis(x = prop(~y, scale = "blah"))
In each case the props will be something very roughly like this (stripped down for simplicity):
# 1
list(x = list(value = quote(y), scale = TRUE))
# 2
list(x2 = list(value = quote(y), scale = TRUE))
# 3
list(x = list(value = quote(y), scale = "x"))
# 4
list(x = list(value = quote(y), scale = "blah"))
In cases 1, 2, and 3, you want property="x", name="x", and label="y".
In case 4, you want property="x", name="blah", and label="y".
Also, this has made me realize that there's a bug in getting the labels.
Here are real, examples:
df <- data.frame(xvar = 1:5)
# 1
df %>% ggvis(x = ~xvar, y = 1) %>% layer_points()
# 2
df %>% ggvis(x = 1, x2 = ~xvar+0.5, y = ~xvar, y2 = 0.5) %>% layer_rects(fillOpacity := 0.2)
# 3
df %>% ggvis(x = prop(~xvar, scale = "x"), y = 1) %>% layer_points()
# 4
df %>% ggvis(x = prop(~xvar, scale = "blah"), y = 1) %>% layer_points() %>%
add_guide_axis("x", "blah")
|
It's not clear that this actually makes the code easier to understand - in particular, I don't like this: vis <- add_scale(vis, ggvis_scale("x", label = prop_name(cur_props(vis)$x.update)))because it implies we have two types of scale objects - complete scales, and partial scales. Could we at least extract that out into its own function, e.g.: vis <- set_scale_name(vis, "x", prop_name(cur_props(vis)$x.update))It's also not clear why you needed to pull the scales functions apart into three pieces (internal, external and defaults). That makes the code much harder to follow. |
|
There is a bit more complexity with the internal vs. external, but that's necessary because scale_info stuff is all gone now -- so that parallel data structure is gone. Instead of adding a scale_info object each time a mark is added, a scale object is added. But this requires the ability to add "sparse" scale objects, where a scale might only set a title, or set the domain, instead of setting all of the properties needed for a fully-fleshed-out, usable scale. The internal functions are for creating these sparse scale objects. When the plot is built, the scale objects for a particular scale (like fill, or x) get collapsed down into a single scale object. So they're collapsed, and then defaults are applied, in case the collapsed scale object is still missing some required fields. I agree that it makes sense to add a function for setting scale label, like |
|
How about only checking if properties are not null? |
Fixes #34.