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

Convert linetype NAs to blank lines #6270

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

teunbrand
Copy link
Collaborator

This PR aims to fix #6269.

Reprex from the issue; note no errors and no red outline:

devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2

ggplot(data.frame(x = 1:2, y = 1:2), aes(x, y)) +
  geom_tile(linetype = NA, colour = "red")

Created on 2025-01-06 with reprex v2.1.1

@thomasp85
Copy link
Member

Does this change mean that the linetype legend guide now has a blank slot for NA values? That is not very telling if so

@teunbrand
Copy link
Collaborator Author

It has a blank slot for NA values when there are NA values included in the scale limits/breaks, which by default occurs when there are NAs in the data.

@thomasp85
Copy link
Member

I guess this is a rare occurrence but it may catch users off-guard. I know this isn't really a fault of this PR but more a consequence of changing na.value to NA.

I think I'd feel better if na.value remained as "blank" but internally got translated to NA since this makes it clearer to the user that lines are expected to be blank

@teunbrand
Copy link
Collaborator Author

The issue with na.value = "blank" is that you can't use numeric linetypes like 1 (solid), 2 (dashed) etc. Of course you could then specify na.value = 0 but that is a bit of an unecessary inconvenience in my view.

I'm not sure I understand why having a blank linetype key would be undesirable? Is this not the correct behaviour?

devtools::load_all("~/packages/ggplot2/")
#> ℹ Loading ggplot2

df <- data.frame(x = 1:3)

ggplot(df, aes(x, xend = x + 1, y = x)) +
  geom_segment(aes(linetype = c("A", "B", NA)))
#> Warning: Removed 1 row containing missing values or values outside the scale range
#> (`geom_segment()`).

Created on 2025-01-28 with reprex v2.1.1

@thomasp85
Copy link
Member

I just mean that the default in the scale constructor is "blank" but under the hood we convert it to an NA and then keep the changes in this PR. It is more about signalling to the user what the scale does as I think na.value = NA is very confusing and doesn't help the user

All that being said, there are actual issues to having it blank as it completely removes the data. So you cannot see where the missing data supposedly is. But that is a whole other discussion and the ship has sailed on it I think

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jan 28, 2025

So if I interpret this right, this is just about making sure the usage section in the documentation gives an interpretable value?

If that is the case, I have a counterproposal. We remove na.value = NA from the linetype scales, as it is the default in discrete_scale(). However, we give linetypes the #6236 treatment and include a little table with character/numeric interpretation of linetypes that includes NA.

@teunbrand
Copy link
Collaborator Author

teunbrand commented Jan 28, 2025

I went ahead and implemented the linetype table idea. For the future, if anyone needs to replicate the figures, this is the code I used:

df <- data.frame(
  number = c(0:6),
  num_display = c("0/NA", 1:6),
  name = c('"blank"/NA', '"solid"', '"dashed"', '"dotted"', '"dotdash"', '"longdash"', '"twodash"'),
  hex = c("", "", "\"44\"", "\"13\"", "\"1343\"", "\"73\"", "\"2262\"")
)

p <- ggplot(df, aes(x = 2, xend = 5, y = 6 - number)) +
  geom_text(aes(x = 0, label = name), hjust = 0) +
  geom_text(aes(x = -1, label = num_display), hjust = 0) +
  geom_text(aes(x = 1, label = hex), hjust = 0) +
  geom_segment(aes(linetype = I(number))) +
  scale_x_continuous(
    breaks = c(-1:2),
    labels = c("Number", "Name", "Hex code", "Display"),
    position = 'top'
  ) +
  theme_void() +
  theme(
    axis.text.x.top = element_text(hjust = 0, size = 12, margin = margin_auto(10)),
    plot.margin = margin_auto(5)
  )

ggsave(
  file.path("man", "figures", "linetype_table.svg"),
  plot = p, device = svglite::svglite, fix_text_size = FALSE,
  width = 6, height = 4, dpi = 72, bg = "white"
)
ggsave(
  file.path("man", "figures", "linetype_table.pdf"), 
  plot = p,
  width = 6, height = 4, dpi = 72, bg = "white"
)

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.

linetype = NA should imply no line, not errors
2 participants