Skip to content

Proposed re-design #40

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

Closed
chriddyp opened this issue May 23, 2014 · 8 comments · Fixed by #226
Closed

Proposed re-design #40

chriddyp opened this issue May 23, 2014 · 8 comments · Fixed by #226

Comments

@chriddyp
Copy link
Member

With the configuration files we can re-architect the module to be a little more "R"-y. I propose the following interface:

Before

py = plotly(username, key)
trace0 = list(x=...y=...)
py$plotly(trace0)

After

plotly:::set_credentials(username, key)
plotly:::plot(trace0)

Or, if you prefer to send your credentials on a per-call basis (e.g. you're using the R demo account):

plotly:::plot(trace0, username=..., key=....)

And similarly for the other functions:

plotly:::ggplotly(..., plotly_options)
plotly:::plot(..., plotly_options)
plotly:::export_image(..., plotly_options)
plotly:::get_figure(..., plotly_options)

With additional functionality ...

figure = plotly:::get_figure(file_owner, file_id, plotly_options) # returns a list of lists of lists of ... (plotly's JSON graph JSON)

data = plotly:::get_data(figure) # doesn't make a request to plotly, it just removes strips keys out of the named list

A few questions I have (because I'm not an R expert)

1 - Is it the R convention to have nested modules/functions?

e.g. in Python we do:

import plotly.plotly as py

# all the functions in py, call the plotly servers
py.plot(...)
py.iplot(...)
py.get_figure(...)
py.image.save_as(...)

# other functions, like tools, are in a separate namespace:

```python
import plotly.tools as tls
tls.embed(figure)

Do we do a similar thing in R? Or do we keep all the functions at the same "level", e.g.

This:

plotly:::embed(figure)

or this:

plotly:::tools:::embed(figure) #??
@tdhock
Copy link
Contributor

tdhock commented May 27, 2014

Some constructive criticism:

"if you prefer to send your credentials on a per-call basis" --- that would be really inconvenient for me, who do you think would do that?

personally I prefer the existing interface, where the ~/.Rprofile file itself can be used instead of a credentials file. I write my plotly() call inside

py <- plotly("USER", "PASS")

Then later I will call py$ggplotly(). But I do understand the desire to have a common credentials file if you will be using several client APIs on the same machine.

In R all functions are on the same level:

plotly:::embed(figure)

@tdhock
Copy link
Contributor

tdhock commented May 28, 2014

An argument for your redesign is that it should be easier for debugging and development.

Developing/testing/debugging with "Before" means executing

source("plotly.R")
source("ggplotly.R")
py <- plotly("USER", "PASS")
py$ggplotly()

whereas with "After" it is 1 line shorter (assuming default user/pass taken from credentials file)

source("plotly.R")
source("ggplotly.R")
ggplotly()

currently instead of re-making the py object every time, I use Plotly/test_ggplotly defined as follows in my ~/.Rprofile

  Plotly <- plotly("tdhock", "PASS")
  test_ggplotly <- function(gg, p=Plotly){
    if(!is.ggplot(gg)){
      stop("gg must be a ggplot")
    }
    if(!is.function(p$plotly)){
      stop("p must be a plotly interface object")
    }
    pargs <- gg2list(gg)
    resp <- do.call(p$plotly, pargs)
    browseURL(resp$url)
    invisible(list(data=pargs, response=resp))
  }

and my testing/dev command lines look like

source("ggplotly.R")
test_ggplotly(some_ggplot)

@chriddyp
Copy link
Member Author

My main argument for:

plotly::plot(data)

instead of

py<-plotly()
py$plot(data)

is that:

1 - Users will set-up their config once (like they do with git/.gitconfig). After they've set-up their config, I think it is unclear for people what py<-plotly() does, and why it's needed!
2 - It seems (but maybe I'm off here, I'm pretty new to R) that the list$function syntax is unconventional/not used often?

@mkcor
Copy link
Contributor

mkcor commented May 28, 2014

@tdhock We spoke yesterday, so this is for reference only. ;)
Reply to your constructive criticism:

py <- plotly()

retrieves the credentials from your global Plotly configuration (let's call them your default credentials).

You may want to use certain credentials on a per-call basis (different from the default credentials). Use cases include: You use more than one Plotly account, where one is public, and/or one is private, and/or one is personal, and/or one is shared with other people, and/or one is for demos, testing, a specific project, etc.

py2 <- plotly("otherAccount", "otherKey")

@tdhock
Copy link
Contributor

tdhock commented May 28, 2014

Chris you are right that it is not obvious why one would need to type
py<-plotly().

With that in mind I think it is a good idea to do the re-deisgn.

However please do not use plotly::plot(data) -- it is problematic to
define R functions in a package that have the same name as base R
functions. Just call the main plotly function something else other
than "plot"

On Wed, May 28, 2014 at 3:27 PM, Marianne Corvellec <
[email protected]> wrote:

@tdhock https://github.com/tdhock We spoke yesterday, so this is for
reference only. ;)
Reply to your constructive criticism:

py <- plotly()

retrieves the credentials from your global Plotly configuration (let's
call them your default credentials).

You may want to use certain credentials on a per-call basis (different
from the default credentials). Use cases include: You use more than one
Plotly account, where one is public, and/or one is private, and/or one is
personal, and/or one is shared with other people, and/or one is for demos,
testing, a specific project, etc.


Reply to this email directly or view it on GitHubhttps://github.com//issues/40#issuecomment-44453120
.

@vdimarco
Copy link

vdimarco commented Jun 2, 2014

@tdhock regarding your comment:

However please do not use plotly::plot(data) -- it is problematic to
define R functions in a package that have the same name as base R
functions. Just call the main plotly function something else other
than "plot"

Do namespaces solve this problem?

@tdhock
Copy link
Contributor

tdhock commented Jun 2, 2014

Defining a plotly::plot function is not a technical problem; it is indeed possible.

It is a user interface problem -- I think it is confusing to use the same function name as the base plot function.

@cpsievert
Copy link
Collaborator

I agree with @chriddyp that the current interface is confusing. As I understand it, the main reason for having a function return a list of functions is to enable a mutable state. As far I can tell, that isn't necessary here.

In addition to avoiding permission issues when reading/writing credentials to disk, if we kept track of them via environment variables, you could still specify non-default credential on a per-call basis:

plotly <- function(p, username, key, ...) {
  if (missing(username)) username <- Sys.getenv("PLOTLY_USER")
  if (missing(username)) username <- Sys.getenv("PLOTLY_KEY")

  ....
}

Then you could just do

# use default credentials
plotly::plotly(qplot(1:10))

or

plotly::plotly(qplot(1:10), "my_username", "my_key")

I'd be interested in helping with a redesign, but the changes would be backwards-incompatible and it might be a lot of work.

Also, @chriddyp, I don't think nested namespaces are possible in R. However, it's worth noting that with knitr, we can control printing of objects based on their class. We do this in animint. For example,

```r
(p <- qplot(1:10))
```

would embed a ggplot2 plot

```r
structure(p, class = "animint")
```

calls animint2dir and embeds the animint plot

This was referenced May 2, 2015
@cpsievert cpsievert added this to the v1.0 milestone May 30, 2015
@cpsievert cpsievert mentioned this issue Jun 3, 2015
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 a pull request may close this issue.

5 participants