From 1196fd3a780808d451dd8a6517c1794eb4fac6c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 31 Mar 2017 14:30:49 -0400 Subject: [PATCH 1/6] flatten scattegeo_test.js --- test/jasmine/tests/scattergeo_test.js | 120 ++++++++++++++------------ 1 file changed, 66 insertions(+), 54 deletions(-) diff --git a/test/jasmine/tests/scattergeo_test.js b/test/jasmine/tests/scattergeo_test.js index 2391cdfb512..0e442076ec4 100644 --- a/test/jasmine/tests/scattergeo_test.js +++ b/test/jasmine/tests/scattergeo_test.js @@ -1,73 +1,85 @@ var ScatterGeo = require('@src/traces/scattergeo'); -describe('Test scattergeo', function() { - 'use strict'; - describe('supplyDefaults', function() { - var traceIn, - traceOut; +describe('Test scattergeo defaults', function() { + var traceIn, + traceOut; - var defaultColor = '#444', - layout = {}; + var defaultColor = '#444', + layout = {}; - beforeEach(function() { - traceOut = {}; - }); + beforeEach(function() { + traceOut = {}; + }); + + it('should slice lat if it it longer than lon', function() { + traceIn = { + lon: [-75], + lat: [45, 45, 45] + }; - it('should slice lat if it it longer than lon', function() { - traceIn = { - lon: [-75], - lat: [45, 45, 45] - }; + ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); + expect(traceOut.lat).toEqual([45]); + expect(traceOut.lon).toEqual([-75]); + }); + + it('should slice lon if it it longer than lat', function() { + traceIn = { + lon: [-75, -75, -75], + lat: [45] + }; + ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); + expect(traceOut.lat).toEqual([45]); + expect(traceOut.lon).toEqual([-75]); + }); + + it('should not coerce lat and lon if locations is valid', function() { + traceIn = { + locations: ['CAN', 'USA'], + lon: [20, 40], + lat: [20, 40] + }; + + ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); + expect(traceOut.lon).toBeUndefined(); + expect(traceOut.lat).toBeUndefined(); + }); + + it('should make trace invisible if lon or lat is omitted and locations not given', function() { + function testOne() { ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); - expect(traceOut.lat).toEqual([45]); - expect(traceOut.lon).toEqual([-75]); + expect(traceOut.visible).toBe(false); + } + + traceIn = { + lat: [45, 45, 45] + }; + testOne(); + + traceIn = { + lon: [-75, -75, -75] + }; + traceOut = {}; + testOne(); + + traceIn = {}; + traceOut = {}; + testOne(); + }); +}); + }); - it('should slice lon if it it longer than lat', function() { - traceIn = { - lon: [-75, -75, -75], - lat: [45] - }; - ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); - expect(traceOut.lat).toEqual([45]); - expect(traceOut.lon).toEqual([-75]); }); - it('should not coerce lat and lon if locations is valid', function() { - traceIn = { - locations: ['CAN', 'USA'], - lon: [20, 40], - lat: [20, 40] - }; - ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); - expect(traceOut.lon).toBeUndefined(); - expect(traceOut.lat).toBeUndefined(); }); - it('should make trace invisible if lon or lat is omitted and locations not given', function() { - function testOne() { - ScatterGeo.supplyDefaults(traceIn, traceOut, defaultColor, layout); - expect(traceOut.visible).toBe(false); - } - - traceIn = { - lat: [45, 45, 45] - }; - testOne(); - - traceIn = { - lon: [-75, -75, -75] - }; - traceOut = {}; - testOne(); - - traceIn = {}; - traceOut = {}; - testOne(); + + + }); }); From 2a91da672fbed5023849da99b9c9ad8f24b4e896 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 31 Mar 2017 14:34:07 -0400 Subject: [PATCH 2/6] set lonlat to [BADNUM, BADNUM] if lon OR lat is non-numeric - to improve performance in check downstream - add calc tests to scattergeo_test.js --- src/traces/scattergeo/calc.js | 8 +- test/jasmine/tests/scattergeo_test.js | 137 ++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 5 deletions(-) diff --git a/src/traces/scattergeo/calc.js b/src/traces/scattergeo/calc.js index e00cdb0341a..8f2fcdee80f 100644 --- a/src/traces/scattergeo/calc.js +++ b/src/traces/scattergeo/calc.js @@ -26,14 +26,12 @@ module.exports = function calc(gd, trace) { if(hasLocationData) { var loc = trace.locations[i]; calcPt.loc = typeof loc === 'string' ? loc : null; - } - else { + } else { var lon = trace.lon[i]; var lat = trace.lat[i]; - calcPt.lonlat = new Array(2); - calcPt.lonlat[0] = isNumeric(lon) ? +lon : BADNUM; - calcPt.lonlat[1] = isNumeric(lat) ? +lat : BADNUM; + if(isNumeric(lon) && isNumeric(lat)) calcPt.lonlat = [+lon, +lat]; + else calcPt.lonlat = [BADNUM, BADNUM]; } } diff --git a/test/jasmine/tests/scattergeo_test.js b/test/jasmine/tests/scattergeo_test.js index 0e442076ec4..33581c718bd 100644 --- a/test/jasmine/tests/scattergeo_test.js +++ b/test/jasmine/tests/scattergeo_test.js @@ -1,3 +1,7 @@ +var Plots = require('@src/plots/plots'); +var Lib = require('@src/lib'); +var BADNUM = require('@src/constants/numerical').BADNUM; + var ScatterGeo = require('@src/traces/scattergeo'); @@ -69,18 +73,151 @@ describe('Test scattergeo defaults', function() { }); }); +describe('Test scattergeo calc', function() { + + function _calc(opts) { + var base = { type: 'scattermapbox' }; + var trace = Lib.extendFlat({}, base, opts); + var gd = { data: [trace] }; + + Plots.supplyDefaults(gd); + + var fullTrace = gd._fullData[0]; + return ScatterGeo.calc(gd, fullTrace); + } + + it('should place lon/lat data in lonlat pairs', function() { + var calcTrace = _calc({ + lon: [10, 20, 30], + lat: [20, 30, 10] + }); + + expect(calcTrace).toEqual([ + { lonlat: [10, 20] }, + { lonlat: [20, 30] }, + { lonlat: [30, 10] } + ]); + }); + + it('should coerce numeric strings lon/lat data into numbers', function() { + var calcTrace = _calc({ + lon: [10, 20, '30', '40'], + lat: [20, '30', 10, '50'] + }); + + expect(calcTrace).toEqual([ + { lonlat: [10, 20] }, + { lonlat: [20, 30] }, + { lonlat: [30, 10] }, + { lonlat: [40, 50] } + ]); + }); + + it('should set non-numeric values lon/lat pairs to BADNUM', function() { + var calcTrace = _calc({ + lon: [null, 10, null, null, 20, '30', null, '40', null, 10], + lat: [10, 20, '30', null, 10, '50', null, 60, null, null] }); + expect(calcTrace).toEqual([ + { lonlat: [BADNUM, BADNUM] }, + { lonlat: [10, 20] }, + { lonlat: [BADNUM, BADNUM] }, + { lonlat: [BADNUM, BADNUM] }, + { lonlat: [20, 10] }, + { lonlat: [30, 50] }, + { lonlat: [BADNUM, BADNUM] }, + { lonlat: [40, 60] }, + { lonlat: [BADNUM, BADNUM] }, + { lonlat: [BADNUM, BADNUM] } + ]); + }); + it('should fill array text (base case)', function() { + var calcTrace = _calc({ + lon: [10, 20, 30, null, 40], + lat: [20, 30, 10, 'no-good', 50], + text: ['A', 'B', 'C', 'D', 'E'] }); + expect(calcTrace).toEqual([ + { lonlat: [10, 20], tx: 'A' }, + { lonlat: [20, 30], tx: 'B' }, + { lonlat: [30, 10], tx: 'C' }, + { lonlat: [BADNUM, BADNUM], tx: 'D' }, + { lonlat: [40, 50], tx: 'E' } + ]); + }); + it('should fill array text (invalid entry case)', function() { + var calcTrace = _calc({ + lon: [10, 20, 30, null, 40], + lat: [20, 30, 10, 'no-good', 50], + text: ['A', null, 'C', 'D', {}] }); + expect(calcTrace).toEqual([ + { lonlat: [10, 20], tx: 'A' }, + { lonlat: [20, 30], tx: null }, + { lonlat: [30, 10], tx: 'C' }, + { lonlat: [BADNUM, BADNUM], tx: 'D' }, + { lonlat: [40, 50], tx: {} } + ]); + }); + it('should fill array marker attributes (base case)', function() { + var calcTrace = _calc({ + lon: [10, 20, null, 30], + lat: [20, 30, null, 10], + marker: { + color: ['red', 'blue', 'green', 'yellow'], + size: [10, 20, 8, 10] + } + }); + expect(calcTrace).toEqual([ + { lonlat: [10, 20], mc: 'red', ms: 10 }, + { lonlat: [20, 30], mc: 'blue', ms: 20 }, + { lonlat: [BADNUM, BADNUM], mc: 'green', ms: 8 }, + { lonlat: [30, 10], mc: 'yellow', ms: 10 } + ]); + }); + it('should fill array marker attributes (invalid scale case)', function() { + var calcTrace = _calc({ + lon: [10, 20, null, 30], + lat: [20, 30, null, 10], + marker: { + color: [0, null, 5, 10], + size: [10, NaN, 8, 10], + colorscale: [ + [0, 'blue'], [0.5, 'red'], [1, 'green'] + ] + } }); + + expect(calcTrace).toEqual([ + { lonlat: [10, 20], mc: 0, ms: 10 }, + { lonlat: [20, 30], mc: null, ms: NaN }, + { lonlat: [BADNUM, BADNUM], mc: 5, ms: 8 }, + { lonlat: [30, 10], mc: 10, ms: 10 } + ]); }); + it('should fill marker attributes (symbol case)', function() { + var calcTrace = _calc({ + lon: [10, 20, null, 30], + lat: [20, 30, null, 10], + marker: { + symbol: ['cross', 'square', 'diamond', null] + } + }); + + expect(calcTrace).toEqual([ + { lonlat: [10, 20], mx: 'cross' }, + { lonlat: [20, 30], mx: 'square' }, + { lonlat: [BADNUM, BADNUM], mx: 'diamond' }, + { lonlat: [30, 10], mx: null } + ]); + }); }); From 28d94da4530c76c25fec9e91ee3e9a99c972d45a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 31 Mar 2017 14:35:11 -0400 Subject: [PATCH 3/6] only need to check lonlat[0] === BADNUM - to determine if calcdata item should be skipped --- src/lib/geojson_utils.js | 2 +- src/traces/scattergeo/hover.js | 2 +- src/traces/scattergeo/plot.js | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/lib/geojson_utils.js b/src/lib/geojson_utils.js index 183685c42ed..b123c1c68ba 100644 --- a/src/lib/geojson_utils.js +++ b/src/lib/geojson_utils.js @@ -33,7 +33,7 @@ exports.calcTraceToLineCoords = function(calcTrace) { var calcPt = calcTrace[i]; var lonlat = calcPt.lonlat; - if(lonlat[0] !== BADNUM && lonlat[1] !== BADNUM) { + if(lonlat[0] !== BADNUM) { lineString.push(lonlat); } else if(!connectgaps && lineString.length > 0) { coords.push(lineString); diff --git a/src/traces/scattergeo/hover.js b/src/traces/scattergeo/hover.js index 9bf608eaa7d..dd609047e99 100644 --- a/src/traces/scattergeo/hover.js +++ b/src/traces/scattergeo/hover.js @@ -31,7 +31,7 @@ module.exports = function hoverPoints(pointData) { function distFn(d) { var lonlat = d.lonlat; - if(lonlat[0] === BADNUM || lonlat[1] === BADNUM) return Infinity; + if(lonlat[0] === BADNUM) return Infinity; if(geo.isLonLatOverEdges(lonlat)) return Infinity; diff --git a/src/traces/scattergeo/plot.js b/src/traces/scattergeo/plot.js index e713b297dd7..10451a247be 100644 --- a/src/traces/scattergeo/plot.js +++ b/src/traces/scattergeo/plot.js @@ -27,8 +27,7 @@ module.exports = function plot(geo, calcData) { function keyFunc(d) { return d[0].trace.uid; } function removeBADNUM(d, node) { - var lonlat = d.lonlat; - if(lonlat[0] === BADNUM || lonlat[1] === BADNUM) { + if(d.lonlat[0] === BADNUM) { d3.select(node).remove(); } } From da7748c454441498265f491b688fd7fea85a1717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 31 Mar 2017 14:36:34 -0400 Subject: [PATCH 4/6] bump mapbox-gl to 0.33.1 --- package.json | 2 +- src/plots/mapbox/mapbox.js | 2 +- test/jasmine/tests/mapbox_test.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/package.json b/package.json index 4f83fa1e065..34f03eaedbd 100644 --- a/package.json +++ b/package.json @@ -81,7 +81,7 @@ "gl-shader": "4.2.0", "gl-spikes2d": "^1.0.1", "gl-surface3d": "^1.3.0", - "mapbox-gl": "^0.22.0", + "mapbox-gl": "^0.33.1", "mouse-change": "^1.4.0", "mouse-wheel": "^1.0.2", "ndarray": "^1.0.18", diff --git a/src/plots/mapbox/mapbox.js b/src/plots/mapbox/mapbox.js index 3c3185b1c44..163c62264e0 100644 --- a/src/plots/mapbox/mapbox.js +++ b/src/plots/mapbox/mapbox.js @@ -191,7 +191,7 @@ proto.updateMap = function(calcData, fullLayout, resolve, reject) { self.styleObj = styleObj; map.setStyle(styleObj.style); - map.style.once('load', function() { + map.once('style.load', function() { // need to rebuild trace layers on reload // to avoid 'lost event' errors diff --git a/test/jasmine/tests/mapbox_test.js b/test/jasmine/tests/mapbox_test.js index d494573ecb2..4ac8d6acb1d 100644 --- a/test/jasmine/tests/mapbox_test.js +++ b/test/jasmine/tests/mapbox_test.js @@ -859,7 +859,7 @@ describe('@noCI, mapbox plots', function() { var subplot = gd._fullLayout.mapbox._subplot, map = subplot.map; - var sources = map.style.sources, + var sources = map.style.sourceCaches, layers = map.style._layers, uid = subplot.uid; From 1ca15262251e911cc260e06857104b2c23140005 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 31 Mar 2017 14:39:54 -0400 Subject: [PATCH 5/6] refactor scattermapbox calc & convert steps - :hocho: scattermapbox/calc.js, reuse scattergeo/calc.js instead! - this makes calcdata items 1-1 with fullData coords (as all our other trace types) - Use identity function type in properties (available since 0.26.0) to replace brutal hack with `stops` :tada: --- src/traces/scattermapbox/calc.js | 101 ------------ src/traces/scattermapbox/convert.js | 158 ++++++------------- src/traces/scattermapbox/hover.js | 12 +- src/traces/scattermapbox/index.js | 2 +- test/jasmine/tests/scattermapbox_test.js | 191 +++-------------------- 5 files changed, 77 insertions(+), 387 deletions(-) delete mode 100644 src/traces/scattermapbox/calc.js diff --git a/src/traces/scattermapbox/calc.js b/src/traces/scattermapbox/calc.js deleted file mode 100644 index 6f6230851e8..00000000000 --- a/src/traces/scattermapbox/calc.js +++ /dev/null @@ -1,101 +0,0 @@ -/** -* Copyright 2012-2017, Plotly, Inc. -* All rights reserved. -* -* This source code is licensed under the MIT license found in the -* LICENSE file in the root directory of this source tree. -*/ - - -'use strict'; - -var isNumeric = require('fast-isnumeric'); - -var Lib = require('../../lib'); -var Colorscale = require('../../components/colorscale'); - -var subtypes = require('../scatter/subtypes'); -var calcMarkerColorscale = require('../scatter/colorscale_calc'); -var makeBubbleSizeFn = require('../scatter/make_bubble_size_func'); - - -module.exports = function calc(gd, trace) { - var len = trace.lon.length, - marker = trace.marker; - - var hasMarkers = subtypes.hasMarkers(trace), - hasColorArray = (hasMarkers && Array.isArray(marker.color)), - hasSizeArray = (hasMarkers && Array.isArray(marker.size)), - hasSymbolArray = (hasMarkers && Array.isArray(marker.symbol)), - hasTextArray = Array.isArray(trace.text); - - calcMarkerColorscale(trace); - - var colorFn = Colorscale.hasColorscale(trace, 'marker') ? - Colorscale.makeColorScaleFunc( - Colorscale.extractScale( - marker.colorscale, - marker.cmin, - marker.cmax - ) - ) : - Lib.identity; - - var sizeFn = subtypes.isBubble(trace) ? - makeBubbleSizeFn(trace) : - Lib.identity; - - var calcTrace = [], - cnt = 0; - - // Different than cartesian calc step - // as skip over non-numeric lon, lat pairs. - // This makes the hover and convert calculations simpler. - - for(var i = 0; i < len; i++) { - var lon = trace.lon[i], - lat = trace.lat[i]; - - if(!isNumeric(lon) || !isNumeric(lat)) { - if(cnt > 0) calcTrace[cnt - 1].gapAfter = true; - continue; - } - - var calcPt = {}; - cnt++; - - // coerce numeric strings into numbers - calcPt.lonlat = [+lon, +lat]; - - if(hasMarkers) { - - if(hasColorArray) { - var mc = marker.color[i]; - - calcPt.mc = mc; - calcPt.mcc = colorFn(mc); - } - - if(hasSizeArray) { - var ms = marker.size[i]; - - calcPt.ms = ms; - calcPt.mrc = sizeFn(ms); - } - - if(hasSymbolArray) { - var mx = marker.symbol[i]; - calcPt.mx = (typeof mx === 'string') ? mx : 'circle'; - } - } - - if(hasTextArray) { - var tx = trace.text[i]; - calcPt.tx = (typeof tx === 'string') ? tx : ''; - } - - calcTrace.push(calcPt); - } - - return calcTrace; -}; diff --git a/src/traces/scattermapbox/convert.js b/src/traces/scattermapbox/convert.js index 3e46c8d8950..4215fa919f1 100644 --- a/src/traces/scattermapbox/convert.js +++ b/src/traces/scattermapbox/convert.js @@ -10,14 +10,15 @@ 'use strict'; var Lib = require('../../lib'); +var BADNUM = require('../../constants/numerical').BADNUM; var geoJsonUtils = require('../../lib/geojson_utils'); - +var Colorscale = require('../../components/colorscale'); +var makeBubbleSizeFn = require('../scatter/make_bubble_size_func'); var subTypes = require('../scatter/subtypes'); var convertTextOpts = require('../../plots/mapbox/convert_text_opts'); -var COLOR_PROP = 'circle-color'; -var SIZE_PROP = 'circle-radius'; - +var COLOR_PROP = 'mcc'; +var SIZE_PROP = 'mrc'; module.exports = function convert(calcTrace) { var trace = calcTrace[0].trace; @@ -42,17 +43,17 @@ module.exports = function convert(calcTrace) { symbol: symbol }; - // early return if not visible or placeholder - if(!isVisible || calcTrace[0].placeholder) return opts; + // early return if not visible + if(!isVisible) return opts; // fill layer and line layer use the same coords - var coords; + var lineCoords; if(hasFill || hasLines) { - coords = geoJsonUtils.calcTraceToLineCoords(calcTrace); + lineCoords = geoJsonUtils.calcTraceToLineCoords(calcTrace); } if(hasFill) { - fill.geojson = geoJsonUtils.makePolygon(coords); + fill.geojson = geoJsonUtils.makePolygon(lineCoords); fill.layout.visibility = 'visible'; Lib.extendFlat(fill.paint, { @@ -61,7 +62,7 @@ module.exports = function convert(calcTrace) { } if(hasLines) { - line.geojson = geoJsonUtils.makeLine(coords); + line.geojson = geoJsonUtils.makeLine(lineCoords); line.layout.visibility = 'visible'; Lib.extendFlat(line.paint, { @@ -74,17 +75,20 @@ module.exports = function convert(calcTrace) { } if(hasCircles) { - var hash = {}; - hash[COLOR_PROP] = {}; - hash[SIZE_PROP] = {}; - - circle.geojson = makeCircleGeoJSON(calcTrace, hash); + circle.geojson = makeCircleGeoJSON(calcTrace); circle.layout.visibility = 'visible'; + var circleColor = Array.isArray(trace.marker.color) ? + { type: 'identity', property: COLOR_PROP } : + trace.marker.color; + var circleRadius = Array.isArray(trace.marker.size) ? + { type: 'identity', property: SIZE_PROP } : + trace.marker.size / 2; + Lib.extendFlat(circle.paint, { 'circle-opacity': trace.opacity * trace.marker.opacity, - 'circle-color': calcCircleColor(trace, hash), - 'circle-radius': calcCircleRadius(trace, hash) + 'circle-color': circleColor, + 'circle-radius': circleRadius }); } @@ -141,33 +145,24 @@ function initContainer() { }; } -// N.B. `hash` is mutated here -// -// The `hash` object contains mapping between values -// (e.g. calculated marker.size and marker.color items) -// and their index in the input arrayOk attributes. -// -// GeoJSON features have their 'data-driven' properties set to -// the index of the first value found in the data. -// -// The `hash` object is then converted to mapbox `stops` arrays -// mapping index to value. -// -// The solution prove to be more robust than trying to generate -// `stops` arrays from scale functions. -function makeCircleGeoJSON(calcTrace, hash) { +function makeCircleGeoJSON(calcTrace) { var trace = calcTrace[0].trace; + var marker = trace.marker; + + var colorFn; + if(Colorscale.hasColorscale(trace, 'marker')) { + colorFn = Colorscale.makeColorScaleFunc( + Colorscale.extractScale(marker.colorscale, marker.cmin, marker.cmax) + ); + } else if(Array.isArray(marker.color)) { + colorFn = Lib.identity; + } - var marker = trace.marker, - hasColorArray = Array.isArray(marker.color), - hasSizeArray = Array.isArray(marker.size); - - // Translate vals in trace arrayOk containers - // into a val-to-index hash object - function translate(props, key, val, index) { - if(hash[key][val] === undefined) hash[key][val] = index; - - props[key] = hash[key][val]; + var sizeFn; + if(subTypes.isBubble(trace)) { + sizeFn = makeBubbleSizeFn(trace); + } else if(Array.isArray(marker.size)) { + sizeFn = Lib.identity; } var features = []; @@ -175,9 +170,11 @@ function makeCircleGeoJSON(calcTrace, hash) { for(var i = 0; i < calcTrace.length; i++) { var calcPt = calcTrace[i]; + if(isBADNUM(calcPt.lonlat)) continue; + var props = {}; - if(hasColorArray) translate(props, COLOR_PROP, calcPt.mcc, i); - if(hasSizeArray) translate(props, SIZE_PROP, calcPt.mrc, i); + if(colorFn) props[COLOR_PROP] = colorFn(calcPt.mc); + if(sizeFn) props[SIZE_PROP] = sizeFn(calcPt.ms); features.push({ type: 'Feature', @@ -197,10 +194,9 @@ function makeCircleGeoJSON(calcTrace, hash) { function makeSymbolGeoJSON(calcTrace) { var trace = calcTrace[0].trace; - - var marker = trace.marker || {}, - symbol = marker.symbol, - text = trace.text; + var marker = trace.marker || {}; + var symbol = marker.symbol; + var text = trace.text; var fillSymbol = (symbol !== 'circle') ? getFillFunc(symbol) : @@ -215,6 +211,8 @@ function makeSymbolGeoJSON(calcTrace) { for(var i = 0; i < calcTrace.length; i++) { var calcPt = calcTrace[i]; + if(isBADNUM(calcPt.lonlat)) continue; + features.push({ type: 'Feature', geometry: { @@ -234,72 +232,16 @@ function makeSymbolGeoJSON(calcTrace) { }; } -function calcCircleColor(trace, hash) { - var marker = trace.marker, - out; - - if(Array.isArray(marker.color)) { - var vals = Object.keys(hash[COLOR_PROP]), - stops = []; - - for(var i = 0; i < vals.length; i++) { - var val = vals[i]; - - stops.push([ hash[COLOR_PROP][val], val ]); - } - - out = { - property: COLOR_PROP, - stops: stops - }; - - } - else { - out = marker.color; - } - - return out; -} - -function calcCircleRadius(trace, hash) { - var marker = trace.marker, - out; - - if(Array.isArray(marker.size)) { - var vals = Object.keys(hash[SIZE_PROP]), - stops = []; - - for(var i = 0; i < vals.length; i++) { - var val = vals[i]; - - stops.push([ hash[SIZE_PROP][val], +val ]); - } - - // stops indices must be sorted - stops.sort(function(a, b) { - return a[0] - b[0]; - }); - - out = { - property: SIZE_PROP, - stops: stops - }; - } - else { - out = marker.size / 2; - } - - return out; +function isBADNUM(lonlat) { + return lonlat[0] === BADNUM || lonlat[1] === BADNUM; } function getFillFunc(attr) { if(Array.isArray(attr)) { return function(v) { return v; }; - } - else if(attr) { + } else if(attr) { return function() { return attr; }; - } - else { + } else { return blankFillFunc; } } diff --git a/src/traces/scattermapbox/hover.js b/src/traces/scattermapbox/hover.js index 088d35f6dc5..8c14f03bcb0 100644 --- a/src/traces/scattermapbox/hover.js +++ b/src/traces/scattermapbox/hover.js @@ -11,6 +11,7 @@ var Fx = require('../../plots/cartesian/graph_interact'); var getTraceColor = require('../scatter/get_trace_color'); +var BADNUM = require('../../constants/numerical').BADNUM; module.exports = function hoverPoints(pointData, xval, yval) { @@ -31,10 +32,13 @@ module.exports = function hoverPoints(pointData, xval, yval) { var xval2 = xval - lonShift; function distFn(d) { - var lonlat = d.lonlat, - dx = Math.abs(xa.c2p(lonlat) - xa.c2p([xval2, lonlat[1]])), - dy = Math.abs(ya.c2p(lonlat) - ya.c2p([lonlat[0], yval])), - rad = Math.max(3, d.mrc || 0); + var lonlat = d.lonlat; + + if(lonlat[0] === BADNUM) return Infinity; + + var dx = Math.abs(xa.c2p(lonlat) - xa.c2p([xval2, lonlat[1]])); + var dy = Math.abs(ya.c2p(lonlat) - ya.c2p([lonlat[0], yval])); + var rad = Math.max(3, d.mrc || 0); return Math.max(Math.sqrt(dx * dx + dy * dy) - rad, 1 - 3 / rad); } diff --git a/src/traces/scattermapbox/index.js b/src/traces/scattermapbox/index.js index fa32f9847a0..6de4241ed82 100644 --- a/src/traces/scattermapbox/index.js +++ b/src/traces/scattermapbox/index.js @@ -14,7 +14,7 @@ var ScatterMapbox = {}; ScatterMapbox.attributes = require('./attributes'); ScatterMapbox.supplyDefaults = require('./defaults'); ScatterMapbox.colorbar = require('../scatter/colorbar'); -ScatterMapbox.calc = require('./calc'); +ScatterMapbox.calc = require('../scattergeo/calc'); ScatterMapbox.hoverPoints = require('./hover'); ScatterMapbox.eventData = require('./event_data'); ScatterMapbox.plot = require('./plot'); diff --git a/test/jasmine/tests/scattermapbox_test.js b/test/jasmine/tests/scattermapbox_test.js index 88aa8adfdb0..47dc7dfe6ff 100644 --- a/test/jasmine/tests/scattermapbox_test.js +++ b/test/jasmine/tests/scattermapbox_test.js @@ -112,143 +112,6 @@ describe('scattermapbox defaults', function() { }); }); -describe('scattermapbox calc', function() { - 'use strict'; - - function _calc(trace) { - var gd = { data: [trace] }; - - Plots.supplyDefaults(gd); - - var fullTrace = gd._fullData[0]; - return ScatterMapbox.calc(gd, fullTrace); - } - - var base = { type: 'scattermapbox' }; - - it('should place lon/lat data in lonlat pairs', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, 30], - lat: [20, 30, 10] - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20] }, - { lonlat: [20, 30] }, - { lonlat: [30, 10] } - ]); - }); - - it('should coerce numeric strings lon/lat data into numbers', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, '30', '40'], - lat: [20, '30', 10, '50'] - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20] }, - { lonlat: [20, 30] }, - { lonlat: [30, 10] }, - { lonlat: [40, 50] } - ]); - }); - - it('should keep track of gaps in data', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [null, 10, null, null, 20, '30', null, '40', null, 10], - lat: [10, 20, '30', null, 10, '50', null, 60, null, null] - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20], gapAfter: true }, - { lonlat: [20, 10] }, - { lonlat: [30, 50], gapAfter: true }, - { lonlat: [40, 60], gapAfter: true } - ]); - }); - - it('should fill array text (base case)', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, 30], - lat: [20, 30, 10], - text: ['A', 'B', 'C'] - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20], tx: 'A' }, - { lonlat: [20, 30], tx: 'B' }, - { lonlat: [30, 10], tx: 'C' } - ]); - }); - - it('should fill array text (invalid entry case)', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, 30], - lat: [20, 30, 10], - text: ['A', 'B', null] - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20], tx: 'A' }, - { lonlat: [20, 30], tx: 'B' }, - { lonlat: [30, 10], tx: '' } - ]); - }); - - it('should fill array marker attributes (base case)', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, null, 30], - lat: [20, 30, null, 10], - marker: { - color: ['red', 'blue', 'green', 'yellow'], - size: [10, 20, 8, 10] - } - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20], mc: 'red', ms: 10, mcc: 'red', mrc: 5 }, - { lonlat: [20, 30], mc: 'blue', ms: 20, mcc: 'blue', mrc: 10, gapAfter: true }, - { lonlat: [30, 10], mc: 'yellow', ms: 10, mcc: 'yellow', mrc: 5 } - ]); - }); - - it('should fill array marker attributes (invalid scale case)', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, null, 30], - lat: [20, 30, null, 10], - marker: { - color: [0, null, 5, 10], - size: [10, NaN, 8, 10], - colorscale: [ - [0, 'blue'], [0.5, 'red'], [1, 'green'] - ] - } - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20], mc: 0, ms: 10, mcc: 'rgb(0, 0, 255)', mrc: 5 }, - { lonlat: [20, 30], mc: null, ms: NaN, mcc: '#444', mrc: 0, gapAfter: true }, - { lonlat: [30, 10], mc: 10, ms: 10, mcc: 'rgb(0, 128, 0)', mrc: 5 } - ]); - }); - - it('should fill marker attributes (symbol case)', function() { - var calcTrace = _calc(Lib.extendFlat({}, base, { - lon: [10, 20, null, 30], - lat: [20, 30, null, 10], - marker: { - symbol: ['monument', 'music', 'harbor', null] - } - })); - - expect(calcTrace).toEqual([ - { lonlat: [10, 20], mx: 'monument' }, - { lonlat: [20, 30], mx: 'music', gapAfter: true }, - { lonlat: [30, 10], mx: 'circle' } - ]); - }); -}); - describe('scattermapbox convert', function() { 'use strict'; @@ -286,29 +149,26 @@ describe('scattermapbox convert', function() { assertVisibility(opts, ['none', 'none', 'visible', 'none']); expect(opts.circle.paint['circle-color']).toEqual({ - property: 'circle-color', - stops: [ - [0, 'rgb(220, 220, 220)'], [1, '#444'], [2, 'rgb(178, 10, 28)'] - ] - }, 'circle-color stops'); + type: 'identity', + property: 'mcc' + }); expect(opts.circle.paint['circle-radius']).toEqual({ - property: 'circle-radius', - stops: [ [0, 5], [1, 10], [2, 0] ] - }, 'circle-radius stops'); + type: 'identity', + property: 'mrc' + }); - var circleProps = opts.circle.geojson.features.map(function(f) { + var props = opts.circle.geojson.features.map(function(f) { return f.properties; }); - // N.B repeated values have same geojson props - expect(circleProps).toEqual([ - { 'circle-color': 0, 'circle-radius': 0 }, - { 'circle-color': 1, 'circle-radius': 1 }, - { 'circle-color': 2, 'circle-radius': 2 }, - { 'circle-color': 1, 'circle-radius': 2 }, - { 'circle-color': 1, 'circle-radius': 2 } - ], 'geojson feature properties'); + expect(props).toEqual([ + { mcc: 'rgb(220, 220, 220)', mrc: 5 }, + { mcc: '#444', mrc: 10 }, + { mcc: 'rgb(178, 10, 28)', mrc: 0 }, + { mcc: '#444', mrc: 0 }, + { mcc: '#444', mrc: 0 } + ], 'circle properties'); }); it('should generate correct output for fill + markers + lines traces', function() { @@ -376,7 +236,7 @@ describe('scattermapbox convert', function() { return f.properties.text; }); - expect(actualText).toEqual(['A', 'B', 'C', 'F', '']); + expect(actualText).toEqual(['A', 'B', 'C', 'F', undefined]); }); it('should generate correct output for lines traces with trailing gaps', function() { @@ -426,23 +286,6 @@ describe('scattermapbox convert', function() { }); }); - it('should generate correct output for markers + circle bubbles traces with repeated values', function() { - var opts = _convert(Lib.extendFlat({}, base, { - lon: ['-96.796988', '-81.379236', '-85.311819', ''], - lat: ['32.776664', '28.538335', '35.047157', '' ], - marker: { size: ['5', '49', '5', ''] } - })); - - expect(opts.circle.paint['circle-radius'].stops) - .toBeCloseTo2DArray([[0, 2.5], [1, 24.5]], 'no replicate stops'); - - var radii = opts.circle.geojson.features.map(function(f) { - return f.properties['circle-radius']; - }); - - expect(radii).toBeCloseToArray([0, 1, 0], 'link features to correct stops'); - }); - it('should generate correct output for traces with only blank points', function() { var opts = _convert(Lib.extendFlat({}, base, { mode: 'lines', @@ -451,7 +294,9 @@ describe('scattermapbox convert', function() { fill: 'toself' })); - assertVisibility(opts, ['none', 'none', 'none', 'none']); + // not optimal, but doesn't break anything as mapbox-gl accepts empty + // coordinate arrays + assertVisibility(opts, ['visible', 'visible', 'none', 'none']); expect(opts.line.geojson.coordinates).toEqual([], 'line coords'); expect(opts.fill.geojson.coordinates).toEqual([], 'fill coords'); From 7dabdd03c00b2956f6074e0b7fe4a3c760fd4731 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=89tienne=20T=C3=A9treault-Pinard?= Date: Fri, 31 Mar 2017 14:40:30 -0400 Subject: [PATCH 6/6] :hocho: last remaining signs of `placeholder` key in calcdata - no need for it anymore. --- src/plots/plots.js | 5 +---- src/traces/scattermapbox/hover.js | 2 -- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/src/plots/plots.js b/src/plots/plots.js index 6f4dfcf913e..dede12865f4 100644 --- a/src/plots/plots.js +++ b/src/plots/plots.js @@ -2012,11 +2012,8 @@ plots.doCalcdata = function(gd, traces) { // // This ensures there is a calcdata item for every trace, // even if cartesian logic doesn't handle it (for things like legends). - // - // Tag this artificial calc point with 'placeholder: true', - // to make it easier to skip over them in during the plot and hover step. if(!Array.isArray(cd) || !cd[0]) { - cd = [{x: false, y: false, placeholder: true}]; + cd = [{x: false, y: false}]; } // add the trace-wide properties to the first point, diff --git a/src/traces/scattermapbox/hover.js b/src/traces/scattermapbox/hover.js index 8c14f03bcb0..1e49b13bc5e 100644 --- a/src/traces/scattermapbox/hover.js +++ b/src/traces/scattermapbox/hover.js @@ -20,8 +20,6 @@ module.exports = function hoverPoints(pointData, xval, yval) { xa = pointData.xa, ya = pointData.ya; - if(cd[0].placeholder) return; - // compute winding number about [-180, 180] globe var winding = (xval >= 0) ? Math.floor((xval + 180) / 360) :