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

Clean up design #19

Closed
sglyon opened this issue Jan 18, 2016 · 5 comments
Closed

Clean up design #19

sglyon opened this issue Jan 18, 2016 · 5 comments

Comments

@sglyon
Copy link
Member

sglyon commented Jan 18, 2016

This package is a bit tricky to design because we need to have both Julia and Plotly versions of a plot object.

The original goal was to have the Julia object be first class and simply have the plotly version be a "clone" of the Julia version that handles the view. I think that design was compromised, especially in the plotly.js api methods.

This issue is a place to brainstorm possible ways to clean up the design.

cc: @one-more-minute @tbreloff

@sglyon
Copy link
Member Author

sglyon commented Jan 18, 2016

One idea I had was to make the current api methods (addtraces, relayout, etc.) have two variants:

  1. A version with a ! (e.g. addtraces!) that would update both the Julia and Plotly objects
  2. A version without the ! (e.g. addtraces) that only updates the view.

I think we could then suggest that users use the ! methods and the goal of the Julia object being first class and always up to date is accomplished.

@MikeInnes
Copy link
Contributor

Ok, so I'll try to outline my thought process here. I think what would help a lot to start with is to draw a clear distinction between plots and views of plots, as @spencerlyon2 mentioned above.

A Plot is simply a piece of data, in Julia, which can easily be turned into a view. Plots have various methods for working with them, and in Julian tradition you'd have ! methods which mutate the Plot and regular methods that don't.

A PlotView is some rendering of a plot via plotly.js; e.g. a window at the repl, an inline result in IJulia, or a pane in Juno. A PlotView may or may not be represented as a Julia type with its own methods; e.g. it could be a thin wrapper around a Blink Window which forwards the API methods linked above to plotly.js. However, an IJulia result can't be meaningfully represented in Julia, so some PlotViews will necessarily be immutable.

In many cases, it might be convenient to synchronise views with their parent plot. I think the cleanest way to do this is to be completely explicit about it, and introduce a new type which joins the above two types together; in its simplest form:

type SyncPlot
  plot::Plot
  view::PlotView
end

SyncPlot(p::Plot) = SyncPlot(p, display(p))

plot(...) = SyncPlot(Plot(...))

addtraces!(p::SyncPlot, ...) = (addtraces!(p.plot, ...); addtraces!(p.view, ...))

I think this relieves the tension between plot as a declarative constructor and as an imperative command; make Plot the former and plot the latter. In general this is a slightly more complex design but I think the flexibility it can give is well worthwhile.

Does that seem sane to everyone? Would be great to hear what people think, whether this is the right path in general and/or there are refinements to be made.

@tbreloff
Copy link
Member

I think I understand your reasoning @one-more-minute, but from a practical perspective what does this framework allow that you couldn't easily do before? Is it primarily to enable multiple optional views? I thought Julia's multimedia io (displays, etc) already did that part. Why abstract on top of that?

One idea I had was to make the current api methods (addtraces, relayout, etc.) have two variants

When would you want to manipulate the "plot object" differently than the "view object"? I would think it makes more sense to implement Base.copy and just manipulate 2 different plots if you want them to be different.

@MikeInnes
Copy link
Contributor

Assuming the alternative is something similar to the current design -- where the plot and its views are treated as being part of the same mutable object -- there are a few reasons to be wary.

  1. It's fundamentally a very leaky abstraction, the most obvious example being that some views are immutable. By default the current API behaves quite inconsistently across the repl, IJulia etc.
  2. Plots themselves are no longer "just data" but have identity and so on. Dealing with hashing, equality, serialisation etc., and working with plots programmatically in general, gets more complicated.
  3. It makes working with different display models harder. Under the one-object design plots, views and synchronisation still exist, they're just implicit and muddled together, which makes it impossible to mix-and-match different parts. This makes it harder to integrate plot views with the plotting pane in Juno, for example.

None of these is a real deal breaker on its own, all of them could probably be worked around, and the experience for a user at the repl probably isn't going to change that much. But for people building stuff on top of PlotlyJS.jl a really solid internal design would be a big boon.

@sglyon sglyon mentioned this issue Feb 28, 2016
@sglyon
Copy link
Member Author

sglyon commented Mar 1, 2016

Hey @MikeInnes and @tbreloff I've implemented the SyncPlot idea in #21 and do think it is definitely a good change.

If you have a chance to take a look at that PR and give any comments that'd be great -- no pressure of course.

I'll close this in favor of the other one.

@sglyon sglyon closed this as completed Mar 1, 2016
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

No branches or pull requests

3 participants