Skip to content

Consistency of handling a wrong length of parameter #3026

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
yutannihilation opened this issue Dec 6, 2018 · 10 comments · Fixed by #6100
Closed

Consistency of handling a wrong length of parameter #3026

yutannihilation opened this issue Dec 6, 2018 · 10 comments · Fixed by #6100

Comments

@yutannihilation
Copy link
Member

During reviewing #3024, I'm a bit confused to see multiple values are handled differently. I'm yet to find which behaviour is the supposed one.

library(ggplot2)

df <- data.frame(
  x = c(1,3,2,5),
  y = c("a","c","d","c")
)

nudge_x and nudge_y accept a different length of parameter w/ a warning.

p <- ggplot(df, aes(x, y)) +
  geom_point() +
  geom_label(aes(label = y),
             nudge_y = c(-0.1, 0.1, -0.1))

invisible(ggplot_build(p))
#> Warning in y + params$y: longer object length is not a multiple of shorter
#> object length

If the parameter is an aesthetic variable, it raises an error.

p <- ggplot(df, aes(x, y)) +
  geom_point() +
  geom_label(aes(label = y),
             colour = c("red", "red", "green"))
invisible(ggplot_build(p))
#> Error: Aesthetics must be either length 1 or the same as the data (4): colour

Some parameters just ignore multiple values (this seems because gpar() does so).

ggplot(df, aes(x, y)) +
  geom_point() +
  geom_label(aes(label = y),
             label.size = c(1, 10, 100))

Created on 2018-12-06 by the reprex package (v0.2.1)

@clauswilke
Copy link
Member

A related issue: The nudge_x and nudge_y parameters in geom_text()/geom_label() are weird because they're not handled like other parameters for geoms or stats. Instead, they are handed off to position_nudge(). Is there a reason why it is done that way? Couldn't geom_text()/geom_label() just add the values of nudge_x and nudge_y to the x and y coordinates in draw_panel()? This would also make it possible to turn nudge_x and nudge_y into aesthetics that can be mapped. I guess the devil is in the details for transformed and non-linear coordinates, but either way it should be possible to exactly mimic what position_nudge() currently does.

@yutannihilation
Copy link
Member Author

Basically, I agree with you!

Is there a reason why it is done that way?

I don't see any reason for now, but, if I understand correctly, position is not allowed to require custom aesthetics currently. So, we might need some effort to change internals to make it possible. For example, this is all that are considered as aesthetics:

all <- c(geom$parameters(TRUE), stat$parameters(TRUE), geom$aesthetics())

@clauswilke
Copy link
Member

if I understand correctly, position is not allowed to require custom aesthetics currently.

That's correct. However, I was talking about something else. I was suggesting that nudge_x and nudge_y could be aesthetics of geom_text() and handled by its draw_panel() function directly. So nudging would be independent of the position adjustment, and in fact it would work with any position adjustment you may want to use.

@yutannihilation
Copy link
Member Author

Oh, I read your comment wrong, sorry... Although it sounds good itself, I guess it's difficult to remove position_nudge() even when we don't actually need it, since it's already released. Then, I'm a bit afraid if we can maintain the multiple ways to nudge.

@clauswilke
Copy link
Member

I didn't mean to propose to remove position_nudge() either. :-) I'm just saying geom_text() doesn't need to use it. It could have its own nudging feature.

@yutannihilation
Copy link
Member Author

I mean, if geom_text()/geom_label() take over nudging, I rather feel position_nudge() should be removed to avoid confusion (but it seems not feasible). Otherwise, we will have the nudging implementation both in geom_text() and in position_nudge(), which might make it difficult to maintain.

One more question is, do you mean one can specify nudge_x/nudge_y in aes() and position_nudge() at the same time? This might be just a cornor case, but I'm yet to see if this is OK.

ggplot(df) +
  geom_text(aes(x, y, nudge_y = 0.01 * sign(x), label = lab),
            position = position_nudge(y = 0.1))

@clauswilke
Copy link
Member

I made a PR (#3030) to explain what I mean. I see no technical problems. In either case, position_nudge() should be retained, because it is useful beyond geom_text()/geom_label(). But nudging is sufficiently common for those two that they may deserve more specialized treatment.

@yutannihilation
Copy link
Member Author

Thanks for the PR. I agree that there's no technical problems, but I still feel I'm not convinced...

To be clear, I want to say Position should be allowed to require custom aesthetics, apart form the implementational details. Do you think this idea is semantically wrong? If so, I'll support your PR.

@clauswilke
Copy link
Member

To be clear, I want to say Position should be allowed to require custom aesthetics, apart form the implementational details. Do you think this idea is semantically wrong? If so, I'll support your PR.

No, it's not semantically wrong. In fact, I think it's needed. I just don't know how to implement it without completely rewriting layers.

@yutannihilation
Copy link
Member Author

Hmm, then, I want to wait for the day when position_nudge() gets control over nudge_x and nudge_y aes. IMHO, the problem is not so urgent or serious that we have to implement it as geom_text() immediately.

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.

3 participants