POSIXt looks weird when mapped to color #718

Closed
wch opened this Issue Nov 7, 2012 · 6 comments

Comments

Projects
None yet
3 participants
@wch
Collaborator

wch commented Nov 7, 2012

Example:

library(plyr)
library(ggplot2)

dat <- mutate(data.frame(x=seq(0,10,0.1)),
              y = sin(x),
              t = as.POSIXct(sprintf("2012-07-18 20:%d:%f", as.integer(x), x)))

# Weird discrete colors
ggplot(dat) + geom_point(aes(x=x, y=y, color=t))


# It should look something like this (with gradient)
ggplot(dat) + geom_point(aes(x=x, y=y, color=unclass(t)))
@lehy

This comment has been minimized.

Show comment Hide comment
@lehy

lehy Nov 8, 2012

Contributor

Bisecting this issue gives the following commit. I hope that helps.

1b3ff24 is the first bad commit
commit 1b3ff24
Author: Winston Chang winston@stdout.org
Date: Mon Jun 25 00:53:50 2012 -0500

scales_add_defaults: find discrete/date/datetime/continuous scale type

Previously, it used trans_type to find the appropriate transformation.
Now it uses scale_type to find the appropriate scale -- and the scale
definitions contain information about which trans to use ("date",
"time", "identity").

:040000 040000 e16b33654d4813f72c22688ac371fe21c1b677ad a8b6100702848be4eaa34c5f18b0a8ecc8bd0441 M R

Contributor

lehy commented Nov 8, 2012

Bisecting this issue gives the following commit. I hope that helps.

1b3ff24 is the first bad commit
commit 1b3ff24
Author: Winston Chang winston@stdout.org
Date: Mon Jun 25 00:53:50 2012 -0500

scales_add_defaults: find discrete/date/datetime/continuous scale type

Previously, it used trans_type to find the appropriate transformation.
Now it uses scale_type to find the appropriate scale -- and the scale
definitions contain information about which trans to use ("date",
"time", "identity").

:040000 040000 e16b33654d4813f72c22688ac371fe21c1b677ad a8b6100702848be4eaa34c5f18b0a8ecc8bd0441 M R

@wch

This comment has been minimized.

Show comment Hide comment
@wch

wch Nov 8, 2012

Collaborator

Thanks for the bisect! I suspected it had something to do with that change.

Collaborator

wch commented Nov 8, 2012

Thanks for the bisect! I suspected it had something to do with that change.

@wch

This comment has been minimized.

Show comment Hide comment
@wch

wch Nov 30, 2012

Collaborator

It appears that it has to do with the change from this:

   if (disc) {
      args <- list()
    } else {
      args <- list(trans = trans_type(datacols[[aes]]))
    }
    scale <- do.call(scale_f, args)
    scales$add(scale)

to this:

    scales$add(scale_f())

Previously, when the scale was non-discrete (which includes datetimes) the code called scale_f() with the argument trans=xx. Now it doesn't pass in a trans argument.

# Old version did something like this:
scale_colour_gradient(trans="time")
#continuous_scale(aesthetics = "colour", scale_name = "gradient", 
#    palette = seq_gradient_pal(low, high, space), na.value = na.value, 
#    trans = "time", guide = guide)

# New version does something like this:
scale_colour_gradient()
#continuous_scale(aesthetics = "colour", scale_name = "gradient", 
#    palette = seq_gradient_pal(low, high, space), na.value = na.value, 
#    guide = guide)



# And for plain old continuous scale:
# Old version
scale_x_continuous(trans="identity")
# continuous_scale(aesthetics = c("x", "xmin", "xmax", "xend", 
#     "xintercept"), scale_name = "position_c", palette = identity, 
#     expand = expand, trans = "identity", guide = "none")

# New version
scale_x_continuous()
# continuous_scale(aesthetics = c("x", "xmin", "xmax", "xend", 
#     "xintercept"), scale_name = "position_c", palette = identity, 
#     expand = expand, guide = "none")

# Also, Date objects got trans="date"

The goal of that commit was to get rid of the trans_type() function and replace it with scale_type() instead. Maybe the default trans should be set in the scales, like scale_x_datetime, scale_x_continuous, etc?

Collaborator

wch commented Nov 30, 2012

It appears that it has to do with the change from this:

   if (disc) {
      args <- list()
    } else {
      args <- list(trans = trans_type(datacols[[aes]]))
    }
    scale <- do.call(scale_f, args)
    scales$add(scale)

to this:

    scales$add(scale_f())

Previously, when the scale was non-discrete (which includes datetimes) the code called scale_f() with the argument trans=xx. Now it doesn't pass in a trans argument.

# Old version did something like this:
scale_colour_gradient(trans="time")
#continuous_scale(aesthetics = "colour", scale_name = "gradient", 
#    palette = seq_gradient_pal(low, high, space), na.value = na.value, 
#    trans = "time", guide = guide)

# New version does something like this:
scale_colour_gradient()
#continuous_scale(aesthetics = "colour", scale_name = "gradient", 
#    palette = seq_gradient_pal(low, high, space), na.value = na.value, 
#    guide = guide)



# And for plain old continuous scale:
# Old version
scale_x_continuous(trans="identity")
# continuous_scale(aesthetics = c("x", "xmin", "xmax", "xend", 
#     "xintercept"), scale_name = "position_c", palette = identity, 
#     expand = expand, trans = "identity", guide = "none")

# New version
scale_x_continuous()
# continuous_scale(aesthetics = c("x", "xmin", "xmax", "xend", 
#     "xintercept"), scale_name = "position_c", palette = identity, 
#     expand = expand, guide = "none")

# Also, Date objects got trans="date"

The goal of that commit was to get rid of the trans_type() function and replace it with scale_type() instead. Maybe the default trans should be set in the scales, like scale_x_datetime, scale_x_continuous, etc?

@hadley

This comment has been minimized.

Show comment Hide comment
@hadley

hadley Nov 30, 2012

Member

So maybe it should be scales$add(scale_f(trans = type)) ?

Member

hadley commented Nov 30, 2012

So maybe it should be scales$add(scale_f(trans = type)) ?

@wch

This comment has been minimized.

Show comment Hide comment
@wch

wch Nov 30, 2012

Collaborator

Wouldn't that defeat the purpose of getting rid of the trans_type() function?

I was thinking of redefining scale_x_continuous and the like. For example:

scale_x_datetime <- function(..., expand = waiver(), breaks = pretty_breaks(),
  minor_breaks = waiver()) {

  scale_datetime(c("x", "xmin", "xmax", "xend"), expand = expand,
    breaks = breaks, minor_breaks = minor_breaks, trans = "time", ...)
}

I think that would be more in line with the purpose of the change from trans_type to scale_type (but my memory on this is a bit hazy).

Edit: Actually, nevermind the part about scale_x_datetime -- it looks like scale_datetime already has trans="time". The issue is that scale_colour_gradient needs to get trans="time" somehow, and obviously that can't be the default setting for the scale. So maybe we need another trans_type() replacement, or we should rethink how this works.

Collaborator

wch commented Nov 30, 2012

Wouldn't that defeat the purpose of getting rid of the trans_type() function?

I was thinking of redefining scale_x_continuous and the like. For example:

scale_x_datetime <- function(..., expand = waiver(), breaks = pretty_breaks(),
  minor_breaks = waiver()) {

  scale_datetime(c("x", "xmin", "xmax", "xend"), expand = expand,
    breaks = breaks, minor_breaks = minor_breaks, trans = "time", ...)
}

I think that would be more in line with the purpose of the change from trans_type to scale_type (but my memory on this is a bit hazy).

Edit: Actually, nevermind the part about scale_x_datetime -- it looks like scale_datetime already has trans="time". The issue is that scale_colour_gradient needs to get trans="time" somehow, and obviously that can't be the default setting for the scale. So maybe we need another trans_type() replacement, or we should rethink how this works.

@hadley

This comment has been minimized.

Show comment Hide comment
@hadley

hadley Feb 24, 2014

Member

This sounds like a great feature, but unfortunately we don't currently have the development bandwidth to support it. If you'd like to submit a pull request that implements this feature, please follow the instructions in the development vignette.

Member

hadley commented Feb 24, 2014

This sounds like a great feature, but unfortunately we don't currently have the development bandwidth to support it. If you'd like to submit a pull request that implements this feature, please follow the instructions in the development vignette.

@hadley hadley closed this Feb 24, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment