Finance refactor #2561

alexcjohnson · 2018-04-16T17:06:01Z

Closes #2510 - Makes finance traces (OHLC, candlestick) into 🥇 first-class trace types.

Along with this I added selection to both of these trace types. I did not however add custom selection styles. I'd like to leave this for a later PR.

cc @etpinard

…expanded acceptance

(or more precisely test that it was already fixed)

- notice I added a legend to finance_style, so we test ohlc legend drawing - also notice that I changed ohlc line drawing to not overdraw. Directions were chosen to optimize dashes: - the ticks are drawn starting from the outside toward the middle - the main line is drawn top-to-bottom, so in principle you might not actually see the low value, but that was the case before anyway, there's really no way around this with dashes...

removing direction names & showlegend, and picking a composite name

etpinard

Fantastic PR! I've never been this happy seeing some of my old code disappear. Transformed traces are a thing of the past and should never never be resuscitated (cc #1015 and #1217).

I made a few comments, mostly to highlight things that might be hidden in big commits that I was too lazy to examine in detail.

etpinard · 2018-04-16T18:52:42Z

src/components/fx/hover.js

+        // the rest of this will still apply, so such modules
+        // can still put things in (x|y|z)Label, text, and name
+        // and hoverinfo will still determine their visibility
+        if(d.extraText !== undefined) text += (text ? '<br>' : '') + d.extraText;


Why did you have to move this block down?

Our previous users of extraText use only extraText, no x/y/z text, so they're essentially independent of the intervening block. But finance traces use extraText in place of yLabel, and still use xLabel. Contrary to the comment (that I just moved down here with extraText) previously if you had both parts, the x/y/z labels would obliterate extraText.

Thanks for the headsup 👌

etpinard · 2018-04-16T19:03:48Z

src/components/rangeslider/draw.js

@@ -455,7 +455,8 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) {
            id: id,
            plotgroup: plotgroup,
            xaxis: xa,
-            yaxis: ya
+            yaxis: ya,
+            isRangePlot: true


I don't think this range-plot range slider fix got 🔒 anywhere. We could add a case in select_test.js selects a point in the main plot and asserts that it is really that main-plot plot that gets the selected style.

Ah, thanks for reminding me about the rangeslider + selection fix! 🔒-> c77a8a3

etpinard · 2018-04-16T19:05:32Z

src/plot_api/plot_schema.js

        if(!valObject) valObject = baseAttributes[head];
+
+        // finally check if we have a transform matching the original trace type
+        if(!valObject && transforms) {


Why do we need this?

Ah good catch, this is a holdover from my original attempt to fix these traces without refactoring them. We should never again have a trace type matching a transform type, and we certainly shouldn't have a transform that doesn't exist in gd.data but gets added in during supplyDefaults. I'll take this out.

etpinard · 2018-04-16T19:06:03Z

src/plots/cartesian/constants.js

@@ -68,6 +68,7 @@ module.exports = {
        'carpetlayer',
        'violinlayer',
        'boxlayer',


Can box traces and candlestick coexist on the same subplot?

good 👀 - they probably could have before this PR, but this morning they couldn't. Now they can 0b7541e

etpinard · 2018-04-16T19:07:31Z

src/plots/cartesian/index.js

@@ -203,7 +203,7 @@ function plotOne(gd, plotinfo, cdSubplot, transitionOpts, makeOnCompleteCallback
    // remaining plot traces should also be able to do this. Once implemented,
    // we won't need this - which should sometimes be a big speedup.
    if(plotinfo.plot) {
-        plotinfo.plot.selectAll('g:not(.scatterlayer)').selectAll('g.trace').remove();
+        plotinfo.plot.selectAll('g:not(.scatterlayer):not(.ohlclayer)').selectAll('g.trace').remove();


This query is getting ridiculous.

Can we instead loop over constants.traceLayerClasses?

Can we instead loop over constants.traceLayerClasses?

Somehow we need to distinguish between trace types that know how to update and those that don't - I feel like rather than adding another category for this, we should just teach the remaining types how to update. Probably actually not that hard to do, right? Just adding the right exit() statements and moving things appropriately between enter() and each?

I wouldn't want to do that in this PR of course...

Moved to #2563

Thanks!

etpinard · 2018-04-16T19:31:41Z

src/traces/ohlc/hover.js

+    pointData.xLabelVal = di.pos;
+
+    pointData.spikeDistance = dxy(di) * spikePseudoDistance / hoverPseudoDistance;
+    pointData.xSpike = xa.c2p(di.pos, true);


From candlestick_double-y-axis, are we expected spike lines over this point to be black:

?

are we expecting spike lines over this point to be black

Yeah, that color fails the tinycolor.readability test so we just pick a color that contrasts with the background.

etpinard · 2018-04-16T19:38:53Z

src/traces/ohlc/style.js

+            d3.select(this)
+                .style('fill', 'none')
+                .call(Color.stroke, dirLine.color)
+                .call(Drawing.dashLine, dirLine.dash, dirLine.width)


In the finance_style mock, the dash line pattern makes the new split increasing/decreasing legend item look a little strange:

compare to e.g. for the ohlc_first mock with layout.showlegend: true:

which looks amazing.

Should we just not call Drawing.dashLine here?

True, it looks weird... but it also looks weird in the plot. I think that just says anyone who would really use line.dash here would want it to be a very short dash pattern, like 'dot'. Hardly anyone will use it, but I don't think we should prohibit it, nor should we draw different dashes in the legend vs the plot.

I've changed that mock to dash: 'dot' f84dfae and I have another mock in 0b7541e that shows it without dashes, so users can see the normal look.

True, it looks weird... but it also looks weird in the plot.

Very good point. f84dfae is 👌

etpinard · 2018-04-16T19:40:47Z

test/jasmine/tests/plot_api_test.js

+
+            // Even if both showlegends are false, leave trace.showlegend out
+            // My rationale for this is that legends are sufficiently different
+            // now that it's worthwhile resetting their existence to default


etpinard · 2018-04-16T19:42:38Z

test/jasmine/tests/plot_api_test.js

@@ -2781,6 +2865,53 @@ describe('Test plot api', function() {
            .then(done);
        });

+        it('can change data in candlesticks multiple times', function(done) {


Should we added add a finance trace case for the can redraw with no changes as a noop set of tests?

Should we added add a finance trace case for the can redraw with no changes as a noop set of tests?

It's there ->

plotly.js/test/jasmine/tests/plot_api_test.js

Line 2826 in 7d91d29

['finance_style', require('@mocks/finance_style.json')],

and the previous fullJson exemption was cleared in this PR -> https://github.com/plotly/plotly.js/pull/2561/files#diff-5a140ce1cc87a420496039bbf8d699f1L2913

etpinard · 2018-04-16T19:44:15Z

test/jasmine/tests/select_test.js

        .then(done);
    });

+    ['ohlc', 'candlestick'].forEach(function(type) {


So, apart from selection and the new legend items, can you list all the new features this PR adds to finance traces?

.... mostly for changelog purposes 📚

selection

new legend items (one per trace)

fully compatible with Plotly.react

OHLC works with category x data OHLC calculations are off on category axes #2004

improved event data Event data for transformed traces is inconsistent #1807

I think that's it...

…o real traces

also an image test for #2004 category OHLC and candlestick using box layout attrs

so it doesn't look like quite such a dumb feature

alexcjohnson · 2018-04-17T13:05:31Z

src/traces/splom/base_plot.js

@@ -22,7 +22,7 @@ var SPLOM = 'splom';
 function plot(gd) {
    var fullLayout = gd._fullLayout;
    var _module = Registry.getModule(SPLOM);
-    var splomCalcData = getModuleCalcData(gd.calcdata, _module);
+    var splomCalcData = getModuleCalcData(gd.calcdata, _module)[0];


@etpinard I changed the API for getModuleCalcData in 0b7541e - so that cartesian could avoid double-drawing the same trace when two trace modules share a plotting step (box and candlestick). In principle we could extend this pattern (a plot step removes the calcdata items it plotted and returns the items that are left) which could have a small 🐎 benefit when you have many traces of several types, you don't have to search the whole array each time, only the items that haven't already been handled.

👍 for 0b7541e

In principle we could extend this pattern (a plot step removes the calcdata items it plotted and returns the items that are left) which could have a small racehorse benefit when you have many traces of several types, you don't have to search the whole array each time, only the items that haven't already been handled.

Good idea here. We should keep that in mind when we'll 🔪 calcdata completely.

etpinard · 2018-04-17T13:45:00Z

src/plots/get_data.js

+            moduleCalcData.push(cd);
+        }
+        else {
+            remainingCalcData.push(cd);


Brilliantly efficient. ❤️ this.

etpinard · 2018-04-17T13:52:25Z

src/components/legend/style.js

-    .each(stylePoints);
+    .each(stylePoints)
+    .each(styleCandles)
+    .each(styleOHLC);


🚨 Non-blocking thinking-out-loud comment alert 🚨

We should perhaps make these trace module method (e.g. named _module.legendStyle) instead of having the legend component handle all trace types by itself.

Good idea, that would be way more efficient too - instead of calling every style routine on every trace and quitting if it's not the right one, just something like:

.each(function(d) { d.trace._module.legendStyle(...) })

etpinard · 2018-04-17T14:59:05Z

Amazing work 💃

alexcjohnson added 14 commits April 15, 2018 23:32

refactor candlestick into a first-class trace, also box/violin hover …

e979cf0

…expanded acceptance

refactor OHLC into a first-class trace type

84c36a9

update non-finance tests for finance refactor

3870503

fix finance tests - with a couple of bugfixes too

4c46a73

fix #2004

94c4560

(or more precisely test that it was already fixed)

fix and test finance hover labels - including test for #1807

3509b99

test finance trace select

2849ff9

🔪 hover test in finance_test - hover_label_test covers it better

be2b523

fix prereqs for transform_multi_test

5b6a7d5

update finance bundle tests

121d171

fix lf/uf vs min/max logic for box/violin/candlestick

2554482

fix #2510 - or rather, revive the test that the refactor fixed this

0d80a21

tweak and test cleanData for finance traces

82677ac

removing direction names & showlegend, and picking a composite name

alexcjohnson added bug something broken feature something new status: reviewable labels Apr 16, 2018

etpinard added this to the v1.36.0 milestone Apr 16, 2018

etpinard reviewed Apr 16, 2018

View reviewed changes

alexcjohnson added 2 commits April 16, 2018 16:30

remove plotSchema change from before we decided to refactor finance t…

71fa112

…o real traces

fix for box & candlestick together on one subplot

0b7541e

also an image test for #2004 category OHLC and candlestick using box layout attrs

alexcjohnson mentioned this pull request Apr 16, 2018

Better update pattern for cartesian SVG #2563

Closed

alexcjohnson added 5 commits April 16, 2018 19:01

change to shorter dash in finance_style mock

f84dfae

so it doesn't look like quite such a dumb feature

remove TODO that's been OK'd as is

d64fab6

undefined -> BADNUM in ohlc/calc

dc01685

Merge branch 'master' into finance-refactor

c8b03ee

update new getModuleCalcData callers to new API

302d1e6

alexcjohnson commented Apr 17, 2018

View reviewed changes

etpinard reviewed Apr 17, 2018

View reviewed changes

src/plots/get_data.js

moduleCalcData.push(cd);

}

else {

remainingCalcData.push(cd);

Copy link

Contributor

etpinard Apr 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brilliantly efficient. ❤️ this.

etpinard reviewed Apr 17, 2018

View reviewed changes

alexcjohnson added 4 commits April 17, 2018 10:11

set to null instead of delete in ohlc/calc

f498bd0

test that selection applies to plot, not rangeslider

c77a8a3

🔪 fit

3f43253

mark polar drag test flaky

13204a9

alexcjohnson merged commit 2a628df into master Apr 17, 2018

alexcjohnson deleted the finance-refactor branch April 17, 2018 14:59

etpinard mentioned this pull request Apr 17, 2018

Extra hover text not work in Candlestick charts #2481

Closed

chriddyp mentioned this pull request Aug 28, 2018

relayoutData fails with Candlestick plots plotly/dash#355

Closed

rmarren1 mentioned this pull request Aug 28, 2018

Fix #260, allow '{}' as Graph.figure plotly/dash-core-components#285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finance refactor #2561

Finance refactor #2561

alexcjohnson commented Apr 16, 2018

etpinard left a comment

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 16, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 17, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 17, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 17, 2018

etpinard Apr 16, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 16, 2018

etpinard Apr 16, 2018

etpinard Apr 16, 2018

alexcjohnson Apr 17, 2018

alexcjohnson Apr 17, 2018

etpinard Apr 17, 2018

etpinard Apr 17, 2018

etpinard Apr 17, 2018

alexcjohnson Apr 17, 2018

etpinard commented Apr 17, 2018

Finance refactor #2561

Finance refactor #2561

Conversation

alexcjohnson commented Apr 16, 2018

etpinard 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Apr 17, 2018