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

Major update to plot_ly() #628

Merged
merged 75 commits into from
Jul 13, 2016
Merged

Major update to plot_ly() #628

merged 75 commits into from
Jul 13, 2016

Conversation

cpsievert
Copy link
Collaborator

@cpsievert cpsievert commented Jun 16, 2016

@jackparmer @timelyportfolio

This is a major update to the plot_ly() interface that addresses a number of concerns raised by @hadley / @jcheng5 / others. See the NEWS file for a summary. My goal is to have this on CRAN before I fly out to SF on the 23rd. Here is a list of stuff that should be addressed before submitting:

  • Get a working version of add_polygon()/add_ribbon(). BTW, if anyone has ideas for other add_*() functions (to do things that aren't easy/available or slow in ggplot2) I'm all ears!
  • Add tests/documentation
  • Fix grouping when there is multiple traces, for instance:
g <- expand.grid(visit = 1:10, id = 1:50, cohort = c("A", "B"))
g$response <- rnorm(nrow(g))
g %>%
  group_by(id) %>%
  plot_ly(x = ~visit, y = ~response, color = ~cohort, opacity = 0.5)

I also think that color should be deprecated and split into stroke/fill, but that should probably wait for now.

@cpsievert
Copy link
Collaborator Author

@timelyportfolio it looks like resetting zoom within an R markdown doc is broken on this branch. Could you check out #627 for me and see if it's just v1.13.0 that breaks it?

@timelyportfolio
Copy link
Collaborator

timelyportfolio commented Jun 17, 2016

This will be really nice. I am working through examples and tests now. The first issue I noticed is in the ?plot_ly.

data(economics, package = "ggplot2")
# basic time-series plot
p <- plot_ly(economics, x = ~date, y = ~uempmed)
# add a loess smoother
p2 <- add_trace(p, y = ~fitted(loess(uempmed ~ as.numeric(date))))
# add a title
p3 <- layout(p2, title = "Median duration of unemployment (in weeks)")
# change the font
layout(p3, font = list(family = "Courier New, monospace"))

and I get

image

I would expect two lines (one raw and a second smoother). Also, the title is lost.

I'll work through, but I just want to report, so I don't forget.

@timelyportfolio
Copy link
Collaborator

timelyportfolio commented Jun 17, 2016

this works

plot_ly() %>%
  add_data(economics) %>%
  add_trace(x = ~date, y = ~pce) %>%
  add_trace(x = ~date, y = ~log(pce,base=10)
)

so I'm struggling to understand why the other does not.

p$x$layoutAttrs <- NULL
p$x$layout <- modify_list(p$x$layout, Reduce(modify_list, layouts))

# If type was not specified in plot_ly(), it doesn't create a trace unless
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems this explains #628 (comment), but I don't understand the logic. Could you elaborate on this design?

Copy link
Collaborator

@timelyportfolio timelyportfolio Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I comment out, then both the smoother and raw line are plotted, so this does seem to explain what is happening, but I don't understand why this choice was made. I'm guessing there are use cases where this is beneficial.

@cpsievert
Copy link
Collaborator Author

cpsievert commented Jun 17, 2016

@timelyportfolio similar to ggvis()/ggplot(), I'd like to move to a system where plot_ly() initializes a visualization, and defines global attributes/properties, but doesn't necessarily define a trace (i.e., layer) itself. That way, this would create a single trace, rather than 2:

plot_ly(economics, x = ~date, y = ~pop) %>% add_lines()

If no add_trace()-like functions are used, we essentially add it at print time (so that these would also create the same result).

plot_ly(economics, x = ~date, y = ~pop, type = "scatter")
plot_ly(economics, x = ~date, y = ~pop)
No trace type specified. Using the 'scatter' default.

Unfortunately, this does introduce some confusing (backwards-incompatible) changes. Take the first example on plot_ly(). In the past, this would create two traces:

plot_ly(economics, x = ~date, y = ~uempmed) %>% 
  add_trace(y = ~fitted(loess(uempmed ~ as.numeric(date))))

But, given this new philosophy (which I think is better and more consistent with grammar of graphics systems), we'd expect that to create one trace. This is now the preferred way to express that plot:

plot_ly(economics, x = ~date) %>% 
  add_lines(y = ~uempmed) %>% 
  add_lines(y = ~fitted(loess(uempmed ~ as.numeric(date))))

nlayouts <- length(p$x$layout)
p$x$layout <- c(p$x$layout, attrs)
} else {
p$x$layoutAttrs[[p$x$cur_data]] <- attrs
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will overwrite any previous defined layout. Is this intentional?

data(economics, package = "ggplot2")
# basic time-series plot
p <- plot_ly(economics, x = ~date, y = ~uempmed)
# add a loess smoother
p2 <- add_trace(p, y = ~fitted(loess(uempmed ~ as.numeric(date))))
# add a title
p3 <- layout(p2, title = "Median duration of unemployment (in weeks)")
# change the font
layout(p3, font = list(family = "Courier New, monospace"))

The title is lost.

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, no, not intentional, I'll fix it, thanks!

Copy link
Collaborator

@timelyportfolio timelyportfolio Jun 17, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpsievert, ok that makes much more sense. Then, in this case, I think documentation, discussion, and examples shoul reflect this philosophy/change. Where is the best place to first communicate this? in ?plot_ly?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea, working on that as we speak :)

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.

2 participants