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 3 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
7 changes: 4 additions & 3 deletions src/traces/scattergl/convert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ proto.handlePick = function(pickResult) {
trace: this,
dataCoord: pickResult.dataCoord,
traceCoord: [
this.pickXData[index],
Number(this.pickXData[index]), // non-fancy scattergl has Dates
Copy link
Collaborator

Choose a reason for hiding this comment

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

@monfera while pure-js users of plotly.js can use Date objects, plot.ly (and plotly.js users that may want to serialize their plots) use date strings via Lib.dateTime2ms (here is what regular cartesian does). I realize that this will be slower, but can we at least support it for compatibility with the rest of our dates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Alex, good point. Currently, scatter works with ISO formatted dates e.g. 2016-10-13 but I'll need to add a conversion from string to date for scattergl. It'll be done upfront. The line you refer to above probably won't need to change as by the time the tooltip gets data it's already in the standard JS Date format, but the front-end part needs some logic. To avoid having to loop through a possibly large number of array elements, I'm thinking about just checking the type of the first element of the array; if it's Date it's fine; if it's a string, then convert all elements to Date. Is it OK in your opinion and also @etpinard ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It makes me a little wary, but I can't think of a reason users would mix Dates and date strings (at least within one trace - definitely different traces should be checked independently). The first element may not be enough though, in case it's null or something, but the first valid element seems OK. Especially since this trace type is specifically built for performance, I guess we can impose some sane restrictions on the users.

I haven't really looked at the architecture here, but if you're already converting the values you can just go straight to milliseconds, ignore Date objects altogether. Also notice that it's not exactly ISO 8601 we use, I don't know its name but I think of it as SQL date format - ie maximally it's 2016-10-13 12:34:56.789123 but we're permissive about letting you truncate it after any part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alex I'd probably reuse the date conversion routine you pointed to above, as if at all possible, I'd shy away from rolling a new string -> Date parser. Yes I agree with using epoch milliseconds as a general principle esp. on greenfield projects, although in plotly it looks like it's not typically done. There's already conversion by default, as WebGL buffers cover float arrays, so it does get converted to epoch ms already.

this.pickYData[index]
],
textLabel: Array.isArray(this.textLabels) ?
Expand All @@ -135,7 +135,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 @@ -279,7 +279,8 @@ proto.updateFast = function(options) {
yy = y[i];

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

Choose a reason for hiding this comment

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

Lib.isDateTime is what you need here.

I'm afraid it will be significantly slower than xx instanceof Date though.

Copy link
Contributor Author

@monfera monfera Oct 17, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

@etpinard @alexcjohnson I'd just like to clarify something here. Testing with Lib.isDateTime is useful here as long as we actually do something with the broader set of options, in particular, string representations of dates. So there's got to be logic to check the type and dispatch to either straight-through processing (no conversion, i.e. Date objects that are handled now) or conversion from the string representations of dates to Date objects. Should I go ahead and do these things? As mentioned above, I'd keep the fast path fast by testing the first legitimate value only, and if it's suitable for straight-through processing (STP) then the entire array would be assumed to be so. (As @alexcjohnson mentioned I'd check for the first non-null value).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a little surprised that epoch milliseconds are accepted as date data right now, I don't think our svg cartesian axes accept that, do they? But anyway I think your plan sounds good. Lib.dateTime2ms does short-circuit Date objects, but you could certainly shave off a little extra overhead by not going through it at all, and I think it's fine to assume (at least for these performance-oriented types) that all valid values in a given array have the same type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexcjohnson sorry I conflated two things in the spur of the moment, acceptance of numeric values and the rendering of dates. If I pass on epoch milliseconds it'll render fine but it won't render the numbers as dates; it'll simply show the epoch milliseconds as numbers. I'll revise my comment.


idToIndex[pId++] = i;

Expand Down
79 changes: 79 additions & 0 deletions test/jasmine/tests/axes_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ var Plots = require('@src/plots/plots');
var Lib = require('@src/lib');
var Color = require('@src/components/color');
var tinycolor = require('tinycolor2');
var hasWebGLSupport = require('../assets/has_webgl_support');
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to keep axes_test.js for test cases in relation to src/plots/cartesian/axes.js only.

@monfera can you move your new test cases to one of the gl2d_ suite of your choice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New test are now separated from axes_test.


var handleTickValueDefaults = require('@src/plots/cartesian/tick_value_defaults');
var Axes = PlotlyInternal.Axes;
Expand Down Expand Up @@ -387,6 +388,84 @@ describe('Test axes', function() {
});
});

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
Copy link
Contributor

Choose a reason for hiding this comment

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

nice find 👍

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);
});

});

describe('categoryorder', function() {

var gd;
Expand Down