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

New call signature for Plotly.plot #1014

Closed
rreusser opened this issue Oct 5, 2016 · 12 comments
Closed

New call signature for Plotly.plot #1014

rreusser opened this issue Oct 5, 2016 · 12 comments

Comments

@rreusser
Copy link
Contributor

rreusser commented Oct 5, 2016

Problem: Frames cannot be passed to the initial call to Plotly.plot. I don't think breaking changes are required to change that fact, so this might be nice before it's integrated in a bunch of different places. You also have to unpack plotly JSON data that almost certainly has the format:

{
  "data": [],
  "layout": {},
  "config": {},
  "frames": []
}

(Note: config is not fully serializable)

Currently to create a plot, you must run

Plotly.plot(gd, d.data, d.layout, d.config).then(function() {
  return Plotly.addFrames(gd, d.frames);
});

Possible solutions

It's not bad, but the extra step seems unnecessary. Alternate proposed forms that could be implemented without breaking changes by overloading Plotly.plot:

Plotly.plot(gd, d.data, d.layout, d.config, d.frames);

Frames could simply be appended, though config seems nice as a final argument. Switching the last two arguments:

Plotly.plot(gd, d.data, d.layout, d.frames, d.config);

This case could be distinguished from the current form by the presence of an Array-typed fourth argument.

Taking a different approach and passing a single object:

Plotly.plot({
  container: gd,
  data: [],
  layout: {},
  config: {},
  frames: []
});

Except neither gd nor config are serializable, in general. Extracting the gd part, we get:

Plotly.plot(gd, {
  data: [],
  layout: {},
  config: {},
  frames: []
});

I rather like this final form since with some non-serializable exceptions in config (specifics?), you could simply pass fetched data toPlotly.plot in one go, i.e.:

fetch('path/to/my/data.json').then(function(d) {
  Plotly.plot(gd, d);
});

Which is nice and short, but then at the end of all of this, the conclusion is that this all exists only to save a bit of typing. I think it's still worth discussing though since it seems sub-ideal that you can't fully pass data (missing: frames) to a plot in a single command.

cc: @etpinard @chriddyp

@rreusser
Copy link
Contributor Author

rreusser commented Oct 5, 2016

@etpinard
Copy link
Contributor

etpinard commented Oct 5, 2016

Except neither gd nor config are serializable,

I'd like to point out: it wouldn't be too hard to make config JSON-serializable. At the moment, only setBackground expect a function. We could easily turn setBackground into a enumerated in v2.0.0.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 5, 2016

Ah thanks for the clarification. I didn't know the specifics, but that doesn't sound like a show-stopper.

@etpinard
Copy link
Contributor

etpinard commented Oct 5, 2016

, but that doesn't sound like a show-stopper.

Yes, exactly.

That's why I'm voting for adding in option

Plotly.plot(gd, {
  data: [],
  layout: {},
  config: {},
  frames: []
});

@dy
Copy link
Contributor

dy commented Oct 5, 2016

I am a bit out of context and don’t know all the details, but single argument pattern, where container defaults to document.body makes it easier to run in simple use-case, esp. for new users (who may not care or even know about container creation, which is rather DOM thing than data-vis thing):

Plotly.plot({
  data: [],
  layout: {}
  //container: document.body
});

Also that is common pattern for regl and other components. Though it may be not applicable here.

@rreusser
Copy link
Contributor Author

rreusser commented Oct 5, 2016

I agree! That syntax is pretty tantalizing. The downside I see is that all of the other commands take the graph div as the first argument. This form might be best left for an object-oriented sort of notation that I believe was being discussed for 2.0.

var plot = Plotly.plot({...})
plot.update({...})

@dy
Copy link
Contributor

dy commented Oct 5, 2016

Some oldschool components followed the "middle way" of making first argument optional, not to destroy API and allow for new syntax:

Plotly.plot(container?, how);
Plotly.plot({container: container, ...});

One question — why not just

Plotly.plot(what, how?);
//which is
Plotly.plot(data, options?);

?

@rreusser
Copy link
Contributor Author

rreusser commented Oct 12, 2016

To expand on @dfcreative's comment about regl:

In regl, the equivalent of gd is not required. If you don't provide a container, a div is created and expanded to fill the window. This seems rather more like plotly 2.0 syntax where Plotly.plot returns an object upon which you can call .update() etc. The usage would be:

var plot = Ploty.plot({data: [...], layout: {}});
plot.update(...)

Presumably it would just make a div that fills the whole window and maintain its own reference. It's nothing worth losing sleep over, but maybe nice for people who don't really care about divs.

@monfera
Copy link
Contributor

monfera commented Oct 12, 2016

Indeed a root is optional, e.g. a canvas or even a gl context can be passed (the version here makes a <canvas> directly under <body>). Also, for some reason, inferno.js gives a warning when putting stuff directly into document.body, I'm unsure of the reasons other than perhaps these.

@rreusser
Copy link
Contributor Author

@monfera yeah, was at least thinking it would auto-append a div to document.body and then only operate on the div.

@etpinard etpinard added this to the v1.19.0 milestone Oct 13, 2016
@rreusser
Copy link
Contributor Author

@dfcreative:

One question — why not just ...

That makes sense, but I think the short answer is to keep overloading/variadic style to a minimum. PR #1054 basically allows two styles: the current behavior (with parameters optionally omitted) and the new style, which is basically just the old style wrapped in an object. I think it's best to keep overloading to a minimum though, particularly in the main plot command.

rreusser added a commit that referenced this issue Oct 19, 2016
Modify Plotly.plot to accept frames #1014
@etpinard
Copy link
Contributor

closed by #1054

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

No branches or pull requests

4 participants