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

Enhancement: add property Graph.extendData to support Plotly.extendTraces #461

Merged
merged 22 commits into from Mar 25, 2019

Conversation

Projects
None yet
3 participants
@bcliang
Copy link

commented Feb 18, 2019

This merges code from https://github.com/bcliang/dash-extendable-graph into dcc.Graph()

Graph has a new property named extendData:

  • takes a list of dicts (same format as figure.data)
  • assumes list order == trace_order (does not support explicit definition of trace_order)
  • if len(extendData) > len(figure.data) = n, extend the first n traces, then add the remaining ones in the list
  • if len(extendData) = m < len(figure.data), extend first m traces

2 integration tests added to test.test_integration

Usage:

import dash
from dash.dependencies import Input, Output, State
import dash_html_components as html
import dash_core_components as dcc
import random

app = dash.Dash(__name__)

app.scripts.config.serve_locally = True
app.css.config.serve_locally = True

app.layout = html.Div([
    dcc.Graph(
        id='extendablegraph_example1',
        figure=dict(
            data=[{'x': [0, 1, 2, 3, 4],
                   'y': [0, .5, 1, .5, 0],
                   'mode':'lines+markers'
                   }],
        )
    ),
    dcc.Graph(
        id='extendablegraph_example2',
        figure=dict(
            data=[{'x': [0, 1],
                   'y': [0, .5],
                   'mode':'lines+markers'
                   },
                  {'x': [0, 1, 2, 3, 4, 5],
                   'y': [1, .9, .8, .7, .6, .5],
                   'mode':'lines+markers'
                   }],
        )
    ),
    dcc.Interval(
        id='interval_extendablegraph_update',
        interval=1000,
        n_intervals=0,
        max_intervals=25),
])


@app.callback(Output('extendablegraph_example1', 'extendData'),
              [Input('interval_extendablegraph_update', 'n_intervals')],
              [State('extendablegraph_example1', 'figure')])
def update_extend_then_add(n_intervals, existing):
    x_new = existing['data'][0]['x'][-1] + 1
    y_new = random.random()
    return [dict(x=[x_new], y=[y_new]), dict(x=[x_new], y=[random.random()])]


@app.callback(Output('extendablegraph_example2', 'extendData'),
              [Input('interval_extendablegraph_update', 'n_intervals')],
              [State('extendablegraph_example2', 'figure')])
def update_extend_first_n_traces(n_intervals, existing):
    x_new = existing['data'][0]['x'][-1] + 1
    y_new = random.random()
    return [dict(x=[x_new], y=[y_new])]


if __name__ == '__main__':
    app.run_server(debug=True)
@bcliang

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

Only committed in src/ and test/ (not what is generated in npm run build:all)

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

@bcliang thanks for the PR!

You've sparked a bit of a discussion between me and @chriddyp - it's clear that being able to extend would be a powerful feature for performance, both by limiting data sent over the wire, and in many cases reducing the work you need to do in the callback. But this would be the first case where a prop serves solely to mutate another prop, so we definitely want to be deliberate in our decision whether to introduce it. In general we don't want people thinking about mutations, preferring instead to simply describe the new state - hence our support for Plotly.react but NOT for Plotly.restyle/relayout for example. In general this makes the code easier to work with, and more robust particularly in case of multiple users of the same app.

But there are ways around this, and there's at least one pattern that seems like it will be both simple and robust in multi-user environments: Using multi-output callbacks plotly/dash#436 (expected to be merged soon and included in the next dash release) you could use a timestamp (or some other marker of "what's the last item this client has seen") your callback receives that timestamp as State and returns both the extendTraces update and a new timestamp:

@app.callback([Output('graph', 'extendData'), Output('graph-store', 'data')],
              [Input('graph-interval', 'n_intervals')],
              [State('graph-store', 'data')])
def update_graph(n_intervals, store_data):
    old_ts = store_data['timestamp']
    new_ts = datetime.now().timestamp()
    update = get_new_data(old_ts, new_ts)
    return [update, {'timestamp': new_ts}]

So, all that being said we would like to include this feature in the Graph component 🎉

I would however like to make the API follow much more closely the underlying Plotly.extendTraces API. That would mean instead of an array of trace-like objects, using an object {update, indices, maxPoints}. That enables more flexible usage (not limited to just x and y arrays - lots of trace types use other attributes for their data, and even scatter traces can have data elsewhere like marker.size - and not just the first n traces) at the expense of the implicit new trace functionality. Is the latter an important requirement, vs. simply initializing the graph with empty traces as needed?

@bcliang

This comment has been minimized.

Copy link
Author

commented Feb 18, 2019

Thanks for the comments. The only use case I've really considered was for time-series scatter-like plots (and that's how the tests were written). Most posts on the forum discussing extending data seem to be related to this use case (lots of real-time crypto/stock analytics apps).

I contemplated requiring explicit definition of trace_order but decided to skip that based on my personal use case. That being said, there's no reason not to make the change as you have suggested. What's the expected behavior of Plotly.extendTraces() if trace_order specified a trace that doesn't yet exist? I haven't personally tested it, and maybe it's just an edge case that you wouldn't typically encounter.

I don't have a ton of time to spend on this (more of a weekend hobby), so feel free to take this PR over. Otherwise, I don't mind working on the changes you've recommended. Cheers

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Feb 18, 2019

(FYI @etpinard)

I contemplated requiring explicit definition of trace_order but decided to skip that based on my personal use case. That being said, there's no reason not to make the change as you have suggested.

It might be nice to allow omitting indices (trace_order) and just take the first n traces. Ideally that would be done in plotly.js rather than here, so other applications could make use of it as well. It would also be nice to allow update items to have empty items (null/None or something) in case different keys are applicable to different traces.

What's the expected behavior of Plotly.extendTraces() if trace_order specified a trace that doesn't yet exist? I haven't personally tested it, and maybe it's just an edge case that you wouldn't typically encounter.

I haven't tested it either, but from a quick look at the code it seems like it will throw a difficult-to-interpret error. @etpinard this should perhaps be caught and explicitly thrown by positivifyIndices (along with a too-negative index)?

I don't have a ton of time to spend on this (more of a weekend hobby), so feel free to take this PR over. Otherwise, I don't mind working on the changes you've recommended.

No rush from our side, we're grateful for your contributions! If this bubbles up to the top of our priority list before you get to it we'll let you know.

@bcliang

This comment has been minimized.

Copy link
Author

commented Mar 4, 2019

@alexcjohnson I gave this some more thought.. is there a reason that Plotly.extendTraces() follows the API format of .relayout() / .update() / etc as opposed to newPlot() / react() / addTraces()? In my mind the extend operation should input an array of trace objects rather than an object that assigns an array to each key. What happens if there are different trace types on the same figure? Is there a restriction within figures that they must have a single trace type?

In my use case, when Graph.extend() is called and the figure hasn't been pre-populated, it will run Graph.plot() (or Plotly.addTraces() in the case that figure data exists, but the number of traces to extend exceed the number that exist in the figure).

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Mar 4, 2019

is there a reason that Plotly.extendTraces() follows the API format of .relayout() / .update() / etc as opposed to newPlot() / react() / addTraces()?

historical reasons - feel free to open an issue over at https://github.com/plotly/plotly.js but it'll be fairly hard to get a change like that pushed through unless there's a major gap in functionality that we can't patch in a backward-compatible way. I do NOT think though that we want to support creating traces via extendTrace, that just opens up too many strange cases, and requires bloating the call with all the other trace attributes in case the trace doesn't exist. Better to require all the traces to be present already, even if some start with empty data arrays.

What happens if there are different trace types on the same figure? Is there a restriction within figures that they must have a single trace type?

There is no such restriction, a figure can have many different trace types. The extendTraces API is certainly awkward for this use case but I suspect it can be made to work. Whether or not handles every edge case right now though, that's another question. In principle if you had something like a scatter and a choropleth, we should be able to do something like Plotly.extendTraces(gd, {y: [newY, null], locations: [null, newLocs]}, [0, 1], {y: [maxY, null], locations: [null, maxLocs]}). Again, not real pretty but seems like it should do what's needed. I haven't tested this - it may already work, but if it doesn't please file a bug report at https://github.com/plotly/plotly.js (and reference this issue for context).

@bcliang

This comment has been minimized.

Copy link
Author

commented Mar 14, 2019

Made the discussed changes.. example usage:

import dash
from dash.dependencies import Input, Output, State
import dash_html_components as html
import dash_core_components as dcc
import random

app = dash.Dash(__name__)

app.scripts.config.serve_locally = True
app.css.config.serve_locally = True

app.layout = html.Div([
    html.Div([
        html.H3('Extend a specific trace'),
        dcc.Dropdown(
            id='trace-selection',
            options=[
                {'label': 'extend trace 0', 'value': 0},
                {'label': 'extend trace 1', 'value': 1},
            ],
            value=0
        ),
        dcc.Graph(
            id='graph-extendable',
            figure=dict(
                data=[{'x': [0, 1, 2, 3, 4],
                       'y': [0, .5, 1, .5, 0],
                       'mode':'lines+markers'
                       },
                      {'x': [0, 1, 2, 3, 4],
                       'y': [1, 1, 1, 1, 1],
                       'mode':'lines+markers'
                       }],
            )
        ),
    ]),
    html.Div([
        html.H3('Extend multiple traces at once'),
        dcc.Graph(
            id='graph-extendable-2',
            figure=dict(
                data=[{'x': [0, 1, 2, 3, 4],
                       'y': [0, .5, 1, .5, 0],
                       'mode':'lines+markers'
                       },
                      {'x': [0],
                       'y': [0],
                       'mode':'lines'
                       },
                      {'x': [0, .1, .2, .3, .4],
                       'y': [0, 0, 0, 0, 0],
                       'mode':'markers'
                       }],
            )
        ),
    ]),
    dcc.Interval(
        id='interval-graph-update',
        interval=1000,
        n_intervals=0),
])


@app.callback(Output('graph-extendable', 'extendData'),
              [Input('interval-graph-update', 'n_intervals')],
              [State('graph-extendable', 'figure'),
               State('trace-selection', 'value')])
def update_extend_traces_traceselect(n_intervals, existing, trace_selection):
    x_new = existing['data'][trace_selection]['x'][-1] + 1
    y_new = random.random()
    return dict(x=[[x_new]], y=[[y_new]]), [trace_selection]


@app.callback(Output('graph-extendable-2', 'extendData'),
              [Input('interval-graph-update', 'n_intervals')],
              [State('graph-extendable-2', 'figure')])
def update_extend_traces_simult(n_intervals, existing):
    return (dict(x=[
        [existing['data'][0]['x'][-1] + 1],
        [existing['data'][1]['x'][-1] - .5, existing['data'][1]['x'][-1] + 1],
        [existing['data'][2]['x'][-1] + .1]
    ],
        y=[
        [random.random()],
        [0, random.random()],
        [random.random()]
    ]),
        [0, 1, 2]
    )


if __name__ == '__main__':
    app.run_server(debug=True)
@bcliang

This comment has been minimized.

Copy link
Author

commented Mar 23, 2019

@alexcjohnson I made changes to match the extendTraces api and synced the code with master

Show resolved Hide resolved test/test_integration.py Outdated
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

@bcliang sorry for the delay in reviewing. This is looking great! I made a couple of fairly easy comments, other than that there are a few extra cases I think would be worthwhile to test (the tests you included already look excellent BTW 🏆)

  • If we do include maxPoints and extendData=null we should test these
  • I'd like to test providing the same data twice (which can make sense if you use an implicit x axis for example, either by providing no x data or using x0 and dx). Looks like this should work already, because you're not testing the contents of extendData but rather its identity, but I'd still like a test to confirm it.
@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented on src/components/Graph.react.js in b0a0469 Mar 25, 2019

Actually I think this was better as it was before - that's the case I was concerned about where you provide the same data twice and you really want it to be added to the plot #461 (comment)

@bcliang

This comment has been minimized.

Copy link
Author

commented Mar 25, 2019

Got it. Will revert that. Will modify the test with a case confirming that the same data can be drawn twice.

@bcliang bcliang closed this Mar 25, 2019

@bcliang bcliang reopened this Mar 25, 2019

@alexcjohnson
Copy link
Contributor

left a comment

Super, thanks for following up on my comments - and of course thanks for the contribution! 💃

@alexcjohnson alexcjohnson merged commit b53cd5d into plotly:master Mar 25, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details

@bcliang bcliang deleted the bcliang:graph-extend-then-add branch Mar 28, 2019

@bcliang

This comment has been minimized.

Copy link
Author

commented Mar 31, 2019

@alexcjohnson you previously suggested that the extend property should support multiple trace types. Currently, the Plotly.extendTraces() will fail with undefined/null key values in a trace.

Something like bcliang/dash-extendable-graph#16 would work without changing the underlying library. However, it seems a waste to follow the API just to separate out the data trace-by-trace.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Mar 31, 2019

Currently, the Plotly.extendTraces() will fail with undefined/null key values in a trace.

We should definitely fix this at the plotly.js level rather than in wrapper packages. Would you make an issue for this at https://github.com/plotly/plotly.js/issues? If the syntax I described above seems reasonable to you, feel free to propose that in the issue (or propose something else you think would be better):

Plotly.extendTraces(gd,
{
  y: [newY, null],
  locations: [null, newLocs]
},
[0, 1],
{
  y: [maxY, null],
  locations: [null, maxLocs]
})
@matthijsramlab

This comment has been minimized.

Copy link

commented Apr 29, 2019

I got a multipage dash app, my code works fine. But if I switch to the other page that contains also graphs it crashes (other pages without graphs are no problem), with the following error message. What could be cause?

Edit: It worked fine with returning, the figure. It does however not work with the ExtendData.

image

@bcliang

This comment has been minimized.

Copy link
Author

commented Apr 29, 2019

Appears to be an issue with how the component attempts to automatically generate the traceIndices array. Your stacktrace suggests the object being returned in the callback isn't formed properly.

Can you confirm that the callback returns a dictionary of the form:

dict(
    key1=[[trace1_values], [trace2_values], ..], 
    key2=[[trace1_values], [trace2_values], ..],
)

.. and that the dictionary doesn't include any "non-data" keys?

In your callback, you can also try explicitly defining the trace #(s) that you want to extend (skip the auto-generation logic).. Instead of return extendData, something like return (extendData, [0])

@matthijsramlab

This comment has been minimized.

Copy link

commented Apr 30, 2019

Okay, I tried everything and it works know. Instead of using the dash core components Graph, I am using your https://github.com/bcliang/dash-extendable-graph component.

Further description of the problem:

I have a multi-page web app. I click on the link which takes me to the page containing the graphs. No errors so far. Then when I click on the same link again, I get the error. This error occurs when the page layout is returned.

The layout of the page:

def get_layout(robot_name, robot):
	layout = html.Div([
		dcc.Interval(id='robot-details-updater'),
		time_buttons(),
		dcc.Graph(
			id='robot-voltage-graph',
			figure={
				'data' : [{
					'x': [],
					'y': [],
					'mode': 'lines'
					}],
			},
			),
		dcc.Graph(
			id='robot-current-graph',
			figure={
				'data': [{
					'x': [],
					'y': [],
					'mode': 'lines'
				}],
			},
		),
		html.Div(robot_name, hidden=True, id='robot_name')
	])

	return layout
def register_callbacks(app, data_provider):
	@app.callback(
		[
			Output('robot-voltage-graph', 'extendData'),
			Output('robot-current-graph', 'extendData'),
		],
		[
			Input('robot-details-updater', 'n_intervals')
		],
		[
			State('robot_name', 'children'),
			State('time_dropdown', 'value'),
		])
	def update_live_graph_by_dropdown(n_intervals, robot_name, time):
		
		if not dash.callback_context.triggered:
			raise dash.exceptions.PreventUpdate
		
		if not robot_name:
			raise dash.exceptions.PreventUpdate
		
		if not data_provider.online[robot_name]['online']:
			raise dash.exceptions.PreventUpdate
		
		time_data_length = 60
		if time == '1':
			time_data_length = 60
		elif time == '5':
			time_data_length = 60*5
		elif time == '10':
			time_data_length = 60*10

		# Get the latest data
		latest_data = data_provider.get_latest_data(robot_name, time_data_length)
		time_data = []
		voltage_data = []
		current_data = []
		for data in latest_data:
				time_data.append(datetime.fromtimestamp(float(data['loggingTime'])))
				voltage_data.append(float(data['welder']['params']['act']['voltage']))
				current_data.append(float(data['welder']['params']['act']['current']))
		
		return (dict(x=[time_data], y=[voltage_data]), [0], len(time_data)), (dict(x=[time_data], y=[current_data]), [0], len(time_data))

When I use your component, the syntax of the callback seems different:
return [dict(x=time_data, y=voltage_data)], [0], len(time_data)

Could this be the problem? If I use the same syntax for the dash core component, I get an error.

Ps. This component you made is really nice. It allows me to preserve the graphs state, and the zoom options work. So great work!

@bcliang

This comment has been minimized.

Copy link
Author

commented Apr 30, 2019

Each dictionary key should contain a list of lists, even if you're only extending a single trace. This is a direct match for the plotly.extendTraces() API.

Try this as the return of your callback when using dash-core-components Graph:

return (dict(x=[[time_data]], y=[[voltage_data]]), [0], len(time_data)), (dict(x=[[time_data]], y=[[current_data]]), [0], len(time_data))

As you've mentioned, dash-extendable-graph's API data object is actually a list (or tuple) of dicts -- each dict defined for a trace that you want to extend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.