Dm 10833 multitrace charts in tri-view #474

tgoldina · 2017-10-10T20:04:39Z

https://jira.lsstcorp.org/browse/DM-10833
https://jira.lsstcorp.org/browse/DM-9072

To use multi-trace chart architecture, add charts: {chartEngine: 'plotly', multitrace: true} to window.firefly.app.options. For testing purposes, firefly.html and firefly/demo/ffapi-highlevel-charttest.html have these settings.

This pull request includes:

Supporting default multi-trace charts in triview
The default chart parameters can be passed via META_INFO of the table. Other than that the default chart behavior is the same as before per Xiuqin's request.
Creating multi-trace charts from Charts dropdown
Multi-trace charts error handling, loading indicator, deletable behavior, help_id
Bug fixes:
1. When trace color and alpha are changed, change all color attributes (marker, line, error bars, etc).
2. Make sure the default color is reset after color map option is undone. (Color map option is making marker.color an array, the default color is lost.)
3. Heatmap reversescale option was not always observed
4. Use scatter-gl and non-gl consistently. The two do not work well together.
Reworked ChartsContainer, which now supports tbl_group, and will display a default chart for active table in that group. ChartsContainer is the owner of the sagas, updating chart viewer items and creating the default charts. (The hope is to get rid of ChartsSync and its master sagas.

The test case for tbl_group tracking is added to ffapi-highlevel-charttest

I am sure something fell through the cracks. I am looking forward to your feedback.

Important changes:
html/demo/ffapi-highlevel-charttest.html
ApiHighlevelBuild.js
ChartsContainer
ChartSelectDropdown
BasicOptions

loitly

A chart with a histogram and a scatter trace. When scatter is hidden, the select point is still visible. Shoud not be.

loitly · 2017-10-10T21:03:42Z

src/firefly/js/charts/ChartUtil.js

@@ -163,7 +169,7 @@ export function getNumericCols(cols) {
 * @prop {string}  [chartTitle] title of the chart
 * @prop {string}  xCol         column or expression to use for x values, can contain multiple column names ex. log(col) or (col1-col2)/col3
 * @prop {string}  yCol         column or expression to use for y values, can contain multiple column names ex. sin(col) or (col1-col2)/col3
- * @prop {string}  [plotStyle]  points, linepoints, line
+ * @prop {string}  [plotStyle]  points, linepoints, liine


'i' accidentally inserted here.

loitly · 2017-10-10T21:13:37Z

src/firefly/js/charts/TableStatsCntlr.js

+    const {tbl_id} = searchRequest;
+    // use tblFilePath to determine if a call needs to be placed
+    const tblFilePath = get(flux.getState(), [TBLSTATS_DATA_KEY, tbl_id, 'tblFilePath']);
+    const tblFilePathNow = get(TableUtil.getTblById(tbl_id), 'tableMeta.tblFilePath');


a reminder.. once DM-11814 merged, you should use resultSetID instead of tblFilePath

loitly · 2017-10-10T21:22:09Z

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

+        switch (action.type) {
+            case TABLE_LOADED:
+                const {tbl_id} = action.payload;
+                if (getChartIdsInGroup(tbl_id).length === 0) {


getChartIdsInGroup(groupId) is always called with a tbl_id. Can you explain this?

loitly · 2017-10-10T21:30:04Z

src/firefly/js/templates/fireflyviewer/FireflyViewer.js

@@ -27,6 +27,8 @@ import FFTOOLS_ICO from 'html/images/fftools-logo-offset-small-75x75.png';

 // import {deepDiff} from '../util/WebUtil.js';

+const isDebug = () => get(window, 'firefly.debug', false);


newly added function not used anywhere.

loitly · 2017-10-10T21:46:28Z

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


        if (chartId) {
            return expandedMode ?
                <ExpandedView key='chart-expanded' closeable={closeable} chartId={chartId}/> :
                <ChartPanel key={chartId} expandable={true} chartId={chartId}/>;
        } else {
            return (
-                <MultiChartViewer closeable={closeable} viewerId={viewerId || DEFAULT_PLOT2D_VIEWER_ID} expandedMode={expandedMode}/>
+                <MultiChartViewer {...{closeable, viewerId, expandedMode, monitor}}/>


Just curious, why does monitor has to be passed to MultiChartViewer? All of the logic is here. Why not handle mount/unmount here as well?

loitly

It seems to be working good. I don't see anything stopping this from merging.

loitly

On resize, the top of the chart may be chopped off, especially when it's being expanded vertically.

tgoldina · 2017-10-11T20:16:27Z

@loitly I have fixed the issues found so far, except the resize issue, which I can not reproduce. (I have reduced the top and bottom margins to match the previous tri-view look and to better use the container space. Without the title, the margin on the top is 10px.

ejoliet

looks awesome!
it looks really similar in look and feel and i didn’t see any issues while playing around from the demo page.

i think is ready to show it to IRSA for more feedback… but i don’t see any showstopper for merging from my side.
great job!

robyww · 2017-10-12T18:14:07Z

UI Notes:

If we can add a trace we need to be able to remove it.
The charts panel needs to choose between adding a trace and adding a new chart.
Selected does not seem to work with two traces. to repeat: put two traces up both ra/dec and select and click on select.
image overlay should clear when a filter with no data found (this is probably not your problem)
would be nice to have a warning dialog with there are two traces and the user attempts to filter the non-active trace. This will result in a "no data found" and probably not what the user wants.

robyww

The code look good. I would like you to look over my UI comments. I think some of that needs to be fixed or changed before the PR is ready. We can talk about any that you think are to big.

xiuqin · 2017-10-12T19:35:30Z

when I changed the X/Y to different columns, the range of XY plot did not change (stayed at ra/dec range). The ranges need to be reset so the proper plot will show up. Even after I set the min/max values, I still don't the dots in the plot.
After I added another trace, the range was changed but did not match with either trace data.

xiuqin · 2017-10-12T22:45:59Z

Here are the suggestions after discussing with Trey:

Change the slide-down menu associated with gear icon to the dialog associated with clicking on "Charts".
Modify the dialog with the following:

following the image dialog, give options "new chart", "add to existing chart" (it will be like adding a data series). When it comes up first without existing chart, "New chart" should be the only option.
Change button label "ADD" to "OK" to cover both options.
For "add to existing chart", the trace field could be filled with the default label as "add series" currently does.
Use the same color schema as Image and Catalog dialogs, adding ? for help.

tgoldina · 2017-10-13T15:20:25Z

I think that removing chart trace and modifying charts dropdown possibly replacing current trace settings should be two separate tickets. The last one in particular might need to be clarified.

I agree that chart options are getting too crowded and hard to navigate. Also, we have no way to create new charts from API. Replacing settings on the chart side with a dialog would give us more space and we'll be able to add charts from API. (And it would be consistent with how we deal with images.)

So, the proposed dialog would have up to 3 options:

Modify active trace, Add new trace, Add new chart

I. First, we need to understand which chart viewer these options will be referring to.

The first two (modify active trace, add new trace) would be referring to the active chart of the default chart viewer (if triggered from charts dropdown menu) or to the current chart's viewer if triggered from the chart toolbar.

The last one (create new) would be adding a chart to the default viewer, if triggered from charts dropdown menu and to the chart's viewer, if triggered from the chart toolbar.

II. Second, 'Add new trace' and 'Add new chart' options only make sense if there is an active table.

In Firefly viewer scenario, an active table is the currently active table of the table main group.

How do we know what the active table is when a chart is created from an API, and different traces may possibly reference different tables?

We can decide that the active table is the table referenced in the current active trace of the chart.

Or we can decide, that if chart viewer has tbl_group property, the active table is the one currently active in this group.

If we can not find the active table, we don't display 'Add new trace' and 'Add new chart' options.

OK, it looks like a good plan, but I would still put it in a separate ticket. :)

tgoldina · 2017-10-17T15:22:26Z

The following bugs found in review are fixed:

Selected does not seem to work with two traces. to repeat: put two traces up both ra/dec and select and click on select.
Sticky range, which caused new trace not to be visible after the original was panned or zoomed.
(Now we clear the axis range in chart layout whenever the new x or y field is submitted via the chart options.)

I have merged this branch into dev to avoid merging issues in the future. Also, having it in dev would help finding bugs faster and getting more suggestions. You can always switch back by setting window.firefly.app.optionscharts.multitrace to false.

I'll move all outstanding suggestions into a separate ticket.

tgoldina requested review from loitly, ejoliet and robyww October 10, 2017 20:04

loitly reviewed Oct 10, 2017

View reviewed changes

loitly approved these changes Oct 10, 2017

View reviewed changes

loitly reviewed Oct 10, 2017

View reviewed changes

ejoliet approved these changes Oct 11, 2017

View reviewed changes

robyww requested changes Oct 12, 2017

View reviewed changes

robyww assigned tgoldina Oct 12, 2017

DM-10833 multi-trace charts as the default charts in triview

b1b8a14

tgoldina force-pushed the dm-10833_multitrace_triview branch from 205be8e to b1b8a14 Compare October 16, 2017 23:35

tgoldina added 2 commits October 16, 2017 19:31

DM-10833 multi-trace charts as the default charts in triview

ba416a2

statistics tracing resultSetID

50222e1

tgoldina merged commit 9c9fdf3 into dev Oct 17, 2017

tgoldina deleted the dm-10833_multitrace_triview branch October 25, 2017 15:58

		@@ -27,6 +27,8 @@ import FFTOOLS_ICO from 'html/images/fftools-logo-offset-small-75x75.png';

		// import {deepDiff} from '../util/WebUtil.js';

		const isDebug = () => get(window, 'firefly.debug', false);

Dm 10833 multitrace charts in tri-view #474

Dm 10833 multitrace charts in tri-view #474

Uh oh!

Conversation

tgoldina commented Oct 10, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

loitly Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

loitly Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

loitly Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

loitly Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

loitly Oct 10, 2017

Choose a reason for hiding this comment

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

loitly left a comment

Choose a reason for hiding this comment

Uh oh!

tgoldina commented Oct 11, 2017

Uh oh!

ejoliet 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!

robyww commented Oct 12, 2017

Uh oh!

robyww left a comment

Choose a reason for hiding this comment

Uh oh!

xiuqin commented Oct 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xiuqin commented Oct 12, 2017

Uh oh!

tgoldina commented Oct 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgoldina commented Oct 17, 2017

Uh oh!

Uh oh!

tgoldina commented Oct 10, 2017 •

edited

Loading

ejoliet left a comment •

edited

Loading

xiuqin commented Oct 12, 2017 •

edited

Loading

tgoldina commented Oct 13, 2017 •

edited

Loading