Skip to content

coord_trans/polar() ignore position guides #3959

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

Closed
teunbrand opened this issue Apr 22, 2020 · 4 comments · Fixed by #5334
Closed

coord_trans/polar() ignore position guides #3959

teunbrand opened this issue Apr 22, 2020 · 4 comments · Fixed by #5334

Comments

@teunbrand
Copy link
Collaborator

teunbrand commented Apr 22, 2020

While I was trying to making a variant of guide_axis() that displays tick marks at minor breaks (see for example SO), I stumbled on a possible bug in coord_trans() and coord_polar(), which appear to ignore position guides.

Standard cartesian coordinates seem to work just fine.

library(ggplot2)

p <- ggplot(mpg, aes(hwy, cty)) +
  geom_point() +
  guides(x = guide_axis("custom axis"))

p + coord_cartesian()

Polar coordinates don't render the correct axis title.

p + coord_polar()

Transformed coordinates also don't render the correct axis title.

p + coord_trans()

Created on 2020-04-22 by the reprex package (v0.3.0)

The same pattern holds if I specify scale_x_continuous(guide = guide_axis("custom axis)) or use guides(x.sec = guide_axis("custom axis")).

@yutannihilation
Copy link
Member

Thanks, confirmed.

Currently, the title of guide is referenced only by CoordCartesian$label() via panel_guide_label():

labels = function(self, labels, panel_params) {
positions_x <- c("top", "bottom")
positions_y <- c("left", "right")
list(
x = lapply(c(1, 2), function(i) {
panel_guide_label(
panel_params$guides,
position = positions_x[[i]],
default_label = labels$x[[i]]
)
}),
y = lapply(c(1, 2), function(i) {
panel_guide_label(
panel_params$guides,
position = positions_y[[i]],
default_label = labels$y[[i]])
})
)
},

panel_guide_label <- function(guides, position, default_label) {
guide <- guide_for_position(guides, position) %||% guide_none(title = NULL)
guide$title %|W|% default_label
}

The other Coord classes use the inherited Coord$label():

labels = function(labels, panel_params) labels,

But, it seems we cannot simply transplant the label(). panel_params doesn't contain guides by default, so we need setup_panel_guides() to add it.

panel_params$guides <- guides

Besides, guides needs to have position, otherwise guide_for_position(guides, position) returns NULL.

scale <- panel_params[[aesthetic]]
# position could be NULL here for an empty scale
guide$position <- guide$position %|W|% scale$position

To enable this, we need to include scale in panel_params, which is currently done only in CoordCartesian:

setup_panel_params = function(self, scale_x, scale_y, params = list()) {
c(
view_scales_from_scale(scale_x, self$limits$x, self$expand),
view_scales_from_scale(scale_y, self$limits$y, self$expand)
)
},

e.g. CoordTrans doesn't include ViewScale.

ggplot2/R/coord-transform.r

Lines 138 to 143 in d3d47be

setup_panel_params = function(self, scale_x, scale_y, params = list()) {
c(
train_trans(scale_x, self$limits$x, self$trans$x, "x", self$expand),
train_trans(scale_y, self$limits$y, self$trans$y, "y", self$expand)
)
},

ggplot2/R/coord-transform.r

Lines 217 to 227 in d3d47be

out <- list(
range = out$range,
labels = out$labels,
major = out$major_source,
minor = out$minor_source,
sec.labels = out$sec.labels,
sec.major = out$sec.major_source,
sec.minor = out$sec.minor_source
)
names(out) <- paste(name, names(out), sep = ".")
out

@paleolimbot
I didn't actively follow the discussion about ViewScale (sorry...), do you think we can support position guides on other coords than CoordCartesian?

@paleolimbot
Copy link
Member

No prob! The ViewScale exists because of the SecAxis...both get a ViewScale even though there's only one Scale. I'd prefer the ViewScale not to exist (and have second axes live in the guides framework), but it's designed to be interface-compatible with Scale so that we don't have to rewrite everything if we go that direction.

I think the general idea was to test the guide_axis() stuff with CoordCartesian, but there's no reason (I think) that it couldn't be implemented in the other coords as well. If that's something that would be welcome (and the PR would get reviewed), I could give it a shot (or you could...no problem either way).

@yutannihilation
Copy link
Member

Thanks, then let me try a PR this time. This would be a nice lesson to get familiar with coords and guides (and even if I fail, the experience should be useful to review your PRs...).

@yutannihilation
Copy link
Member

coord_trans() was fixed by #3972. coord_polar() (and other coords) stays the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants