DM-11477 Make plot.ly plotting API to handle Firefly unrecognized charts types cleanly #454

cwang2016 · 2017-09-06T21:02:28Z

This development includes:

render plotly charts by giving any plotly data and layout JSON description.
put Firefly like tool bar for each plotly chart either recognized or unrecognized by Firefly. The tool bar rendering is classified into the following groups,

basic tool bar with buttons for
download chart as PNG + show/edit filter + chart options and tools + Expand panel (for Pie)
1 + buttons for
Zoom to the original range + Zoom + Pan (for Histogram, bar, box)
2 + buttons for select (for each trace with x, y data directly mapped to table numerical columns, like scatter, histogram2d, histogram2dcontour, heatmap)
2 + buttons for orbital rotation + turntable rotation
(for 3D-like charts, such as scatter3d, surface, mesh3d)

update the chart options and tools side bar content for Firefly unrecognized charts.
update the logic for XY options, XY range, marker color, etc.
fixed the issue that the mouse actions on the side bar may inaccurately move the entire grid unit of that chart.

Test:
run demo/ffapi-slate-test3.html to get the following chart samples:
histogram, surface, bar, pie, histogram2dcontour and box.
The chart samples are made for showing purpose with no significant meanings.

…s not interpreted by Firefly

…XY options for chart options side bar.

tgoldina · 2017-09-07T19:15:13Z

src/firefly/js/charts/ui/PlotlyToolbar.jsx

@@ -44,6 +45,38 @@ export class ScatterToolbar extends SimpleComponent {
    }
 }

+function isSelectable(tbl_id, chartId, type) {


I don't quite understand how we decide that selection box should be present. We might be creating unnecessary complexity here. What if x and y are expressions? I think we should not allow selection box for "unknown chart types".

Yes, we need to determine

if showing the selection box for 'unknown chart types'?
I had asked the original design intention if showing XY options in case x, y data is defined for 'unknown chart types', and the reply is 'the intention was to exclude XY options from the charts that do not have XY axes', so similarly, should selection box be excluded if the chart which is 'unknown type' but with x, y data?

If not excluded, selection box is shown in case XY is defined and mapped to table columns directly (per discussion with Trey). Selection box could be rendered if x, y is expressions later once we add new columns into the table for the expression.

This is thought that makes the code so far.

The problem is that firefly enhanced charts might be creating the x and y arrays using custom logic bypassing mappings. May be we can check fireflyData.type here: for fireflyHeatmap we do want to support area selection?

Also you can use getColValidator in ColumnOrExpression.jsx to check for column expressions. TableStatsCntlr.getColValStats(tblId) will return table statistics.

Please check ffapi-slate-test2.html, the selection area is not changed for heatmap and scatter cases. (the firefly chart type implemented is not changed). In test2 example, originally scatter chart shows no selection box, and heatmap charts shows the selection box.

The area selection issue for heatmap in ffapi-highlevel-charttest.html is solved by adding table columns info into the state for PlotlyToolBar for the purpose of rerendering the tool bar when the table loading is complete.

tgoldina

Thank you for testing table connection to other chart types and working through the issues.

I noticed that selection box is not present on the toolbar for firefly scatter and heatmap anymore. This needs to be fixed.

Also noticed that boundaries do not seem to work, but this bug must have been introduced earlier.

tgoldina · 2017-09-07T19:24:59Z

src/firefly/js/charts/ui/PlotlyWrapper.jsx

@@ -236,10 +243,10 @@ const now = Date.now();
                        Plotly.Plots.resize(this.div);
                        break;
                    case RenderType.UPDATE:
-                        Plotly.update(this.div, data, layout);
+                        Plotly.update(this.div, data, cloneDeep(layout));


Why do we need cloneDeep here? I am worried that it will mess optimize method logic, which checks for the differences between our layout and chart layout. We do expect our layout object to be in sync with the chart's layout.

the layout object gets changed after calling Plotly function for bar chart case and that make the logic confliction in submitChanges of BasicOptions. This happens while calling Plotly.newPlot. We could talk in more detail about this solution.

Cindy added the cloneDeep because plotly was adding properties to the layout object. It does not treat it as immutable data. If this is a problem the the optimize then we have a difficult problem. Who can tell us for sure that it is a problem with optimize?

tgoldina · 2017-09-07T19:49:57Z

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

+        const noXY = hasNoXY(type, tablesource);
+        const xColType = noXY ? '' : getColumnType(getTblById(tbl_id), get(tablesource, ['mappings', 'x'], ''));
+        const yColType = noXY ? '' : getColumnType(getTblById(tbl_id), get(tablesource, ['mappings', 'y'], ''));
+        const xNoLog = type.includes('histogram') ? true : undefined;          // histogram2d or histogram2dcontour


I don't think noLog option should be here. This was for firefly histogram: since our bin min/max options did not work well when switching to log, we did not want to display log option. For generic XY chart, we should have log option for both axes.

for some cases if the log is checked, the plotly rendering run into some problems.

if the column is 'string' type

histogram2d, histrogram2dcontour.

so the chart type and table column type for x & y are considered to be checked, but not sure if this check is needed in case no XY options is shown in the side bar for unknown type chart.

tgoldina · 2017-09-07T19:56:37Z

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

+function hasNoXY(type, tablesource) {
+    if (type.endsWith('3d') || ['pie', 'surface'].includes(type)) return true;
+
+    return (!get(tablesource, ['mappings', 'x']) || !get(tablesource, ['mappings', 'y']));


Why do we need to check mappings here? XY options control layout, not data.

In general, I think BasicOptions should not be aware of what is in tablesource beyond tbl_id.

I guess this is a question related to if showing XY options for unknown type chart.

for example, histogram2dcontour has data x, y defined.
pie has no data x, y defined, (it has labels and values)

Please give more thoughts if there is any missing.

cwang2016 · 2017-09-07T20:58:15Z

Per the comments, please help clarify the following specs.:

if rendering selection box for unknown type chart?
what is the logic for rendering the selection box?
if showing XY options in chart options and tools side bar for unknown types?
what is the logic for rendering each XY options? (grid, reverse, top/right, log)
and xmin/xmax, ymin/ymax
(thought: if x, y is related to 'string' or 'char' type column, log and min/max range could be excluded)

robyww

Loi might have the solution to the cloneDeep problem. I think you, he and tatiana need to talk. I advised you wrong on the cloneDeep solution.

cwang2016 · 2017-09-07T21:35:53Z

@robyww
do you see zoom, pan in pie chart? there is no zoom, pan and 1x in the tool bar
for pie chart.

related to controlling event propagation when in the the slate view.

… Exclude XY options from the chart options side bar for 'bar' chart.

…firefly into DM-11477-plotlyAPI

cwang2016 · 2017-09-08T01:31:07Z

Add some updates:

fix the issue regarding selection area in the toolbar for heatmap chart -- some timing issue between the table loading and tool bar rendering.
exclude XY options from BasicOptions for 'bar' chart.
remove deepClone while passing 'layout' to Plotly function calls.

…ptions.

Cindy Wang added 2 commits September 6, 2017 11:48

DM-11477 Make plotly plotting API to handle Plotly chart type which i…

93d8631

…s not interpreted by Firefly

DM-11477 update the logic for rendering tool bar 'select' button and …

c4ad483

…XY options for chart options side bar.

cwang2016 requested review from robyww and tgoldina September 6, 2017 21:02

cwang2016 self-assigned this Sep 6, 2017

tgoldina reviewed Sep 7, 2017

View reviewed changes

tgoldina requested changes Sep 7, 2017

View reviewed changes

robyww requested changes Sep 7, 2017

View reviewed changes

robyww and others added 5 commits September 7, 2017 15:47

DM-11477: fixed: highlighting charts stopped working, this is

04e642e

related to controlling event propagation when in the the slate view.

DM-11477 Update the toolbar state when the table loading is complete.…

2f98836

… Exclude XY options from the chart options side bar for 'bar' chart.

Merge branch 'DM-11477-plotlyAPI' of https://github.com/Caltech-IPAC/…

3d02134

…firefly into DM-11477-plotlyAPI

DM-11477 clean the code

afc8fb7

DM-11477 remove unneed import.

0112ae8

Selection mode should be available for scatter

44af9dd

tgoldina approved these changes Sep 8, 2017

View reviewed changes

robyww approved these changes Sep 8, 2017

View reviewed changes

DM-11477 add column expression for filtering and XY options in BasicO…

2eccdb7

…ptions.

cwang2016 merged commit aad9b7e into dev Sep 8, 2017

cwang2016 deleted the DM-11477-plotlyAPI branch September 8, 2017 21:27

DM-11477 Make plot.ly plotting API to handle Firefly unrecognized charts types cleanly #454

DM-11477 Make plot.ly plotting API to handle Firefly unrecognized charts types cleanly #454

Uh oh!

Conversation

cwang2016 commented Sep 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwang2016 Sep 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwang2016 Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgoldina left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwang2016 commented Sep 7, 2017

Uh oh!

robyww left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cwang2016 commented Sep 7, 2017

Uh oh!

cwang2016 commented Sep 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cwang2016 commented Sep 6, 2017 •

edited

Loading

cwang2016 Sep 7, 2017 •

edited

Loading

cwang2016 Sep 8, 2017 •

edited

Loading

robyww left a comment •

edited

Loading

cwang2016 commented Sep 8, 2017 •

edited

Loading