Scattergeo BADNUM #1538
Scattergeo BADNUM #1538
Conversation
- 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!
- no need for that anymore scattergeo calcdata for visible trace will now always items in it.
- to make it work post scattergeo calcdata 1-1 with lon/lat items - N.B. this break scattermapbox line trace with gaps (to be fix on other branch)
src/lib/geojson_utils.js
Outdated
| lineString.push(calcPt.lonlat); | ||
|
|
||
| if(!connectgaps && calcPt.gapAfter && lineString.length > 0) { | ||
| if(lonlat[0] !== BADNUM && lonlat[1] !== BADNUM) { |
There was a problem hiding this comment.
This is great for robustness (so this comment is nonblocking), but for 🐎 (with sufficient test coverage) we could ensure in calc that if the point is invalid, BOTH components get set to BADNUM, so you only need one test.
src/traces/scattergeo/calc.js
Outdated
| calcPt.lonlat = [+lon, +lat]; | ||
| skip = (!isNumeric(lon) || !isNumeric(lat)); | ||
| calcPt.lonlat[0] = isNumeric(lon) ? +lon : BADNUM; | ||
| calcPt.lonlat[1] = isNumeric(lat) ? +lat : BADNUM; |
There was a problem hiding this comment.
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 []There was a problem hiding this comment.
Sounds good! Thanks for taking a 👁️
|
I guess if the goal here is "get this right once and for all" then we probably should reduce the plot-level filters to a single After we have that, this looks a great pattern to standardize on. 🎉 |
- to improve performance in check downstream - add calc tests to scattergeo_test.js
- to determine if calcdata item should be skipped
| .data(Lib.identity) | ||
| .enter().append('path') | ||
| .classed('point', true) | ||
| .each(function(calcPt) { removeBADNUM(calcPt, this); }); |
There was a problem hiding this comment.
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.
|
Looks great to me (once tests pass, of course) 💃 |
Will passed once #1543 is merged. |
- e.g. in rreusser's plotly-mock-viewer
- so that it handles BADNUM items - does colorscale / size fn item conversions - these were handled in scattermapbox/calc.js previously
- mapbox does this annoying thing were their tiles change on their server independently of the mapbox-gl version used.
- no need for it anymore.
Scattermapbox BADNUM (w/o upgrading mapbox-gl)
fixes #1529
This PR makes the
scattergeocalcdata have as many items as there arelon/latcoordinates orlocationsitems in the full data. In other words, it 🔪 the overly-complicated logic that skipped non-numeric items incalc.js. Now, non-numeric coordinates are set toBADNUMincalc.jsand filtered out inplot.js. Turns out this cleans up a good amount of code and makes it possible forscattergeoto reusescatter/arrays_to_calcdata.jsstraight up 🎉Now, commit a705ed1 breaks
scattermapboxas it too dependscalcTraceToLineCoords. I'll fix this on another branch based off this one.In the meantime (i.e. before adding mucho tests) I would appreciate some 👍 s from @alexcjohnson to get this right once and for all.