Shapes provided via strings instead of integers #2338

daniel-barnett · 2017-11-15T10:04:26Z

Implementation of #2075

data(mtcars)
n_obs <- nrow(mtcars)
pch_table <- c("0" = "square open",
               "1" = "circle open",
               "2" = "triangle open",
               "3" = "plus",
               "4" = "cross",
               "5" = "diamond open",
               "6" = "triangle open down",
               "7" = "square cross",
               "8" = "asterisk",
               "9" = "diamond plus",
               "10" = "circle plus",
               "11" = "star",
               "12" = "square plus",
               "13" = "circle cross",
               "14" = "square triangle",
               "15" = "square",
               "16" = "circle small",
               "17" = "triangle",
               "18" = "diamond",
               "19" = "circle",
               "20" = "bullet",
               "21" = "circle filled",
               "22" = "square filled",
               "23" = "diamond filled",
               "24" = "triangle filled",
               "25" = "triangle filled down")

## Testing previous functionality remains

# All defaults remain the same
ggplot(mtcars) + geom_point(aes(hp, cyl))

# Numeric shapes remain the same
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = 1, size = 3)

# Single character shapes remain the same
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = "a", size = 3)

# Single character shapes for all observations remain the same
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = c(letters, LETTERS)[1:n_obs], size = 3)

# Manually setting shape
mtcars$am2 <- ifelse(mtcars$am, "Manual", "Automatic")
ggplot(mtcars) + geom_point(aes(hp, cyl, shape = am2)) + scale_shape_manual(values = 2:3)

# Manually setting shapes with names
ggplot(mtcars) + geom_point(aes(hp, cyl, shape = am2)) + 
  scale_shape_manual(values = c("Manual" = 2, "Automatic" = 3)) 

## Testing new functionality

# Constant shape defined by a string
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = "triangle", size = 3)

# Constant shape defined by a unique substring
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = "pl", size = 3)

# Constant shape string defined for all observations
shape_vect <- c(unname(pch_table), unname(pch_table)[1:(n_obs - length(pch_table))])
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = shape_vect, size = 3)

# Manually setting shape with strings
mtcars$am2 <- ifelse(mtcars$am, "Manual", "Automatic")
ggplot(mtcars) + geom_point(aes(hp, cyl, shape = am2)) + 
  scale_shape_manual(values = c("triangle open", "plus"))

# Manually setting shapes with strings and named vectors
ggplot(mtcars) + geom_point(aes(hp, cyl, shape = am2)) + 
  scale_shape_manual(values = c("Manual" = "triangle open", "Automatic" = "plus"))

# ERROR: Wrong number of shapes defined 
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = c("triangle", "circle"), size = 3)

# ERROR: Constant shape defined by a non-unique substring
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = "tri", size = 3)

# ERROR: No such shape string 
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = "void", size = 3)

# ERROR: Wrong string within a set
shape_vect[10] <- "void"
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = shape_vect, size = 3)

# ERROR: Multiple wrong strings within a set
shape_vect[11] <- "another_void"
shape_vect[21] <- "yetanother_void"
ggplot(mtcars) + geom_point(aes(hp, cyl), shape = shape_vect, size = 3)

hadley · 2017-11-30T23:17:03Z

Could you please add a couple of unit tests for translate_shape()?

We also need to consider where this behaviour should be documented. Somewhere in geom_point()?

hadley · 2017-11-30T23:15:17Z

R/scale-manual.r

@@ -56,6 +56,12 @@ scale_size_manual <- function(..., values) {
 #' @rdname scale_manual
 #' @export
 scale_shape_manual <- function(..., values) {
+  if (is.character(values) && nchar(values[1]) > 1) {


I think it would be better to incorporate the existing behaviour for single letters in to translate_shape_string(). An easy way do that would be to combine pch_table with setNames(letters, letter)

hadley · 2017-11-30T23:15:29Z

R/geom-point.r

+    "18" = "diamond",
+    "19" = "circle",
+    "20" = "bullet",
+    "21" = "circle filled ",


Trailing space

hadley · 2017-11-30T23:15:51Z

R/geom-point.r

@@ -98,6 +98,14 @@ geom_point <- function(mapping = NULL, data = NULL,
                       na.rm = FALSE,
                       show.legend = NA,
                       inherit.aes = TRUE) {
+  dots <- list(...)


I don't think this is the right place to do translation - it would be better to do so during drawing.

daniel-barnett · 2017-12-08T10:11:49Z

I'll get on those and update the pull request this weekend.

I think documentation in geom_point() is natural along with a comment saying that these would also work for other layers based on points.

hadley · 2018-05-01T23:02:11Z

Are you interesting in finishing this off? Sorry for dropping the ball on you, but there are a few little style tweaks needed.

No problem if you don't have time in the next week, just let me know and I'll do it myself (but I wanted you to have the opportunity to finally take this over the finish line if you wanted)

daniel-barnett · 2018-05-01T23:19:55Z

Sure, let me know!

hadley · 2018-05-01T23:51:02Z

R/geom-point.r

+  invalid_strings <- is.na(shape_match)
+  nonunique_strings <- shape_match == 0
+
+  if (any(invalid_strings)) {


Can you please follow the standard tidyverse style here? (Main problem is that ) should go on own line; you can use styler package to do automatically)

hadley · 2018-05-01T23:51:29Z

R/geom-point.r

+    bad_string <- unique(shape_string[invalid_strings])
+    collapsed_names <- paste0(bad_string, collapse = "', '")
+    stop(
+      "Invalid shape name: '", collapsed_names, "'",


Would you mind having a read of http://style.tidyverse.org/error-messages.html and seeing if you can rewrite these errors to more closely follow the guidelines?

For the ambiguous shape names, I've just put in the number of matches that the string would give; I'm not sure, but it might be nicer to give something like the following?

'tri' matches: triangle, triangle open, and 3 others

That's probably a bit too much effort for this case

hadley · 2018-05-01T23:51:57Z

tests/testthat/test-shape-translate.r

@@ -0,0 +1,81 @@
+context("translate_shape_string")


These tests should go in tests/testthat/test-geom-point.R

hadley · 2018-05-01T23:52:33Z

tests/testthat/test-shape-translate.r

+context("translate_shape_string")
+
+test_that("strings translate to their corresponding integers", {
+  shape_strings <- c(


There's no need to systematically test ever possible value - the main thing you want to achieve in this test is to make sure everything is plumbed together correctly.

hadley · 2018-05-01T23:53:03Z

tests/testthat/test-shape-translate.r

+})
+
+test_that("non-unique substrings of shape names raise an error", {
+  shape_strings <- c(


Similarly you only need to test a single string here, and in the next error (and you could probably combine the two tests)

daniel-barnett · 2018-05-02T08:18:20Z

Also, would it be best to document this in the Aesthetic Specifications vignette?

hadley

Mention in the aesthetic specification vignette would be great!

hadley · 2018-05-02T13:56:04Z

NEWS.md

@@ -177,6 +180,7 @@ up correct aspect ratio, and draws a graticule.
  use matrix-columns. These are rarely used but are produced by `scale()`;
  to continue use `scale()` you'll need to wrap it with `as.numeric()`, 
  e.g. `as.numeric(scale(x))`.
+>>>>>>> upstream/master


You missed this

hadley · 2018-05-02T13:56:59Z

tests/testthat/test-geom-point.R

@@ -0,0 +1,32 @@
+context("translate_shape_string")


And this should now become geom-point

hadley · 2018-05-02T13:58:33Z

tests/testthat/test-geom-point.R

+  )
+
+  expect_equal(translate_shape_string(shape_strings[1]), 0)
+  expect_equal(translate_shape_string(shape_strings), 0:2)


It's not immediately clear to me what the intent of these tests are — I think it's to check that translate_shape_string() is vectorised? If so, I think you should create a new test, and I think you should only need one expectation.

hadley · 2018-05-09T20:56:14Z

Looks good! Can you please take a look at the CI failures? You might just need to merge/rebase to get fixes from the master branch.

daniel-barnett · 2018-05-10T04:48:30Z

All passing now (the perils of using newer base R functions!)

hadley · 2018-05-10T13:19:26Z

Thanks for all your hard work on this!

lock · 2018-11-06T13:42:11Z

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

daniel-barnett and others added 8 commits November 9, 2017 14:41

Add shape string translate function

a12f81a

Call translate_shape_string from geom_point if shape is a character

46b3d79

Call translate_shape_string from scale_shape_manual if character

92ad484

Tidy-up/changing pch strings

f8812b7

Updated NEWS.md

8f6bae3

Sryle fixes

d874a8c

Style fixes

69af1f6

Remove paste() from stop()

02a19ec

hadley reviewed Nov 30, 2017

View reviewed changes

daniel-barnett added 7 commits December 9, 2017 12:17

Update translate_shape_string() function

6d7dab9

Remove shape string translation from manual scales (is superfluous)

0ea30b3

Add shape string translation to draw_key_point()

f160450

Strip names from translated shape integer vector

a96f312

translate_shape_string() tests

bea09d6

Merge branch 'master' of https://github.com/daniel-barnett/ggplot2

d2de3eb

Merge remote-tracking branch 'upstream/master'

f3948a8

hadley reviewed May 1, 2018

View reviewed changes

daniel-barnett added 5 commits May 2, 2018 17:59

Rename shape translate test file

ec64f9c

Fix style

487b603

Adhere to error message guidelines

d8dadce

Remove superfluous tests and merge two tests for invalidity

4e79af7

Merge remote-tracking branch 'upstream/master'

aee9b47

hadley reviewed May 2, 2018

View reviewed changes

daniel-barnett added 2 commits May 3, 2018 11:53

Adjust geom-point tests

4cac31d

Remove merge remnants

acf9f18

daniel-barnett added 3 commits May 3, 2018 22:57

Add "triangle square" as an alias for "square triangle" shape

2916e40

Add shape name example in aesthetic vignette

8e9e09f

Quick formatting adjustments

747cf63

daniel-barnett added 2 commits May 10, 2018 15:43

Merge remote-tracking branch 'upstream/master'

044de3e

Replace startsWith for <= 3.2 compatibility

1aa5a04

hadley merged commit 22691ab into tidyverse:master May 10, 2018

clauswilke mentioned this pull request May 12, 2018

Request: support providing shapes as names rather than integers #2075

Closed

clauswilke mentioned this pull request May 25, 2018

geom_boxplot(outlier.size = NA) doesn't remove outliers after non-ggplot2 updates #2505

Closed

lock bot locked and limited conversation to collaborators Nov 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shapes provided via strings instead of integers #2338

Shapes provided via strings instead of integers #2338

daniel-barnett commented Nov 15, 2017

hadley commented Nov 30, 2017

hadley Nov 30, 2017

hadley Nov 30, 2017

hadley Nov 30, 2017

daniel-barnett commented Dec 8, 2017

hadley commented May 1, 2018

daniel-barnett commented May 1, 2018

hadley May 1, 2018

hadley May 1, 2018

daniel-barnett May 2, 2018 •

edited

Loading

hadley May 2, 2018

hadley May 1, 2018

hadley May 1, 2018

hadley May 1, 2018

daniel-barnett commented May 2, 2018

hadley left a comment

hadley May 2, 2018

hadley May 2, 2018

hadley May 2, 2018

hadley commented May 9, 2018

daniel-barnett commented May 10, 2018

hadley commented May 10, 2018

lock bot commented Nov 6, 2018

Shapes provided via strings instead of integers #2338

Shapes provided via strings instead of integers #2338

Conversation

daniel-barnett commented Nov 15, 2017

hadley commented Nov 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-barnett commented Dec 8, 2017

hadley commented May 1, 2018

daniel-barnett commented May 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-barnett May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-barnett commented May 2, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hadley commented May 9, 2018

daniel-barnett commented May 10, 2018

hadley commented May 10, 2018

lock bot commented Nov 6, 2018

daniel-barnett May 2, 2018 •

edited

Loading