Skip to content

Scattergeo BADNUM #1538

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 14 commits into from
Apr 7, 2017
Merged

Scattergeo BADNUM #1538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
bfe0211
make scattergeo calcdata 1-1 with lon/lat items in fullData
etpinard Mar 30, 2017
18091e8
rm `placeholder` early exit in scattergeo hover routine
etpinard Mar 30, 2017
a705ed1
update geojson util calcTraceToLineCoords to use BADNUM
etpinard Mar 30, 2017
64cbf2f
flatten scattegeo_test.js
etpinard Mar 31, 2017
f84627d
set lonlat to [BADNUM, BADNUM] if lon OR lat is non-numeric
etpinard Mar 31, 2017
e85fba5
only need to check lonlat[0] === BADNUM
etpinard Mar 31, 2017
6a7e2a4
Merge branch 'master' into scattergeo-badnum
etpinard Apr 7, 2017
aec6248
make sure mapbox-gl canvas does not inherit left and top css
etpinard Apr 7, 2017
1abbd90
:hocho: scattermapbox/calc.js | :recycle: scattergeo/calc.js
etpinard Apr 7, 2017
3e64aa0
update scattermapbox convert & hover
etpinard Apr 7, 2017
785bade
update mapbox baselines
etpinard Apr 7, 2017
5c2393a
:hocho: last remaining signs of `placeholder` key in calcdata
etpinard Mar 31, 2017
b5cd573
replace false values with BADNUM in empty calcdata traces
etpinard Apr 7, 2017
248b760
Merge pull request #1564 from plotly/scattermapbox-badnum2
etpinard Apr 7, 2017
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
17 changes: 10 additions & 7 deletions src/lib/geojson_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@

'use strict';

var BADNUM = require('../constants/numerical').BADNUM;

/**
* Convert calcTrace to GeoJSON 'MultiLineString' coordinate arrays
*
Expand All @@ -21,18 +23,19 @@
*
*/
exports.calcTraceToLineCoords = function(calcTrace) {
var trace = calcTrace[0].trace,
connectgaps = trace.connectgaps;
var trace = calcTrace[0].trace;
var connectgaps = trace.connectgaps;

var coords = [],
lineString = [];
var coords = [];
var lineString = [];

for(var i = 0; i < calcTrace.length; i++) {
var calcPt = calcTrace[i];
var lonlat = calcPt.lonlat;

lineString.push(calcPt.lonlat);

if(!connectgaps && calcPt.gapAfter && lineString.length > 0) {
if(lonlat[0] !== BADNUM) {
lineString.push(lonlat);
} else if(!connectgaps && lineString.length > 0) {
coords.push(lineString);
lineString = [];
}
Expand Down
9 changes: 6 additions & 3 deletions src/plots/mapbox/mapbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,10 +111,14 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) {
});

// clear navigation container
var className = constants.controlContainerClassName,
controlContainer = self.div.getElementsByClassName(className)[0];
var className = constants.controlContainerClassName;
var controlContainer = self.div.getElementsByClassName(className)[0];
self.div.removeChild(controlContainer);

// make sure canvas does not inherit left and top css
map._canvas.canvas.style.left = '0px';
map._canvas.canvas.style.top = '0px';

self.rejectOnError(reject);

map.once('load', function() {
Expand Down Expand Up @@ -176,7 +180,6 @@ proto.createMap = function(calcData, fullLayout, resolve, reject) {

map.on('dragstart', unhover);
map.on('zoomstart', unhover);

};

proto.updateMap = function(calcData, fullLayout, resolve, reject) {
Expand Down
6 changes: 2 additions & 4 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ var Plotly = require('../plotly');
var Registry = require('../registry');
var Lib = require('../lib');
var Color = require('../components/color');
var BADNUM = require('../constants/numerical').BADNUM;

var plots = module.exports = {};

Expand Down Expand Up @@ -2012,11 +2013,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: BADNUM, y: BADNUM}];
}

// add the trace-wide properties to the first point,
Expand Down
39 changes: 13 additions & 26 deletions src/traces/scattergeo/calc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,45 +10,32 @@
'use strict';

var isNumeric = require('fast-isnumeric');
var BADNUM = require('../../constants/numerical').BADNUM;

var calcMarkerColorscale = require('../scatter/colorscale_calc');

var arraysToCalcdata = require('../scatter/arrays_to_calcdata');

module.exports = function calc(gd, trace) {
var hasLocationData = Array.isArray(trace.locations),
len = hasLocationData ? trace.locations.length : trace.lon.length;

var calcTrace = [],
cnt = 0;
var hasLocationData = Array.isArray(trace.locations);
var len = hasLocationData ? trace.locations.length : trace.lon.length;
var calcTrace = new Array(len);

for(var i = 0; i < len; i++) {
var calcPt = {},
skip;
var calcPt = calcTrace[i] = {};

if(hasLocationData) {
var loc = trace.locations[i];
calcPt.loc = typeof loc === 'string' ? loc : null;
} else {
var lon = trace.lon[i];
var lat = trace.lat[i];

calcPt.loc = loc;
skip = (typeof loc !== 'string');
}
else {
var lon = trace.lon[i],
lat = trace.lat[i];

calcPt.lonlat = [+lon, +lat];
skip = (!isNumeric(lon) || !isNumeric(lat));
if(isNumeric(lon) && isNumeric(lat)) calcPt.lonlat = [+lon, +lat];
else calcPt.lonlat = [BADNUM, BADNUM];
}

if(skip) {
if(cnt > 0) calcTrace[cnt - 1].gapAfter = true;
continue;
}

cnt++;

calcTrace.push(calcPt);
}

arraysToCalcdata(calcTrace, trace);
calcMarkerColorscale(trace);

return calcTrace;
Expand Down
6 changes: 2 additions & 4 deletions src/traces/scattergeo/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

var Fx = require('../../plots/cartesian/graph_interact');
var Axes = require('../../plots/cartesian/axes');
var BADNUM = require('../../constants/numerical').BADNUM;

var getTraceColor = require('../scatter/get_trace_color');
var attributes = require('./attributes');
Expand All @@ -23,17 +24,14 @@ module.exports = function hoverPoints(pointData) {
ya = pointData.ya,
geo = pointData.subplot;

if(cd[0].placeholder) return;

function c2p(lonlat) {
return geo.projection(lonlat);
}

function distFn(d) {
var lonlat = d.lonlat;

// this handles the not-found location feature case
if(lonlat[0] === null || lonlat[1] === null) return Infinity;
if(lonlat[0] === BADNUM) return Infinity;

if(geo.isLonLatOverEdges(lonlat)) return Infinity;

Expand Down
97 changes: 32 additions & 65 deletions src/traces/scattergeo/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,27 @@ var Drawing = require('../../components/drawing');
var Color = require('../../components/color');

var Lib = require('../../lib');
var BADNUM = require('../../constants/numerical').BADNUM;
var getTopojsonFeatures = require('../../lib/topojson_utils').getTopojsonFeatures;
var locationToFeature = require('../../lib/geo_location_utils').locationToFeature;
var geoJsonUtils = require('../../lib/geojson_utils');
var arrayToCalcItem = require('../../lib/array_to_calc_item');
var subTypes = require('../scatter/subtypes');


module.exports = function plot(geo, calcData) {

function keyFunc(d) { return d[0].trace.uid; }

function removeBADNUM(d, node) {
if(d.lonlat[0] === BADNUM) {
d3.select(node).remove();
}
}

for(var i = 0; i < calcData.length; i++) {
fillLocationLonLat(calcData[i], geo.topojson);
}

var gScatterGeoTraces = geo.framework.select('.scattergeolayer')
.selectAll('g.trace.scattergeo')
.data(calcData, keyFunc);
Expand All @@ -39,27 +49,11 @@ module.exports = function plot(geo, calcData) {
gScatterGeoTraces.selectAll('*').remove();

gScatterGeoTraces.each(function(calcTrace) {
var s = d3.select(this),
trace = calcTrace[0].trace,
convertToLonLatFn = makeConvertToLonLatFn(trace, geo.topojson);

// skip over placeholder traces
if(calcTrace[0].placeholder) s.remove();

// just like calcTrace but w/o not-found location datum
var _calcTrace = [];

for(var i = 0; i < calcTrace.length; i++) {
var _calcPt = convertToLonLatFn(calcTrace[i]);

if(_calcPt) {
arrayItemToCalcdata(trace, calcTrace[i], i);
_calcTrace.push(_calcPt);
}
}
var s = d3.select(this);
var trace = calcTrace[0].trace;

if(subTypes.hasLines(trace) || trace.fill !== 'none') {
var lineCoords = geoJsonUtils.calcTraceToLineCoords(_calcTrace);
var lineCoords = geoJsonUtils.calcTraceToLineCoords(calcTrace);

var lineData = (trace.fill !== 'none') ?
geoJsonUtils.makePolygon(lineCoords, trace) :
Expand All @@ -72,66 +66,39 @@ module.exports = function plot(geo, calcData) {
}

if(subTypes.hasMarkers(trace)) {
s.selectAll('path.point').data(_calcTrace)
.enter().append('path')
.classed('point', true);
s.selectAll('path.point')
.data(Lib.identity)
.enter().append('path')
.classed('point', true)
.each(function(calcPt) { removeBADNUM(calcPt, this); });
Copy link
Collaborator

Choose a reason for hiding this comment

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

For the record, I just had a discussion with @etpinard about this where my gut reaction was that we should do

s.selectAll('path.point')
    .data(filterOutBadPoints)
  .enter().append('path')
    ...

instead of creating all the nodes and then deleting the bad ones.

The conclusion we came to though is that the way it is here is actually better, despite being a bit less "d3-idiomatic." The rationale is that bad points should be considered an edge case, so we don't worry about the overhead of creating and deleting their nodes (which will happen on every replot), just about how the .each call to test (good) points for removal compares to whatever filterOutBadPoints does. And since the latter would always need to duplicate the calcdata array, it's almost certainly slower than this .each.

}

if(subTypes.hasText(trace)) {
s.selectAll('g').data(_calcTrace)
s.selectAll('g')
.data(Lib.identity)
.enter().append('g')
.append('text');
.append('text')
.each(function(calcPt) { removeBADNUM(calcPt, this); });
}
});

// call style here within topojson request callback
style(geo);
};

function makeConvertToLonLatFn(trace, topojson) {
if(!Array.isArray(trace.locations)) return Lib.identity;

var features = getTopojsonFeatures(trace, topojson),
locationmode = trace.locationmode;
function fillLocationLonLat(calcTrace, topojson) {
var trace = calcTrace[0].trace;

return function(calcPt) {
var feature = locationToFeature(locationmode, calcPt.loc, features);
if(!Array.isArray(trace.locations)) return;

if(feature) {
calcPt.lonlat = feature.properties.ct;
return calcPt;
}
else {
// mutate gd.calcdata so that hoverPoints knows to skip this datum
calcPt.lonlat = [null, null];
return false;
}
};
}
var features = getTopojsonFeatures(trace, topojson);
var locationmode = trace.locationmode;

function arrayItemToCalcdata(trace, calcItem, i) {
var marker = trace.marker;

function merge(traceAttr, calcAttr) {
arrayToCalcItem(traceAttr, calcItem, calcAttr, i);
}

merge(trace.text, 'tx');
merge(trace.textposition, 'tp');
if(trace.textfont) {
merge(trace.textfont.size, 'ts');
merge(trace.textfont.color, 'tc');
merge(trace.textfont.family, 'tf');
}
for(var i = 0; i < calcTrace.length; i++) {
var calcPt = calcTrace[i];
var feature = locationToFeature(locationmode, calcPt.loc, features);

if(marker && marker.line) {
var markerLine = marker.line;
merge(marker.opacity, 'mo');
merge(marker.symbol, 'mx');
merge(marker.color, 'mc');
merge(marker.size, 'ms');
merge(markerLine.color, 'mlc');
merge(markerLine.width, 'mlw');
calcPt.lonlat = feature ? feature.properties.ct : [BADNUM, BADNUM];
}
}

Expand Down
Loading