-
Notifications
You must be signed in to change notification settings - Fork 2.1k
Shapes provided via strings instead of integers #2338
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
Changes from 20 commits
a12f81a
46b3d79
92ad484
f8812b7
8f6bae3
d874a8c
69af1f6
02a19ec
6d7dab9
0ea30b3
f160450
a96f312
bea09d6
d2de3eb
f3948a8
ec64f9c
487b603
d8dadce
4e79af7
aee9b47
4cac31d
acf9f18
2916e40
8e9e09f
747cf63
044de3e
1aa5a04
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -126,6 +126,10 @@ GeomPoint <- ggproto("GeomPoint", Geom, | |
), | ||
|
||
draw_panel = function(data, panel_params, coord, na.rm = FALSE) { | ||
if (is.character(data$shape)) { | ||
data$shape <- translate_shape_string(data$shape) | ||
} | ||
|
||
coords <- coord$transform(data, panel_params) | ||
ggname("geom_point", | ||
pointsGrob( | ||
|
@@ -144,3 +148,90 @@ GeomPoint <- ggproto("GeomPoint", Geom, | |
|
||
draw_key = draw_key_point | ||
) | ||
|
||
translate_shape_string <- function(shape_string) { | ||
if (nchar(shape_string[1]) == 1) { | ||
return(shape_string) | ||
} | ||
|
||
pch_table <- c( | ||
"square open" = 0, | ||
"circle open" = 1, | ||
"triangle open" = 2, | ||
"plus" = 3, | ||
"cross" = 4, | ||
"diamond open" = 5, | ||
"triangle down open" = 6, | ||
"square cross" = 7, | ||
"asterisk" = 8, | ||
"diamond plus" = 9, | ||
"circle plus" = 10, | ||
"star" = 11, | ||
"square plus" = 12, | ||
"circle cross" = 13, | ||
"square triangle" = 14, | ||
"square" = 15, | ||
"circle small" = 16, | ||
"triangle" = 17, | ||
"diamond" = 18, | ||
"circle" = 19, | ||
"bullet" = 20, | ||
"circle filled" = 21, | ||
"square filled" = 22, | ||
"diamond filled" = 23, | ||
"triangle filled" = 24, | ||
"triangle down filled" = 25 | ||
) | ||
|
||
shape_match <- charmatch(shape_string, names(pch_table)) | ||
|
||
invalid_strings <- is.na(shape_match) | ||
nonunique_strings <- shape_match == 0 | ||
|
||
if (any(invalid_strings)) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please follow the standard tidyverse style here? (Main problem is that |
||
bad_string <- unique(shape_string[invalid_strings]) | ||
n_bad <- length(bad_string) | ||
|
||
collapsed_names <- sprintf("\n* '%s'", bad_string[1:min(5, n_bad)]) | ||
|
||
more_problems <- if (n_bad > 5) { | ||
sprintf("\n* ... and %d more problem%s", n_bad - 5, ifelse(n_bad > 6, "s", "")) | ||
} | ||
|
||
stop( | ||
"Can't find shape name:", | ||
collapsed_names, | ||
more_problems, | ||
call. = FALSE | ||
) | ||
} | ||
|
||
if (any(nonunique_strings)) { | ||
bad_string <- unique(shape_string[nonunique_strings]) | ||
n_bad <- length(bad_string) | ||
|
||
n_matches <- vapply( | ||
bad_string[1:min(5, n_bad)], | ||
function(shape_string) sum(startsWith(names(pch_table), shape_string)), | ||
integer(1) | ||
) | ||
|
||
collapsed_names <- sprintf( | ||
"\n* '%s' partially matches %d shape names", | ||
bad_string[1:min(5, n_bad)], n_matches | ||
) | ||
|
||
more_problems <- if (n_bad > 5) { | ||
sprintf("\n* ... and %d more problem%s", n_bad - 5, ifelse(n_bad > 6, "s", "")) | ||
} | ||
|
||
stop( | ||
"Shape names must be unambiguous:", | ||
collapsed_names, | ||
more_problems, | ||
call. = FALSE | ||
) | ||
} | ||
|
||
unname(pch_table[shape_match]) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
context("translate_shape_string") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And this should now become |
||
|
||
test_that("strings translate to their corresponding integers", { | ||
shape_strings <- c( | ||
"square open", | ||
"circle open", | ||
"triangle open" | ||
) | ||
|
||
expect_equal(translate_shape_string(shape_strings[1]), 0) | ||
expect_equal(translate_shape_string(shape_strings), 0:2) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not immediately clear to me what the intent of these tests are — I think it's to check that |
||
|
||
expect_equal( | ||
translate_shape_string(rep.int(shape_strings[1], 10)), | ||
rep.int(0, 10) | ||
) | ||
|
||
expect_equal( | ||
translate_shape_string(rep(shape_strings, each = 4)), | ||
rep(0:2, each = 4) | ||
) | ||
}) | ||
|
||
test_that("single characters are not translated to integers", { | ||
expect_equal(translate_shape_string(letters), letters) | ||
expect_equal(translate_shape_string(as.character(0:9)), as.character(0:9)) | ||
}) | ||
|
||
test_that("invalid shape names raise an error", { | ||
expect_error(translate_shape_string("void"), "Can't find shape name") | ||
expect_error(translate_shape_string("tri"), "Shape names must be unambiguous") | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You missed this