Skip to content
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

Typed arrays support #2388

Merged
merged 28 commits into from
Feb 28, 2018
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
91b03ff
rename Lib.isArray -> Lib.isArrayOrTypedArray
etpinard Feb 20, 2018
a25ff13
accept typed array when coercing 'data_array' attributes
etpinard Feb 20, 2018
45c2f35
[PoC] bypass ax.d2c for typedArray during ax.makeCalcdata
etpinard Feb 20, 2018
2d81bdc
replace Array.isArray -> Lib.isArrayOrTypedArray in various places
etpinard Feb 20, 2018
bcf59d7
replace Lib.isArray -> Array.isArray in tests w/o typed arrays
etpinard Feb 20, 2018
bc32981
[wip] a few todos
etpinard Feb 20, 2018
2372629
Merge branch 'master' into typed-arrays-support
etpinard Feb 22, 2018
909120e
make scattergl dragmode relayout replot not recalc
etpinard Feb 26, 2018
5316c47
large lint commit in scattergl/index.js
etpinard Feb 26, 2018
0aa0f5e
improvements to Scattergl.calc
etpinard Feb 26, 2018
39ef5a9
update baselines
etpinard Feb 26, 2018
36b4e25
update jasmine tests for new scattergl auto-ranges
etpinard Feb 26, 2018
40f93f7
add svg vs gl scatter autorange :lock:s
etpinard Feb 26, 2018
d98dcc0
improve makeCalcdata typed array handling + some tests + some linting
etpinard Feb 26, 2018
c0e2f73
a few more Array.isArray -> Lib.isArrayOrTypedArray
etpinard Feb 26, 2018
09d37b6
some typed array tests
etpinard Feb 26, 2018
a7ed2c2
:lock: Plotly.restyle support for typed arrays
etpinard Feb 28, 2018
b95e462
add and :lock: typed array support for extendTraces and prependTraces
etpinard Feb 28, 2018
53ea0ec
Merge branch 'master' into typed-arrays-support
etpinard Feb 28, 2018
98d2407
use c2l instead of d2l
etpinard Feb 28, 2018
01a8443
:hocho: TODOs
etpinard Feb 28, 2018
c15722f
add mock :lock:ing down cleanNumber for scattergl
etpinard Feb 28, 2018
a2fb88b
Merge pull request #2404 from plotly/scattergl-autorange
etpinard Feb 28, 2018
f0395b5
improve isTypedArray
etpinard Feb 28, 2018
9b83826
don't require all of Lib when only isArrayOrTypedArray is needed
etpinard Feb 28, 2018
6dd2f69
turn coord typed arrays into plain array for 'date' and 'category axes
etpinard Feb 28, 2018
306986d
check that typed arrays items aren't all NaNs in hasColorscale
etpinard Feb 28, 2018
d4cb0c4
no need to check for typed array in outer array of a 2d array
etpinard Feb 28, 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
6 changes: 4 additions & 2 deletions src/lib/coerce.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,18 +19,20 @@ var nestedProperty = require('./nested_property');
var counterRegex = require('./regex').counter;
var DESELECTDIM = require('../constants/interactions').DESELECTDIM;
var wrap180 = require('./angles').wrap180;
var isArrayOrTypedArray = require('./is_array').isArrayOrTypedArray;

exports.valObjectMeta = {
data_array: {
// You can use *dflt=[] to force said array to exist though.
description: [
'An {array} of data.',
'The value MUST be an {array}, or we ignore it.'
'The value MUST be an {array}, or we ignore it.',
'Note that typed arrays (e.g. Float32Array) are supported.'
].join(' '),
requiredOpts: [],
otherOpts: ['dflt'],
coerceFunction: function(v, propOut, dflt) {
if(Array.isArray(v)) propOut.set(v);
if(isArrayOrTypedArray(v)) propOut.set(v);
else if(dflt !== undefined) propOut.set(dflt);
}
},
Expand Down
5 changes: 4 additions & 1 deletion src/lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,15 @@ lib.nestedProperty = require('./nested_property');
lib.keyedContainer = require('./keyed_container');
lib.relativeAttr = require('./relative_attr');
lib.isPlainObject = require('./is_plain_object');
lib.isArray = require('./is_array');
lib.mod = require('./mod');
lib.toLogRange = require('./to_log_range');
lib.relinkPrivateKeys = require('./relink_private');
lib.ensureArray = require('./ensure_array');

var isArrayModule = require('./is_array');
lib.isTypedArray = isArrayModule.isTypedArray;
lib.isArrayOrTypedArray = isArrayModule.isArrayOrTypedArray;

var coerceModule = require('./coerce');
lib.valObjectMeta = coerceModule.valObjectMeta;
lib.coerce = coerceModule.coerce;
Expand Down
8 changes: 3 additions & 5 deletions src/lib/is_array.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,13 @@

'use strict';

/**
* Return true for arrays, whether they're untyped or not.
*/

// IE9 fallback
var ab = (typeof ArrayBuffer === 'undefined' || !ArrayBuffer.isView) ?
{isView: function() { return false; }} :
ArrayBuffer;

module.exports = function isArray(a) {
exports.isTypedArray = ab.isView;

exports.isArrayOrTypedArray = function(a) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

perusing https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/isView - there's also DataView that passes but looks like it's a little too generic to be meaningful to us as an input. Should we be concerned about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

improved in f0395b5

return Array.isArray(a) || ab.isView(a);
};
14 changes: 7 additions & 7 deletions src/lib/nested_property.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
'use strict';

var isNumeric = require('fast-isnumeric');
var isArray = require('./is_array');
var isArrayOrTypedArray = require('./is_array').isArrayOrTypedArray;
var isPlainObject = require('./is_plain_object');
var containerArrayMatch = require('../plot_api/container_array_match');

Expand Down Expand Up @@ -96,7 +96,7 @@ function npGet(cont, parts) {
}
return allSame ? out[0] : out;
}
if(typeof curPart === 'number' && !isArray(curCont)) {
if(typeof curPart === 'number' && !isArrayOrTypedArray(curCont)) {
return undefined;
}
curCont = curCont[curPart];
Expand Down Expand Up @@ -144,7 +144,7 @@ function isDeletable(val, propStr) {
) {
return false;
}
if(!isArray(val)) return true;
if(!isArrayOrTypedArray(val)) return true;

if(propStr.match(INFO_PATTERNS)) return true;

Expand All @@ -167,7 +167,7 @@ function npSet(cont, parts, propStr) {
for(i = 0; i < parts.length - 1; i++) {
curPart = parts[i];

if(typeof curPart === 'number' && !isArray(curCont)) {
if(typeof curPart === 'number' && !isArrayOrTypedArray(curCont)) {
throw 'array index but container is not an array';
}

Expand Down Expand Up @@ -211,7 +211,7 @@ function joinPropStr(propStr, newPart) {

// handle special -1 array index
function setArrayAll(containerArray, innerParts, val, propStr) {
var arrayVal = isArray(val),
var arrayVal = isArrayOrTypedArray(val),
allSet = true,
thisVal = val,
thisPropStr = propStr.replace('-1', 0),
Expand Down Expand Up @@ -261,7 +261,7 @@ function pruneContainers(containerLevels) {
propPart = containerLevels[i][1];

remainingKeys = false;
if(isArray(curCont)) {
if(isArrayOrTypedArray(curCont)) {
for(j = curCont.length - 1; j >= 0; j--) {
if(isDeletable(curCont[j], joinPropStr(propPart, j))) {
if(remainingKeys) curCont[j] = undefined;
Expand All @@ -287,7 +287,7 @@ function pruneContainers(containerLevels) {
function emptyObj(obj) {
if(obj === undefined || obj === null) return true;
if(typeof obj !== 'object') return false; // any plain value
if(isArray(obj)) return !obj.length; // []
if(isArrayOrTypedArray(obj)) return !obj.length; // []
return !Object.keys(obj).length; // {}
}

Expand Down
4 changes: 2 additions & 2 deletions src/lib/relink_private.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@

'use strict';

var isArray = require('./is_array');
var isArrayOrTypedArray = require('./is_array').isArrayOrTypedArray;
var isPlainObject = require('./is_plain_object');

/**
Expand All @@ -35,7 +35,7 @@ module.exports = function relinkPrivateKeys(toContainer, fromContainer) {

toContainer[k] = fromVal;
}
else if(isArray(fromVal) && isArray(toVal) && isPlainObject(fromVal[0])) {
else if(isArrayOrTypedArray(fromVal) && isArrayOrTypedArray(toVal) && isPlainObject(fromVal[0])) {

// filter out data_array items that can contain user objects
// most of the time the toVal === fromVal check will catch these early
Expand Down
5 changes: 2 additions & 3 deletions src/plots/array_container_defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

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


/** Convenience wrapper for making array container logic DRY and consistent
*
* @param {object} parentObjIn
Expand Down Expand Up @@ -46,7 +45,7 @@ module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut

var previousContOut = parentObjOut[name];

var contIn = Lib.isArray(parentObjIn[name]) ? parentObjIn[name] : [],
var contIn = Lib.isArrayOrTypedArray(parentObjIn[name]) ? parentObjIn[name] : [],
contOut = parentObjOut[name] = [],
i;

Expand All @@ -70,7 +69,7 @@ module.exports = function handleArrayContainerDefaults(parentObjIn, parentObjOut

// in case this array gets its defaults rebuilt independent of the whole layout,
// relink the private keys just for this array.
if(Lib.isArray(previousContOut)) {
if(Lib.isArrayOrTypedArray(previousContOut)) {
var len = Math.min(previousContOut.length, contOut.length);
for(i = 0; i < len; i++) {
Lib.relinkPrivateKeys(contOut[i], previousContOut[i]);
Expand Down
11 changes: 8 additions & 3 deletions src/plots/cartesian/set_convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -405,10 +405,15 @@ module.exports = function setConvert(ax, fullLayout) {
if(axLetter in trace) {
arrayIn = trace[axLetter];
len = trace._length || arrayIn.length;
arrayOut = new Array(len);

for(i = 0; i < len; i++) {
arrayOut[i] = ax.d2c(arrayIn[i], 0, cal);
if(ax.type === 'linear' && Lib.isTypedArray(arrayIn) && arrayIn.subarray) {
arrayOut = arrayIn.subarray(0, len);
Copy link
Contributor Author

@etpinard etpinard Feb 20, 2018

Choose a reason for hiding this comment

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

More info: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypedArray/subarray

Bypassing isNumeric(v) ? Number(v) : BADNAM coercion saves about ~50ms at 1e4 points during scatter calc step. 🐎

Copy link
Collaborator

Choose a reason for hiding this comment

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

Typed arrays can't hold undefined, only NaN, right? So do we need to switch to NaN as BADNUM? Though this has the follow-on problem that NaN === NaN doesn't work, so v === BADNUM would have to be replaced by IsNaN(v) when we already know it's a number and I guess IsNaN(v) && typeof v === 'number' if we don't... That's pretty annoying. But if we DON'T, won't we have errors dealing with missing data in typed arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, typed arrays "call" Number(v) on each item before "inserting" them into the typed array:

image

so yeah (I think) that means NaNs are the only non-number values accepted in typed arrays.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... so yeah connectgaps: false is probably broken for scatter with typed arrays. Let me check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just for completeness while you're looking into edge cases, (+/-)Infinity are also allowed in float typed arrays. Aside from users directly providing Infinity, I presume this comes up in gl code when we drop doubles to singles, if the input is too large. I believe (+/-)Infinity and NaN are the only non-numerics that have representations in standard floating point encodings.

Copy link
Contributor Author

@etpinard etpinard Feb 28, 2018

Choose a reason for hiding this comment

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

To me surprise, connectgaps: false is working fine with typed arrays:

image


Here are all the places we check equality with BADNUM:

image

by the looks of it, these are all places that check gd.calcdata values (i.e. not in a typed array). Luckily isNumeric is used elsewhere (which works for fine detecting NaNs in typed arrays`).

I'll test out a few more cases this afternoon.

Copy link
Contributor Author

@etpinard etpinard Feb 28, 2018

Choose a reason for hiding this comment

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

All right, I couldn't find an example of BADNUM breaking things for typed arrays.

Replacing BADNUMs with NaNs (and isNaN) might help us be more typed-array friendly down the road, but as long as we're using arrays of object of our calcdata for most things, BADNUM should work just fine.

Note that these two lines in scattergl calc aren't necessary (as Number(BADNUM) // -> NaN), but it makes things clear that BADNUM and typed array aren't the best of friends, so I'll keep it there.

} else {
arrayOut = new Array(len);
Copy link
Collaborator

Choose a reason for hiding this comment

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

In cases like this where we know the length and we know we're dealing with numbers, would we benefit by using typed arrays internally? When they're supported of course - we'd need a helper to revert to Array in IE9. And subject to my question about NaN.

For a later time regardless...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's an idea. Saving a few isNumeric (assuming isNaN is faster) calls downstream of the makeCalcdata should help a little bit.


for(i = 0; i < len; i++) {
arrayOut[i] = ax.d2c(arrayIn[i], 0, cal);
}
}
}
else {
Expand Down
10 changes: 5 additions & 5 deletions src/traces/carpet/cheater_basis.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

'use strict';

var isArray = require('../../lib').isArray;
var isArrayOrTypedArray = require('../../lib').isArrayOrTypedArray;

/*
* Construct a 2D array of cheater values given a, b, and a slope.
Expand All @@ -18,10 +18,10 @@ module.exports = function(a, b, cheaterslope) {
var i, j, ascal, bscal, aval, bval;
var data = [];

var na = isArray(a) ? a.length : a;
var nb = isArray(b) ? b.length : b;
var adata = isArray(a) ? a : null;
var bdata = isArray(b) ? b : null;
var na = isArrayOrTypedArray(a) ? a.length : a;
var nb = isArrayOrTypedArray(b) ? b.length : b;
var adata = isArrayOrTypedArray(a) ? a : null;
var bdata = isArrayOrTypedArray(b) ? b : null;

// If we're using data, scale it so that for data that's just barely
// not evenly spaced, the switch to value-based indexing is continuous.
Expand Down
2 changes: 1 addition & 1 deletion src/traces/parcoords/calc.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ var Lib = require('../../lib');
var wrap = require('../../lib/gup').wrap;

module.exports = function calc(gd, trace) {
var cs = !!trace.line.colorscale && Lib.isArray(trace.line.color);
var cs = !!trace.line.colorscale && Lib.isArrayOrTypedArray(trace.line.color);
var color = cs ? trace.line.color : Array.apply(0, Array(trace.dimensions.reduce(function(p, n) {return Math.max(p, n.values.length);}, 0))).map(function() {return 0.5;});
var cscale = cs ? trace.line.colorscale : [[0, trace.line.color], [1, trace.line.color]];

Expand Down
2 changes: 1 addition & 1 deletion src/traces/parcoords/defaults.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ function handleLineDefaults(traceIn, traceOut, defaultColor, layout, coerce) {

coerce('line.color', defaultColor);

if(hasColorscale(traceIn, 'line') && Lib.isArray(traceIn.line.color)) {
if(hasColorscale(traceIn, 'line') && Lib.isArrayOrTypedArray(traceIn.line.color)) {
coerce('line.colorscale');
colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: 'line.', cLetter: 'c'});
}
Expand Down
4 changes: 2 additions & 2 deletions src/traces/sankey/render.js
Original file line number Diff line number Diff line change
Expand Up @@ -87,15 +87,15 @@ function sankeyModel(layout, d, i) {
return {
pointNumber: i,
label: l,
color: Lib.isArray(nodeSpec.color) ? nodeSpec.color[i] : nodeSpec.color
color: Lib.isArrayOrTypedArray(nodeSpec.color) ? nodeSpec.color[i] : nodeSpec.color
};
});

var links = linkSpec.value.map(function(d, i) {
return {
pointNumber: i,
label: linkSpec.label[i],
color: Lib.isArray(linkSpec.color) ? linkSpec.color[i] : linkSpec.color,
color: Lib.isArrayOrTypedArray(linkSpec.color) ? linkSpec.color[i] : linkSpec.color,
source: linkSpec.source[i],
target: linkSpec.target[i],
value: d
Expand Down
52 changes: 45 additions & 7 deletions test/jasmine/tests/is_array_test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,6 @@
var Lib = require('@src/lib');

describe('isArray', function() {
'use strict';

var isArray = Lib.isArray;

describe('isArrayOrTypedArray', function() {
function A() {}

var shouldPass = [
Expand Down Expand Up @@ -35,13 +31,55 @@ describe('isArray', function() {

shouldPass.forEach(function(obj) {
it('treats ' + JSON.stringify(obj) + ' as an array', function() {
expect(isArray(obj)).toBe(true);
expect(Lib.isArrayOrTypedArray(obj)).toBe(true);
});
});

shouldFail.forEach(function(obj) {
it('treats ' + JSON.stringify(obj !== window ? obj : 'window') + ' as NOT an array', function() {
expect(Lib.isArrayOrTypedArray(obj)).toBe(false);
});
});
});

describe('isTypedArray', function() {
function A() {}

var shouldPass = [
new Float32Array(1),
new Int32Array([1, 2, 3])
];

var shouldFail = [
new Array(10),
[],
A,
new A(),
document,
window,
null,
undefined,
'string',
true,
false,
NaN,
Infinity,
/foo/,
'\n',
new Date(),
new RegExp('foo'),
new String('string')
];

shouldPass.forEach(function(obj) {
it('treats ' + JSON.stringify(obj) + ' as an array', function() {
expect(Lib.isTypedArray(obj)).toBe(true);
});
});

shouldFail.forEach(function(obj) {
it('treats ' + JSON.stringify(obj !== window ? obj : 'window') + ' as NOT an array', function() {
expect(isArray(obj)).toBe(false);
expect(Lib.isTypedArray(obj)).toBe(false);
});
});
});