DM-10462 Adapt scatter plot for multi-trace chart architecture #391

tgoldina · 2017-05-30T22:32:20Z

Test page:
http://localhost:8080/firefly/demo/ffapi-highlevel-charttest.html
(w[1,2,3,4]mpro and w[1,2,3,4]sigmpro are good fields to plot)

Major changes:

Made Plotly chart a part of MultiChartContainer (shared options, taking all length of the container)
Used XYPlotWithErrorProcessor for a proof of concept how the derived data can be handled in the chart. tablesource now stores mappings, optional function fetchData and last fetch options. The data fields that can not be mapped to known plotly trace data are stored in data[traceNum].firefly object.
The derived table can be smaller (less rows) than the original table. data[traceNum].firefly.rowIdx provides the connection between the original table rows and chart points. Chart highlight/selection is using point indexes, table highlight/selection - table row indexes.
Handling highlight of a selected point. plotly_onclick only gives the topmost point clicked. Since we do not store selected and highlighted traces, I had to use plotly trace data when active trace is not matching the clicked point trace.
Updated plotly to the latest version (1.27.1) Unfortunately, onclick still returns only the topmost point. (According to plotly_click and plotly_select buggy on scatter/markers plot plotly/plotly.js#1648 it should be fixed. Am I missing something?)
Adjusted data flow to avoid data refetch, when the updates to data do not require server call (ex. marker color or shape change) Since the updates are passed as the changes to the data, they overwrite existing data. I save old data and pass them to the table connector to be able to restore them. (Maybe, we can come with a better solution later.)
Made adjustments to avoid double updates on highlight and select. Before, chart was triggering table events directly and responded to table events. Now we have chart specific dispatchers which update the chart AND trigger table action. (Added chartTrigger to action payload.)
Added Save [Image] button to the new plotly chart toolbar. It turned out that in API we can have 2 components with the same chart id - one expanded, one non-expanded - adjusted the method to handle both.
Updated ScatterOptions to use table statistics and column lookup and expressions. Added UI to define errors. (ScatterOptions still need a lot of work.)

…rchitecture

loitly

We've already talked on the phone in regard to this PR. Most of the comments made are prior to the call. Feel free to ignore it if you like. We agreed that this PR can be merged as is.

loitly · 2017-05-31T16:20:17Z

src/firefly/html/demo/plotly-concepts.html

@@ -9,7 +9,7 @@
    <link rel=”apple-touch-startup-image” href=”images/fftools-ipad_splash_768x1004.png”>
    <title>Plotly Concept</title>

-    <script  type="text/javascript" src="../plotly-1.25.0.min.js"></script>
+    <script  type="text/javascript" src="../plotly-1.27.1.min.js"></script>


A reminder for everyone as well, when we update plotly lib, it needs to be replaced in other repositories as well, ie ife, suit.

Can't we not having an alias and get it from somewhere else that we maintain separately? github or directly from its CDN?

loitly · 2017-05-31T16:23:14Z

src/firefly/js/charts/ChartUtil.js

-import {getTblById, getColumnIdx, getCellValue, cloneRequest, doFetchTable, watchTableChanges, MAX_ROW} from '../tables/TableUtil.js';
-import {TABLE_FILTER, TABLE_SORT, TABLE_HIGHLIGHT, TABLE_SELECT, TABLE_REMOVE} from '../tables/TablesCntlr.js';
+import {getTblById, getColumnIdx, getCellValue, cloneRequest, doFetchTable, isFullyLoaded, watchTableChanges, MAX_ROW} from '../tables/TableUtil.js';
+import {TABLE_HIGHLIGHT, TABLE_LOADED, TABLE_SELECT, TABLE_REMOVE} from '../tables/TablesCntlr.js';


I am trying to make use to SORT and FILTER directly instead of relying on invokedBy in LOADED. This cannot be done using SORT or FILTER?

loitly · 2017-05-31T16:26:59Z

src/firefly/js/charts/ChartUtil.js

+ * @returns {number}
+ */
+export function getRowIdx(traceData, pointIdx) {
+    // firefly.rowIdx array in the trace data connects plotly points to table row indexes


data should be a pure plotly structure. Can this reference be moved to tableSources?

also, I thought the data points can have a one-to-one relationship with the same indices. I am not sure this is needed.

loitly · 2017-05-31T16:47:13Z

src/firefly/js/charts/ChartUtil.js

+
+        // fetch data
+        if (tablesource.fetchData) {
+            tablesource.fetchData();


This may be a problem in the near future. Functions in the store cannot be saved/restored via json string. We may need to find a different approach.

loitly · 2017-05-31T16:53:32Z

src/firefly/js/charts/ChartUtil.js

-
+export function getTraceTSEntries(traceTS, chartId, traceNum) {
+    const {data} = getChartData(chartId) || {};
+    const chartDataType = get(data[traceNum], 'firefly.dataType');


Same as before, data object should not be used to store non-plotly data. We could introduce another object under chartData for this.

loitly · 2017-05-31T17:04:32Z

src/firefly/js/charts/ChartsCntlr.js

        const ttype = get(data, [activeTrace, 'type'], 'scatter');

        if (!isEmpty(tablesources) && ttype === 'scatter') {
-            const {tbl_id} = tablesources[activeTrace] || {};
+            // activeTrace is different from activeDataTrace if a selected point highlighted, for example


I don't understand this. I'll talk to you offline about it.

loitly · 2017-05-31T19:32:32Z

src/firefly/js/charts/ui/options/NewTracePanel.jsx

+             fieldKey: `_tables.data.${activeTrace}.error_x.arrayminus`,
+             value: get(tablesourceMappings, ['error_x.arrayminus'], ''),
+             //tooltip: 'X error',
+             label: 'X Err\u2193:',


Err instead of error, is that on purpose or typo?

tgoldina · 2017-05-31T22:16:56Z

Thank you for the comments. Here are some clarifications.

I am trying to make use to SORT and FILTER directly instead of relying on invokedBy in LOADED. This cannot be done using SORT or FILTER?

For larger tables that don't load right away, I need to watch TABLE_LOADED. If I also watch TABLE_SORT and TABLE_FILTER – they are preceded by TABLE_LOADED – I will trigger the data fetch twice when we filter or sort the table.

rowIdx – I thought the data points can have a one-to-one relationship with the same indices. I am not sure this is needed.

It's not always needed, but we might have less chart points that table rows and still want to preserve connection between them. At the moment our search processor is not returning the data with missing x or y. Another example: decimated chart point only connects to one (representative) table row. In future, we might be skipping some rows or doing the aggregation by rows rather than columns, so this feature is useful.

activeTrace is different from activeDataTrace if a selected point highlighted, for example - I don't understand this

Suppose we are highlighting a selected point and out layout has an attribute hovermode: 'closest'. Then plotly_onclick event data would only contain the topmost point. CurveNum will be the selected trace number, which does not have the tablesource associated with it.

…efore updating.

…selected and highlighted error bar colors matching marker, other bug fixes

tgoldina · 2017-06-01T02:48:29Z

Two items moved to TODO list:

separating firefly specific things from plotly trace data
not storing fetchData function in tablesource (though I realized that _cancel is also a function which we store in the tablesource)

ejoliet · 2017-06-01T18:29:45Z

Please @tgoldina , raise ticket for those items if not yet done. Thanks!

DM-10462 Adapt scatter plot to use with the multitrace Plotly chart a…

4d33a8f

…rchitecture

tgoldina requested a review from loitly May 30, 2017 22:32

loitly approved these changes May 31, 2017

View reviewed changes

loitly and others added 2 commits May 31, 2017 17:19

remove the need for oldData by omiting table's references from data b…

b5f3648

…efore updating.

fixed "too much recursion", added filter button, added props to have …

d5d0507

…selected and highlighted error bar colors matching marker, other bug fixes

tgoldina merged commit 21904a2 into dev Jun 1, 2017

tgoldina deleted the DM-10462_scatter branch July 5, 2017 17:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-10462 Adapt scatter plot for multi-trace chart architecture #391

DM-10462 Adapt scatter plot for multi-trace chart architecture #391

tgoldina commented May 30, 2017

loitly left a comment

loitly May 31, 2017

ejoliet May 31, 2017

loitly May 31, 2017

loitly May 31, 2017

loitly May 31, 2017

loitly May 31, 2017

loitly May 31, 2017

loitly May 31, 2017

tgoldina commented May 31, 2017 •

edited

Loading

tgoldina commented Jun 1, 2017

ejoliet commented Jun 1, 2017

DM-10462 Adapt scatter plot for multi-trace chart architecture #391

DM-10462 Adapt scatter plot for multi-trace chart architecture #391

Conversation

tgoldina commented May 30, 2017

loitly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgoldina commented May 31, 2017 • edited Loading

tgoldina commented Jun 1, 2017

ejoliet commented Jun 1, 2017

tgoldina commented May 31, 2017 •

edited

Loading