Skip to content

Finance refactor #2561

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 25 commits into from
Apr 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
e979cf0
refactor candlestick into a first-class trace, also box/violin hover …
alexcjohnson Apr 5, 2018
84c36a9
refactor OHLC into a first-class trace type
alexcjohnson Apr 6, 2018
3870503
update non-finance tests for finance refactor
alexcjohnson Apr 10, 2018
4c46a73
fix finance tests - with a couple of bugfixes too
alexcjohnson Apr 13, 2018
94c4560
fix #2004
alexcjohnson Apr 13, 2018
3509b99
fix and test finance hover labels - including test for #1807
alexcjohnson Apr 13, 2018
2849ff9
test finance trace select
alexcjohnson Apr 16, 2018
be2b523
:hocho: hover test in finance_test - hover_label_test covers it better
alexcjohnson Apr 16, 2018
5b6a7d5
fix prereqs for transform_multi_test
alexcjohnson Apr 16, 2018
121d171
update finance bundle tests
alexcjohnson Apr 16, 2018
ad1d8f0
update finance mocks
alexcjohnson Apr 16, 2018
2554482
fix lf/uf vs min/max logic for box/violin/candlestick
alexcjohnson Apr 16, 2018
0d80a21
fix #2510 - or rather, revive the test that the refactor fixed this
alexcjohnson Apr 16, 2018
82677ac
tweak and test cleanData for finance traces
alexcjohnson Apr 16, 2018
71fa112
remove plotSchema change from before we decided to refactor finance t…
alexcjohnson Apr 16, 2018
0b7541e
fix for box & candlestick together on one subplot
alexcjohnson Apr 16, 2018
f84dfae
change to shorter dash in finance_style mock
alexcjohnson Apr 16, 2018
d64fab6
remove TODO that's been OK'd as is
alexcjohnson Apr 16, 2018
dc01685
undefined -> BADNUM in ohlc/calc
alexcjohnson Apr 16, 2018
c8b03ee
Merge branch 'master' into finance-refactor
alexcjohnson Apr 17, 2018
302d1e6
update new getModuleCalcData callers to new API
alexcjohnson Apr 17, 2018
f498bd0
set to null instead of delete in ohlc/calc
alexcjohnson Apr 17, 2018
c77a8a3
test that selection applies to plot, not rangeslider
alexcjohnson Apr 17, 2018
3f43253
:hocho: fit
alexcjohnson Apr 17, 2018
13204a9
mark polar drag test flaky
alexcjohnson Apr 17, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions src/components/fx/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -826,13 +826,6 @@ function createHoverText(hoverData, opts, gd) {
}
}

// used by other modules (initially just ternary) that
// manage their own hoverinfo independent of cleanPoint
// 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 += d.extraText;

if(d.zLabel !== undefined) {
if(d.xLabel !== undefined) text += 'x: ' + d.xLabel + '<br>';
if(d.yLabel !== undefined) text += 'y: ' + d.yLabel + '<br>';
Expand All @@ -851,6 +844,13 @@ function createHoverText(hoverData, opts, gd) {
text += (text ? '<br>' : '') + d.text;
}

// used by other modules (initially just ternary) that
// manage their own hoverinfo independent of cleanPoint
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you have to move this block down?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the headsup 👌


// if 'text' is empty at this point,
// put 'name' in main label and don't show secondary label
if(text === '') {
Expand Down
12 changes: 1 addition & 11 deletions src/components/legend/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -384,20 +384,10 @@ function drawTexts(g, gd) {

if(!this.text()) text = ' \u0020\u0020 ';

var transforms, direction;
var fullInput = legendItem.trace._fullInput || {};
var update = {};

// N.B. this block isn't super clean,
// is unfortunately untested at the moment,
// and only works for for 'ohlc' and 'candlestick',
// but should be generalized for other one-to-many transforms
if(['ohlc', 'candlestick'].indexOf(fullInput.type) !== -1) {
transforms = legendItem.trace.transforms;
direction = transforms[transforms.length - 1].direction;

update[direction + '.name'] = text;
} else if(Registry.hasTransform(fullInput, 'groupby')) {
if(Registry.hasTransform(fullInput, 'groupby')) {
var groupbyIndices = Registry.getTransformIndices(fullInput, 'groupby');
var index = groupbyIndices[groupbyIndices.length - 1];

Expand Down
60 changes: 58 additions & 2 deletions src/components/legend/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ module.exports = function style(s, gd) {
.each(styleBoxes)
.each(stylePies)
.each(styleLines)
.each(stylePoints);
.each(stylePoints)
.each(styleCandles)
.each(styleOHLC);
Copy link
Contributor

Choose a reason for hiding this comment

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

🚨 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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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(...) })


function styleLines(d) {
var trace = d[0].trace;
Expand Down Expand Up @@ -207,7 +209,61 @@ module.exports = function style(s, gd) {
.call(Color.fill, trace.fillcolor);

if(w) {
p.call(Color.stroke, trace.line.color);
Color.stroke(p, trace.line.color);
}
});
}

function styleCandles(d) {
var trace = d[0].trace,
pts = d3.select(this).select('g.legendpoints')
.selectAll('path.legendcandle')
.data(trace.type === 'candlestick' && trace.visible ? [d, d] : []);
pts.enter().append('path').classed('legendcandle', true)
.attr('d', function(_, i) {
if(i) return 'M-15,0H-8M-8,6V-6H8Z'; // increasing
return 'M15,0H8M8,-6V6H-8Z'; // decreasing
})
.attr('transform', 'translate(20,0)')
.style('stroke-miterlimit', 1);
pts.exit().remove();
pts.each(function(_, i) {
var container = trace[i ? 'increasing' : 'decreasing'];
var w = container.line.width,
p = d3.select(this);

p.style('stroke-width', w + 'px')
.call(Color.fill, container.fillcolor);

if(w) {
Color.stroke(p, container.line.color);
}
});
}

function styleOHLC(d) {
var trace = d[0].trace,
pts = d3.select(this).select('g.legendpoints')
.selectAll('path.legendohlc')
.data(trace.type === 'ohlc' && trace.visible ? [d, d] : []);
pts.enter().append('path').classed('legendohlc', true)
.attr('d', function(_, i) {
if(i) return 'M-15,0H0M-8,-6V0'; // increasing
return 'M15,0H0M8,6V0'; // decreasing
})
.attr('transform', 'translate(20,0)')
.style('stroke-miterlimit', 1);
pts.exit().remove();
pts.each(function(_, i) {
var container = trace[i ? 'increasing' : 'decreasing'];
var w = container.line.width,
p = d3.select(this);

p.style('fill', 'none')
.call(Drawing.dashLine, container.line.dash, w);

if(w) {
Color.stroke(p, container.line.color);
}
});
}
Expand Down
14 changes: 8 additions & 6 deletions src/components/rangeslider/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,16 +14,18 @@ var oppAxisAttrs = require('./oppaxis_attributes');
var axisIds = require('../../plots/cartesian/axis_ids');

module.exports = function handleDefaults(layoutIn, layoutOut, axName) {
if(!layoutIn[axName].rangeslider) return;
var axIn = layoutIn[axName];
var axOut = layoutOut[axName];

if(!(axIn.rangeslider || layoutOut._requestRangeslider[axOut._id])) return;

// not super proud of this (maybe store _ in axis object instead
if(!Lib.isPlainObject(layoutIn[axName].rangeslider)) {
layoutIn[axName].rangeslider = {};
if(!Lib.isPlainObject(axIn.rangeslider)) {
axIn.rangeslider = {};
}

var containerIn = layoutIn[axName].rangeslider,
axOut = layoutOut[axName],
containerOut = axOut.rangeslider = {};
var containerIn = axIn.rangeslider;
var containerOut = axOut.rangeslider = {};

function coerce(attr, dflt) {
return Lib.coerce(containerIn, containerOut, attributes, attr, dflt);
Expand Down
3 changes: 2 additions & 1 deletion src/components/rangeslider/draw.js
Original file line number Diff line number Diff line change
Expand Up @@ -455,7 +455,8 @@ function drawRangePlot(rangeSlider, gd, axisOpts, opts) {
id: id,
plotgroup: plotgroup,
xaxis: xa,
yaxis: ya
yaxis: ya,
isRangePlot: true
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

};

if(isMainPlot) mainplotinfo = plotinfo;
Expand Down
4 changes: 1 addition & 3 deletions src/lib/coerce.js
Original file line number Diff line number Diff line change
Expand Up @@ -434,9 +434,7 @@ exports.coerceFont = function(coerce, attr, dfltObj) {
*/
exports.coerceHoverinfo = function(traceIn, traceOut, layoutOut) {
var moduleAttrs = traceOut._module.attributes;
var attrs = moduleAttrs.hoverinfo ?
{hoverinfo: moduleAttrs.hoverinfo} :
baseTraceAttrs;
var attrs = moduleAttrs.hoverinfo ? moduleAttrs : baseTraceAttrs;

var valObj = attrs.hoverinfo;
var dflt;
Expand Down
58 changes: 58 additions & 0 deletions src/plot_api/helpers.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,6 +330,32 @@ exports.cleanData = function(data, existingData) {
}
}

// fixes from converting finance from transforms to real trace types
if(trace.type === 'candlestick' || trace.type === 'ohlc') {
var increasingShowlegend = (trace.increasing || {}).showlegend !== false;
var decreasingShowlegend = (trace.decreasing || {}).showlegend !== false;
var increasingName = cleanFinanceDir(trace.increasing);
var decreasingName = cleanFinanceDir(trace.decreasing);

// now figure out something smart to do with the separate direction
// names we removed
if((increasingName !== false) && (decreasingName !== false)) {
// both sub-names existed: base name previously had no effect
// so ignore it and try to find a shared part of the sub-names

var newName = commonPrefix(
increasingName, decreasingName,
increasingShowlegend, decreasingShowlegend
);
// if no common part, leave whatever name was (or wasn't) there
if(newName) trace.name = newName;
}
else if((increasingName || decreasingName) && !trace.name) {
// one sub-name existed but not the base name - just use the sub-name
trace.name = increasingName || decreasingName;
}
}

// transforms backward compatibility fixes
if(Array.isArray(trace.transforms)) {
var transforms = trace.transforms;
Expand Down Expand Up @@ -388,6 +414,38 @@ exports.cleanData = function(data, existingData) {
}
};

function cleanFinanceDir(dirContainer) {
if(!Lib.isPlainObject(dirContainer)) return false;

var dirName = dirContainer.name;

delete dirContainer.name;
delete dirContainer.showlegend;

return (typeof dirName === 'string' || typeof dirName === 'number') && String(dirName);
}

function commonPrefix(name1, name2, show1, show2) {
// if only one is shown in the legend, use that
if(show1 && !show2) return name1;
if(show2 && !show1) return name2;

// if both or neither are in the legend, check if one is blank (or whitespace)
// and use the other one
// note that hover labels can still use the name even if the legend doesn't
if(!name1.trim()) return name2;
if(!name2.trim()) return name1;

var minLen = Math.min(name1.length, name2.length);
var i;
for(i = 0; i < minLen; i++) {
if(name1.charAt(i) !== name2.charAt(i)) break;
}

var out = name1.substr(0, i);
return out.trim();
}

// textposition - support partial attributes (ie just 'top')
// and incorrect use of middle / center etc.
function cleanTextPosition(textposition) {
Expand Down
7 changes: 4 additions & 3 deletions src/plot_api/plot_schema.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,12 +256,13 @@ exports.getTraceValObject = function(trace, parts) {
var moduleAttrs, valObject;

if(head === 'transforms') {
if(!Array.isArray(trace.transforms)) return false;
var transforms = trace.transforms;
if(!Array.isArray(transforms) || !transforms.length) return false;
var tNum = parts[1];
if(!isIndex(tNum) || tNum >= trace.transforms.length) {
if(!isIndex(tNum) || tNum >= transforms.length) {
return false;
}
moduleAttrs = (Registry.transformsRegistry[trace.transforms[tNum].type] || {}).attributes;
moduleAttrs = (Registry.transformsRegistry[transforms[tNum].type] || {}).attributes;
valObject = moduleAttrs && moduleAttrs[parts[2]];
i = 3; // start recursing only inside the transform
}
Expand Down
1 change: 1 addition & 0 deletions src/plots/cartesian/constants.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ module.exports = {
'carpetlayer',
'violinlayer',
'boxlayer',
Copy link
Contributor

Choose a reason for hiding this comment

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

Can box traces and candlestick coexist on the same subplot?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

'ohlclayer',
'scatterlayer'
],

Expand Down
45 changes: 29 additions & 16 deletions src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -192,20 +192,24 @@ 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();
Copy link
Contributor

Choose a reason for hiding this comment

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

This query is getting ridiculous.

Can we instead loop over constants.traceLayerClasses?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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...

Copy link
Contributor

Choose a reason for hiding this comment

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

Moved to #2563

Thanks!

}

// plot all traces for each module at once
for(var j = 0; j < modules.length; j++) {
var _module = modules[j];

// skip over non-cartesian trace modules
if(_module.basePlotModule.name !== 'cartesian') continue;
if(!_module.plot || _module.basePlotModule.name !== 'cartesian') continue;

// plot all traces of this type on this subplot at once
var cdModule = getModuleCalcData(cdSubplot, _module);
var cdModuleAndOthers = getModuleCalcData(cdSubplot, _module);
var cdModule = cdModuleAndOthers[0];
// don't need to search the found traces again - in fact we need to NOT
// so that if two modules share the same plotter we don't double-plot
cdSubplot = cdModuleAndOthers[1];

if(_module.plot) _module.plot(gd, plotinfo, cdModule, transitionOpts, makeOnCompleteCallback);
_module.plot(gd, plotinfo, cdModule, transitionOpts, makeOnCompleteCallback);
}
}

Expand All @@ -215,6 +219,7 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout)
var oldPlots = oldFullLayout._plots || {};

var hadScatter, hasScatter;
var hadOHLC, hasOHLC;
var hadGl, hasGl;
var i, k, subplotInfo, moduleName;

Expand All @@ -232,28 +237,36 @@ exports.clean = function(newFullData, newFullLayout, oldFullData, oldFullLayout)
moduleName = oldModules[i].name;
if(moduleName === 'scatter') hadScatter = true;
else if(moduleName === 'scattergl') hadGl = true;
else if(moduleName === 'ohlc') hadOHLC = true;
}

for(i = 0; i < newModules.length; i++) {
moduleName = newModules[i].name;
if(moduleName === 'scatter') hasScatter = true;
else if(moduleName === 'scattergl') hasGl = true;
else if(moduleName === 'ohlc') hasOHLC = true;
}

if(hadScatter && !hasScatter) {
for(k in oldPlots) {
subplotInfo = oldPlots[k];
if(subplotInfo.plot) {
subplotInfo.plot.select('g.scatterlayer')
.selectAll('g.trace')
.remove();
var layersToEmpty = [];
if(hadScatter && !hasScatter) layersToEmpty.push('g.scatterlayer');
if(hadOHLC && !hasOHLC) layersToEmpty.push('g.ohlclayer');

if(layersToEmpty.length) {
for(var layeri = 0; layeri < layersToEmpty.length; layeri++) {
for(k in oldPlots) {
subplotInfo = oldPlots[k];
if(subplotInfo.plot) {
subplotInfo.plot.select(layersToEmpty[layeri])
.selectAll('g.trace')
.remove();
}
}
}

oldFullLayout._infolayer.selectAll('g.rangeslider-container')
.select('g.scatterlayer')
.selectAll('g.trace')
.remove();
oldFullLayout._infolayer.selectAll('g.rangeslider-container')
.select(layersToEmpty[layeri])
.selectAll('g.trace')
.remove();
}
}

if(hadGl && !hasGl) {
Expand Down
Loading