OHLC and Candlestick trace types #1020

etpinard · 2016-10-07T03:34:34Z

This PR adds two new trace types the plotly.js: ohlc (short for open-high-low-close) and candlestick to the main plotly.js bundle - making plotlyjs-finance and the plotly.py figure factories obsolete.

This PR also adds a new partial bundle called plotly-finance which includes ohlc and candlestick along with a few other trace type. We should come to an agreement about which trace type to include before this PR is done.

On the technical side of things, this PR introduces a new way of adding trace types: using transforms. In essence, a ohlc trace is transformed internally into one increasing and one decreasing line trace. Similarly, a candlestick trace is transformed transformed internally into one increasing and one decreasing box trace.

etpinard · 2016-10-07T03:36:44Z

This PR is still very much a WIP.

TODO:

It needs Range slider second iteration (part 2: on-par range plots !!!) #1017 to be merged for range slider to work properly with ohlc and candlestick
feedback on attributes name and defaults
mucho tests ❗

etpinard · 2016-10-07T03:43:26Z

@jackparmer @chriddyp @alexcjohnson @cldougl (if not on vaca) would you mind looking at the attribute declarations in

for feedback

Plotly.plot('graph', [{ 
  type: 'ohlc',
  open: [33.01, 33.31, 33.50, 32.06, 34.12, 33.05, 33.31, 33.50],
        high: [34.20, 34.37, 33.62, 34.25, 35.18, 33.25, 35.37, 34.62],
        low: [31.70, 30.75, 32.87, 31.62, 30.81, 32.75, 32.75, 32.87],
        close: [34.10, 31.93, 33.37, 33.18, 31.18, 33.10, 32.93, 33.70],
  t: ['2016-09-01', '2016-09-02', '2016-09-03', '2016-09-04', '2016-09-05', '2016-09-06', '2016-09-07', '2016-09-10']
}]);

currently gives

forget about the rangeslider for now, PR #1017 will fix the issue

The same data with trace: 'candlestick' gives

etpinard · 2016-10-07T03:43:49Z

lib/index-finance.js

+    require('./histogram'),
+    require('./pie'),
+    require('./ohlc'),
+    require('./candlestick')


@mdtusz @rreusser thoughts on this one?

etpinard · 2016-10-07T03:44:18Z

src/plots/plots.js

@@ -830,6 +830,7 @@ function applyTransforms(fullTrace, fullData, layout) {
                fullTrace: fullTrace,
                fullData: fullData,
                layout: layout,
+                fullLayout: fullLayout,


FYI @bpostlethwaite

sweet I was going to ask for this :) Now I can grab the axis.type

In fact this might allow the whole Streambed Plotly.js build to stop requiring special Axes API.

cldougl · 2016-10-07T10:56:53Z

src/traces/ohlc/attributes.js

+            'If a single string, the same string appears over',
+            'all the data points.',
+            'If an array of string, the items are mapped in order to the',
+            'this trace\'s sample points.'


extra the/this

cldougl · 2016-10-07T12:11:46Z

src/traces/ohlc/attributes.js

+        description: 'Sets the close values.'
+    },
+
+    // TODO find better colors


if it helps we used:

increasing = '#3D9970'
decreasing = '#FF4136'

in the python wrapper (but that red is still pretty bright)

done in 7b19b74

alexcjohnson · 2016-10-07T14:06:10Z

src/traces/candlestick/transform.js

+        boxpoints: false,
+
+        // TODO could do better
+        name: direction,


what happens if you try to edit this by clicking in the legend?

Hmm. Good question. Let me check.

handled in 2b1918c

alexcjohnson · 2016-10-07T14:12:55Z

src/traces/candlestick/attributes.js

+var boxAttrs = require('../box/attributes');
+
+var directionAttrs = {
+    visible: {


Is that actually something people want, to be able to show only one direction? Seems weird. I can imagine wanting to delete one of them from the legend, like, if I had candlesticks for several companies and I set the colors such that I could tell immediately which increasing/decreasing sets went together but I wanted only one line for each company in the legend... but I can't see why I would want to hide one entirely.

Is that actually something people want, to be able to show only one direction

I'm ok dropping it. It is an option in the plotly.py version though. Not sure if parity with the current ohlc / candlestick offerings is a must.

and by dropping increasing.visible and decreasing.visible, it would make the name problem easier to solve.

For example, with

trace = { type: 'ohlc', name: 'company A', // ... }; // would yield fullTrace[0] = { type: 'scatter', mode: 'lines', name: 'Company A - increasing', // ... }; fullTrace[1] = { type: 'scatter', mode: 'lines', name: 'Company A - decreasing' // ... };

That looks like a good default, but we had better let users override the name separately for the two halves as well, including with a blank string in case they want the legend to have both symbols but only one label.

We could ask the plotly.py candlestick authors ( @chriddyp @cldougl ? tools.py is getting a little out of hand!) if there was a specific request for separately controlling the two visibilities.

I'm pretty sure the goal was to make it so if people were plotting multiple companies they could group decA + incA, decB + incB, etc and only show one legend entry per company to toggle between companies easily.

I think the main reason I allowed the 2 visibilities to be controlled was because that was the easiest way for me to make sure users could edit specific attributes of each trace more easily (color/name etc). I think I wasn't sure how'd these would be used/styled and what attributes would be important. So I'd say if we can't think of a use case of allowing separate control of the visibilities here (I can't think of one) then we don't have to make this completely parallel to the python wrapper.

in https://github.com/plotly/plotly.js/pull/1020/commits I chose to remove increasing / decreasing visible and add increasing / decreasing name and showlegend so that users can edit legend item name / visibility per direction, but disallowing hiding a direction completely for the graph.

that's commit 2b1918c

monfera · 2016-10-07T14:32:59Z

@etpinard this is beautiful work!!!

alexcjohnson · 2016-10-07T16:47:47Z

src/traces/ohlc/attributes.js

+    //
+    //
+    // should we add the option for ohlc along y-axis ??
+    t: {


I don't have a very strong opinion on this, but my gut reaction would have been to make it x, and change it to y if we offered an option for horizontal values like we do with bar charts (though I can't find any examples of people actually doing this). Stela says she thinks she's seen these charts against a different axis (asset type, for example). I can't find an example of this, but can imagine folks wanting to see if different stocks or asset types were subject to the same trends during a given time period (you'd have to normalize their values somehow of course). And If that were to happen, it would be awkward to call stock A, stock B, etc t but calling them x would be natural.

Ok. Let's go with x then which matches layout.xaxis

done in d2d6845

alexcjohnson · 2016-10-07T17:06:14Z

src/traces/ohlc/attributes.js

+    width: Lib.extendFlat({}, lineAttrs.width),
+    dash: Lib.extendFlat({}, lineAttrs.dash),
+
+    tickwidth: {


Shouldn't this be shared between the directions? Not sure if there's a reason people would want up and down to have different widths (if so there could be an override within each direction), but it would be very normal to want to adjust both at once.

Shouldn't this be shared between the directions?

Probably. I'd vote 👍 for a shared attribute.

done in 6f3eac2

alexcjohnson · 2016-10-07T17:07:23Z

src/traces/ohlc/attributes.js

+    tickwidth: {
+        valType: 'number',
+        min: 0,
+        max: 1,


They'll start to overlap at 0.5, right? Not sure if we want to forbid overlap, but normally you wouldn't want to go past this.

Nice catch. Thanks!

done in 6f3eac2

alexcjohnson · 2016-10-07T17:14:09Z

src/traces/candlestick/attributes.js

+    color: Lib.extendFlat({}, boxAttrs.line.color),
+    width: Lib.extendFlat({}, boxAttrs.line.width),
+    fillcolor: Lib.extendFlat({}, boxAttrs.fillcolor),
+    tickwidth: Lib.extendFlat({}, boxAttrs.whiskerwidth, { dflt: 0 }),


Same as for OHLC, should the widths just be shared by both directions? But also, is there precedent for calling the box whiskers "ticks"? That seems to conflict with what ticks mean for OHLC. I might have left it just as "whiskerwidth."

tickwidth felt more natural for OHLC, then I copied the name over to candlesticks for consistency.

I guess whiskerwidth in OHLC would make sense too.

I'd vote for whiskerwidth as a shared attribute for both directions, in both OHLC in candlestick.

EDIT following a quick chat with @alexcjohnson :

Let's go with:

tickwidth for OHLC as per the wiki article

whiskerwidth for candlestick on par with our box attributes
whiskerwidth for

done in 38e5bd7

- recalc when gd.calcdata doesn't match with fullData (not gd.data)

- so that transform writing can set layout option during transform step

- after 5b5a5a0

- factor out hacky moves in ohlc/helpers.js - pass open, high, low, close array in transform opts, then pass then to traceOut in transform supplyDefaults - use trace (not trace._fullInput) to filter -> this makes ohlc and candlestick compatible with filters & groupby

- this may have some side effects, but this is necessary to make ohlc and candlestick work with filter and groupby transforms - in brief, we need to make sure 'data_array' of input module are used correctly during filter and groupby operations

- remove `increasing/decreasing.visible` attribute - add increasing/decreasing 'name' and 'showlegend'

- so that editing legend item text restyles increasing / decreasing 'name' instead of trace-wide name

etpinard · 2016-10-11T20:27:33Z

I'll try to add a jasmine test for fbb299b and I haven't pushed the image mocks yet

But, apart from that, this PR is ready for (hopefully final) review.

alexcjohnson · 2016-10-12T02:49:17Z

src/traces/candlestick/attributes.js

+    showlegend: OHLCattrs.increasing.showlegend,
+
+    color: Lib.extendFlat({}, boxAttrs.line.color),
+    width: Lib.extendFlat({}, boxAttrs.line.width),


I probably should have noticed this earlier (sorry!) but color and width seem odd for line color and width. It took me a bit to figure out that this wasn't the width of the candle, and that we don't need an attribute for that because layout.boxgap does that... This should probably be mentioned somewhere!

Also just like whiskerwidth I'd think width is a property shared by both directions. Can we call these linecolor (in directionAttrs) and linewidth (in attributes)? I guess we could argue that for consistency these should go inside a line container instead... it seems a bit funny to make a one-member container just to keep the names consistent but maybe that will actually be easier and clearer.

Can we call these linecolor (in directionAttrs) and linewidth (in attributes)?

sounds good.

it seems a bit funny to make a one-member container

Agreed. I much prefer increasing.linecolor and decreasing.lincolor

- by filling in a 'text' array in the generated traces

alexcjohnson · 2016-10-12T03:45:07Z

src/transforms/groupby.js

    if(!(Array.isArray(groups)) || groups.length === 0) {
        return trace;
    }

+    var groupNames = groups.filter(function(g, i, self) {
+        return self.indexOf(g) === i;
+    });


Took me a minute to figure out that this is uniquifying the array when I saw it up in findArrayAttributes... here its purpose is a little clearer. But it makes me think (since it looks like these will get called a lot) that we might want to put a faster one in our Lib - see eg "Performance" in http://stackoverflow.com/a/9229821

Good call. Thanks!

done in f6f072f

for ohlc and candlestick traces

- use it in groupby transform and findArrayAttributes

- the default hovermode: 'x' now looks good after adding transformed hover text

- trace-wide 'line' style sets default for increasing / decreasing line styling

etpinard · 2016-10-12T18:31:54Z

@alexcjohnson @bpostlethwaite

This PR is ready for (hopefully final) review.

etpinard · 2016-10-12T19:29:39Z

src/traces/ohlc/helpers.js

+
+var Lib = require('../../lib');
+
+// This routine gets called during the trace supply-defaults step.


@bpostlethwaite basically all the hackery I needed is in this file.

I hope you find the comments useful.

alexcjohnson · 2016-10-12T19:38:41Z

src/traces/ohlc/attributes.js

@@ -102,7 +102,7 @@ module.exports = {
        valType: 'number',
        min: 0,
        max: 0.5,
-        dflt: 0.05,
+        dflt: 0.1,


I might actually vote for quite a bit higher - like 0.3 perhaps?

at 0.1

at 0.3

at 0.2

I'd vote for 0.2.

@chriddyp @cldougl any objections?

Sure, either 0.2 or 0.3 is OK. I still sort of prefer 0.3... but having never used these charts myself it's not a very informed opinion.

0.3 from me 3️⃣

done in b98835c

alexcjohnson · 2016-10-12T19:47:25Z

@etpinard this looks great! Just a question about the default tickwidth - feels to me like we should be using more of the space we have available, both to guide the eye better to whether one day's close is above or below the next day's open, and so the ticks are more visible when they're really closely spaced.

Aside from that, 💃 from me!

bpostlethwaite · 2016-10-12T21:08:13Z

src/traces/ohlc/helpers.js

+// This routine gets called during the transform supply-defaults step
+// where it clears ephemeral transform opts in user data
+// and effectively put back user date in its pre-supplyDefaults state.
+exports.clearEphemeralTransformOpts = function(traceIn) {


oh man... you know the transform implementation isn't 💯 when you roll out a function like this. Fancy workarounds though 🎉

etpinard added feature something new status: in progress labels Oct 7, 2016

etpinard added this to the v1.18.0 milestone Oct 7, 2016

etpinard commented Oct 7, 2016

View reviewed changes

cldougl reviewed Oct 7, 2016

View reviewed changes

alexcjohnson reviewed Oct 7, 2016

View reviewed changes

etpinard added 14 commits October 11, 2016 11:38

relax requirement for re-calc

e915a29

- recalc when gd.calcdata doesn't match with fullData (not gd.data)

plots: pass user layout to transforms

cf5da9b

- so that transform writing can set layout option during transform step

first pass ohlc trace type

7864fa8

first pass candlestick trace type

44f2bec

add ohlc and candlestick to main plotly.js lib index

df24552

[maybe?] add 'finance' partial bundle

6cac3fe

fixup user-defined transform test

0f17423

- after 5b5a5a0

finance: replace attribute 't' -> 'x'

61c72cc

candlestick: rename 'tickwidth' -> 'whiskerwidth' + make it 1 per trace

a9eb7c5

ohlc: make 'tickwidth' 1 per trace

ede8ee2

fix groupby when `enabled: false

96e129a

test: first pass finance suite

d61afb9

etpinard added 3 commits October 11, 2016 16:17

finance: improve dflt colors

7b19b74

finance: improve inc / dec 'name' / 'showlegend' logic

2b1918c

- remove `increasing/decreasing.visible` attribute - add increasing/decreasing 'name' and 'showlegend'

legend: add logic for 'ohlc' and 'candlestick' in legend name edits

fbb299b

- so that editing legend item text restyles increasing / decreasing 'name' instead of trace-wide name

etpinard added status: reviewable and removed status: in progress labels Oct 11, 2016

doc: add comments about non-trivial logic in ohlc / candlestick

caf390b

alexcjohnson reviewed Oct 12, 2016

View reviewed changes

etpinard added 3 commits October 11, 2016 23:37

finance: pass trace 'xaxis' & 'yaxis' to generated traces

672d517

finance: make sure computed tickWidth is always positive

ea3c5a1

olhc: add custom hover text

a892b26

- by filling in a 'text' array in the generated traces

alexcjohnson reviewed Oct 12, 2016

View reviewed changes

etpinard added 6 commits October 12, 2016 12:09

finance: ensure that user data isn't mutated in Plots.supplyDefaults

7e0a382

for ohlc and candlestick traces

utils: add filterUnique helper

f6f072f

- use it in groupby transform and findArrayAttributes

ohlc: don't force hovermode: 'closest'

6036045

- the default hovermode: 'x' now looks good after adding transformed hover text

ohlc: bump tickwidth dflt to 0.1

d26a4b4

finance: add common 'line' style container

3803005

- trace-wide 'line' style sets default for increasing / decreasing line styling

test: add finance mocks

facbf3d

etpinard commented Oct 12, 2016

View reviewed changes

alexcjohnson reviewed Oct 12, 2016

View reviewed changes

bpostlethwaite reviewed Oct 12, 2016

View reviewed changes

etpinard added 2 commits October 13, 2016 09:44

ohlc: bump tickwidth dflt to 0.3

b98835c

test: add finance trace module bundle test

c9de6c8

etpinard merged commit 2dc6d7a into master Oct 13, 2016

etpinard deleted the finance branch October 13, 2016 13:57

This was referenced Oct 13, 2016

Provide a data table view usable via JavaScript #991

Closed

Pass 'name' in hoverinfo for OHLC generated traces #1050

Merged

etpinard mentioned this pull request Feb 1, 2017

Finance trace editable legend fix #1349

Merged


		var Lib = require('../../lib');

		// This routine gets called during the trace supply-defaults step.

OHLC and Candlestick trace types #1020

OHLC and Candlestick trace types #1020

Conversation

etpinard commented Oct 7, 2016 • edited Loading

etpinard commented Oct 7, 2016 • edited Loading

etpinard commented Oct 7, 2016

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 Oct 11, 2016 • edited Loading

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

cldougl Oct 7, 2016 • edited Loading

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

monfera commented Oct 7, 2016

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 Oct 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard commented Oct 11, 2016

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 Oct 12, 2016

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

alexcjohnson commented Oct 12, 2016

Choose a reason for hiding this comment

etpinard commented Oct 7, 2016 •

edited

Loading

etpinard commented Oct 7, 2016 •

edited

Loading

etpinard Oct 11, 2016 •

edited

Loading

cldougl Oct 7, 2016 •

edited

Loading

etpinard Oct 7, 2016 •

edited

Loading