search

MUCollective / pgog

Supplemental to A Probabilistic Grammar of Graphics
14 stars 1 forks source link

handle fill = factor() correctly in parsing #82

Closed xiaoyingpu closed 4 years ago

xiaoyingpu commented 4 years ago

Well PR #81 broke this bit

Somewhere around https://github.com/MUCollective/pgog/blame/parser_fixes/R/parse.R#L294

mjskay commented 4 years ago

is this a case where if someone is using factor() in the spec they should actually be converting the variable ahead of time before running it through ggplot?

xiaoyingpu commented 4 years ago

Yeah theoretically but I do stuff like this all the time especially when I need to map factors to colors

ggplot(mtcars, aes(x = factor(cyl), y = mpg, fill = factor(cyl)) + geom_col()
mjskay commented 4 years ago

Sure, but a simple rewrite is something like:

mtcars %>%
  mutate(cyl = factor(cyl)) %>%
  ggplot(aes(x = cyl, y = mpg, fill = cyl)) +
  geom_col()

Then you only have to do the conversion once?

On the other hand, you could allow arbitrary expressions but evaluate them first, and then label them according to the expression converted into a string and use that string to match variables (e.g. when making sure a probability expression is grounded properly / verifying it multiplies to a joint). So you wouldn't have to special-case the factor() function (or any other function), but people would be able to include arbitrary expressions as long as they used the same expression everywhere.

xiaoyingpu commented 4 years ago

This should be taken care of now