8000 Scattergeo BADNUM by etpinard · Pull Request #1538 · plotly/plotly.js · GitHub
[go: up one dir, main page]
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 1 commit
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
Next Next commit
make scattergeo calcdata 1-1 with lon/lat items in fullData 
- use BADNUM to normalize non-numeric lon/lat handling
- and instead remove BADNUM pts in plot step
- this is consistent with all other d3 plot type (!!)
- allows us to reuse scatter/arrays_to_calcdata
- and cleans up locations -> lonlat logic!
  • Loading branch information
@etpinard
etpinard committed Mar 30, 2017
commit bfe02119f53bdbadde92ec7060bf2510ac415e68
37 changes: 13 additions & 24 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,34 @@
'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 = loc;
skip = (typeof loc !== 'string');
calcPt.loc = typeof loc === 'string' ? loc : null;
}
else {
var lon = trace.lon[i],
lat = trace.lat[i];
var lon = trace.lon[i];
var lat = trace.lat[i];
calcPt.lonlat = new Array(2);

calcPt.lonlat = [+lon, +lat];
skip = (!isNumeric(lon) || !isNumeric(lat));
calcPt.lonlat[0] = isNumeric(lon) ? +lon : BADNUM;
calcPt.lonlat[1] = isNumeric(lat) ? +lat : BADNUM;
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably all we'd need to do for https://github.com/plotly/plotly.js/pull/1538/files#r109043439 to work is change this to something like

if(isNumeric(lon) && isNumeric(lat)) calcPt.lonlat = [+lon, +lat];
else calcPt.lonlat = [BADNUM, BADNUM]; // or even [BADNUM] or []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good! Thanks for taking a 👁️

}

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

cnt++;

calcTrace.push(calcPt);
}

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

return calcTrace;
Expand Down
4 changes: 2 additions & 2 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 @@ -32,8 +33,7 @@ module.exports = function hoverPoints(pointData) {
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 || lonlat[1] === BADNUM) return Infinity;

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

Expand Down
98 changes: 33 additions & 65 deletions src/traces/scattergeo/plot.js
View file
Open in desktop
6D40
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,28 @@ 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) {
var lonlat = d.lonlat;
if(lonlat[0] === BADNUM || lonlat[1] === 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 +50,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 +67,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
0