Skip to content

Non-fancy scattergl to work with dates #1021

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.

Sign up for GitHub

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 7 commits into from
Oct 24, 2016
Merged

Non-fancy scattergl to work with dates #1021

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
fac70b2
Admit date X axes for fast (non-fancy) scattergl
monfera Oct 7, 2016
a23756d
Admit date X axes for fast (non-fancy) scattergl hover tooltip too
monfera Oct 7, 2016
ab4ffbf
adding tests for fancy vs non-fancy scattergl
monfera Oct 10, 2016
f5d6113
separate the new date rendering tests from axes_test.js
monfera Oct 17, 2016
3c79068
Admit isDateTime === true values (e.g. date strings) in non-fancy sca…
monfera Oct 19, 2016
5b71b47
doing cheaper, probabilistic checks outside the scattergl conversion …
monfera Oct 20, 2016
46a6b6c
minor speedup - avoid some boolean checks in the scattergl conversion…
monfera Oct 20, 2016
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
31 changes: 20 additions & 11 deletions src/traces/scattergl/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,13 @@ proto.handlePick = function(pickResult) {
index = this.idToIndex[pickResult.pointId];
}

var x = this.pickXData[index];

return {
trace: this,
dataCoord: pickResult.dataCoord,
traceCoord: [
this.pickXData[index],
isNumeric(x) || !Lib.isDateTime(x) ? x : Lib.dateTime2ms(x),
this.pickYData[index]
],
textLabel: Array.isArray(this.textLabels) ?
Expand All @@ -135,7 +137,7 @@ proto.handlePick = function(pickResult) {

// check if trace is fancy
proto.isFancy = function(options) {
if(this.scene.xaxis.type !== 'linear') return true;
if(this.scene.xaxis.type !== 'linear' && this.scene.xaxis.type !== 'date') return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

if(this.scene.yaxis.type !== 'linear') return true;

if(!options.x || !options.y) return true;
Expand Down Expand Up @@ -270,7 +272,7 @@ proto.updateFast = function(options) {
pId = 0,
ptr = 0;

var xx, yy;
var xx, yy, fastType;

// TODO add 'very fast' mode that bypasses this loop
// TODO bypass this on modebar +/- zoom
Expand All @@ -279,17 +281,24 @@ proto.updateFast = function(options) {
yy = y[i];

// check for isNaN is faster but doesn't skip over nulls
if(!isNumeric(xx) || !isNumeric(yy)) continue;
fastType = isNumeric(xx) || xx instanceof Date;

idToIndex[pId++] = i;
if(isNumeric(yy) && (fastType || Lib.isDateTime(xx))) {

positions[ptr++] = xx;
positions[ptr++] = yy;
if(!fastType) {
xx = Lib.dateTime2ms(xx);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard @alexcjohnson as you see I ended up not skimping on tests, i.e. not just checking the first value, as we already had a check with relatively fast tests (number or Date). It's only when this fails is when the slower path is taken. If you think I can optimize away the isDateTime part, i.e. just assume that once we hit one of those, the rest of them will be like it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It might worth exploring with placing the instanceOf Date vs ms vs Lib.isDateTime check as part of the autotype routine here.

Maybe, we could add a Lib.isDateTime alternative that would track what kind of date time the x are input at the defaults step and store that datetime type in e.g. fullLayout.xaxis._datetimeType ?

Copy link
Contributor

Choose a reason for hiding this comment

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

... I think the autotype routine does it right. It samples not-more than 1000 pts (that takes ~ 3 ms) to determine the axis type.

In the cartesian code, with that axis type, the data-to-calcData routines (i.e. ax.d2c) simply assume a lone axis type for all items in x.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etpinard thanks for testing and making a decision over whether this approach is fast enough or not. The current code in master already incurred the expense of two isNumeric calls per loop execution and admittedly I haven't attempted to make it faster than that. I agree that we can take this Date rework as an opportunity to speed up the loop a bit. For now, it can only be done probabilistically (for future options, I added a few lines of comment), as you said. In my previous commit I didn't shoot for such speedup, in part because it would be based on a heuristic, and in part because, if we want to increase speed, we might be able to do other things:

  • if we want to speed up plotly.js time, then permitting a typed array input (= no array copy), or at least letting the user specify the value types can speed things up
  • if we want to speed it up when called via Python, then either the Python interface could directly convert the date strings into JavaScript Date objects (or better, directly populate a typed array), or, the Python -> JS WebSockets interface can directly deserialize into a JS typed array (this of course assumes that Python sends epoch milliseconds).

Back to present time: the fast path can be provided if all values can be assumed to be number (or Date) rather than a majority, so I ended up not tampering with the autotype logic (it could be more DRY but wanted to keep loop bodies simple, and direct, for speed).


idToIndex[pId++] = i;

bounds[0] = Math.min(bounds[0], xx);
bounds[1] = Math.min(bounds[1], yy);
bounds[2] = Math.max(bounds[2], xx);
bounds[3] = Math.max(bounds[3], yy);
positions[ptr++] = xx;
positions[ptr++] = yy;

bounds[0] = Math.min(bounds[0], xx);
bounds[1] = Math.min(bounds[1], yy);
bounds[2] = Math.max(bounds[2], xx);
bounds[3] = Math.max(bounds[3], yy);
}
}

positions = truncate(positions, ptr);
Expand Down
99 changes: 99 additions & 0 deletions test/jasmine/tests/gl2d_date_axis_render_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
var PlotlyInternal = require('@src/plotly');

var hasWebGLSupport = require('../assets/has_webgl_support');

var createGraphDiv = require('../assets/create_graph_div');
var destroyGraphDiv = require('../assets/destroy_graph_div');

describe('date axis', function() {

if(!hasWebGLSupport('axes_test date axis')) return;

var gd;

beforeEach(function() {
gd = createGraphDiv();
});

afterEach(destroyGraphDiv);

it('should use the fancy gl-vis/gl-scatter2d', function() {
PlotlyInternal.plot(gd, [{
type: 'scattergl',
'marker': {
'color': 'rgb(31, 119, 180)',
'size': 18,
'symbol': [
'diamond',
'cross'
]
},
x: [new Date('2016-10-10'), new Date('2016-10-12')],
y: [15, 16]
}]);

expect(gd._fullLayout.xaxis.type).toBe('date');
expect(gd._fullLayout.yaxis.type).toBe('linear');
expect(gd._fullData[0].type).toBe('scattergl');
expect(gd._fullData[0]._module.basePlotModule.name).toBe('gl2d');

// one way of check which renderer - fancy vs not - we're using
expect(gd._fullLayout._plots.xy._scene2d.glplot.objects[3].pointCount).toBe(0);
});

it('should use the fancy gl-vis/gl-scatter2d once again', function() {
PlotlyInternal.plot(gd, [{
type: 'scattergl',
'marker': {
'color': 'rgb(31, 119, 180)',
'size': 36,
'symbol': [
'circle',
'cross'
]
},
x: [new Date('2016-10-10'), new Date('2016-10-11')],
y: [15, 16]
}]);

expect(gd._fullLayout.xaxis.type).toBe('date');
expect(gd._fullLayout.yaxis.type).toBe('linear');
expect(gd._fullData[0].type).toBe('scattergl');
expect(gd._fullData[0]._module.basePlotModule.name).toBe('gl2d');

// one way of check which renderer - fancy vs not - we're using
expect(gd._fullLayout._plots.xy._scene2d.glplot.objects[3].pointCount).toBe(0);
});

it('should now use the non-fancy gl-vis/gl-scatter2d', function() {
PlotlyInternal.plot(gd, [{
type: 'scattergl',
mode: 'markers', // important, as otherwise lines are assumed (which needs fancy)
x: [new Date('2016-10-10'), new Date('2016-10-11')],
y: [15, 16]
}]);

expect(gd._fullLayout.xaxis.type).toBe('date');
expect(gd._fullLayout.yaxis.type).toBe('linear');
expect(gd._fullData[0].type).toBe('scattergl');
expect(gd._fullData[0]._module.basePlotModule.name).toBe('gl2d');

expect(gd._fullLayout._plots.xy._scene2d.glplot.objects[3].pointCount).toBe(2);
});

it('should use the non-fancy gl-vis/gl-scatter2d with string dates', function() {
PlotlyInternal.plot(gd, [{
type: 'scattergl',
mode: 'markers', // important, as otherwise lines are assumed (which needs fancy)
x: ['2016-10-10', '2016-10-11'],
y: [15, 16]
}]);

expect(gd._fullLayout.xaxis.type).toBe('date');
expect(gd._fullLayout.yaxis.type).toBe('linear');
expect(gd._fullData[0].type).toBe('scattergl');
expect(gd._fullData[0]._module.basePlotModule.name).toBe('gl2d');

expect(gd._fullLayout._plots.xy._scene2d.glplot.objects[3].pointCount).toBe(2);
});
});