Multiple scattergeo improvement #1004

etpinard · 2016-10-04T21:48:22Z

fixes #963

and supersedes #964

In brief, this PR brings several ideas from scattermapbox over to scattergeo - while reusing some parts of logic (namely regarding GeoJSON) keeping things 🌴

In details, this PR:

fixes Strange behavior with missing values in scattergeo #963 (thanks again @cpsievert for letting me know)
scattergeo traces with non-circle markers now get the correct legend item
make scattergeo use a proper calc step
make scattergeo use Fx.hover to pick (using calcdata) and draw its hover labels
- this means that user no longer have to hover over scattergeo data points to trigger hover event / draw labels
- this means that scattergeo mode: 'lines' now (I mean finally) has a hover handlers!
adds support for connecgaps to scattergeo
adds support for fill: 'toself' to scattergeo (similar to how scattermapbox implement fill)

🎉 Yay geo line hover text:

- which is consistent with mapbox subplot, and comptabitible with Fx.hover

- to be reuse in 'scattergeo'

- which skips and cast lon/lat values - fixes #963 - note that 'locations[i]' -> feature must still be done at the plot (after the topojson is loaded)

- traces with no data points are given a place-holder calc trace to retatin fullData.length === gd.calcdata.length, and make some component (e.g. legend) logic easier - tag these place-holder traces so that the plot modules can easily skip over them

- so plotting that data-pt less traces do result in mapbox errors.

- add support for 'connectgaps' - add support for 'fill: 'toself' - remove all traces of hover / click handlers

- make Fx.hover pass subplot info to hoverPoints module - add scattergeo hoverPoints and eventData modules - mock xaxis and yaxis in geo instances - factor out is-over-edge logic to use it in hoverPoints - use 'mousemove' instead of 'mouseover' in test to trigger hover

- until we make choropleth use fullLayout._hoverlayer for its hover labels.

- the first legend item is now correct!

bpostlethwaite

Looks ok to me but I didn't get right into the details for all these changes. If I didn't understand what the code was doing fairly quickly I made a comment about improving the naming or comments.

bpostlethwaite · 2016-10-05T19:46:58Z

src/traces/scattergeo/hover.js

+        ya = pointData.ya,
+        geo = pointData.subplot;
+
+    if(cd[0].placeholder) return;


this is a little mysterious, at least as someone who doesn't look at Plotly.js code daily. Maybe a comment here?

Here's the comment you're looking for: https://github.com/plotly/plotly.js/pull/1004/files#diff-ad4f76ccd6044ed16514297078e13b84R1672

Putting comment about something where it is set is probably more robust than putting it where it is used.

bpostlethwaite · 2016-10-05T19:48:07Z

src/traces/scattergeo/hover.js

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


why return Infinity and not say, null

because this function returns a distance.

I would have expected NaN I think.

bpostlethwaite · 2016-10-05T19:49:34Z

src/traces/scattergeo/hover.js

+    var di = cd[pointData.index],
+        lonlat = di.lonlat,
+        pos = c2p(lonlat),
+        rad = di.mrc || 1;


is di.mrc undefined or 0? Or could be both and we want it to be 1 regardless. Also why 1? Maybe some comments here

di.mrc is set only when there mode includes 'markers', so we need a fallback for mode 'line' and 'text'

I wish I could gather that from that line of code ;)

bpostlethwaite · 2016-10-05T19:57:08Z

src/traces/scattergeo/hover.js

+
+        var dx = Math.abs(xPx - pos[0]),
+            dy = Math.abs(yPx - pos[1]),
+            rad = Math.max(3, d.mrc || 0);


what are the constraints on radius here for?

marker pt radii are taken into consideration in the picking routine.

Do you think it will be obvious to someone who is maintaining this code what the 3 and 0 are doing? I didn't get it right away which is usually a sign that a comment may be helpful

but perhaps it is obvious ;/

bpostlethwaite · 2016-10-05T19:57:59Z

src/traces/scattergeo/plot.js


-        if(!lonlat) continue;  // filter the blank points here
+        // skip over placeholder traces
+        if(calcTrace[0].placeholder) s.remove();


what are placeholder traces?

bpostlethwaite · 2016-10-05T20:01:09Z

src/traces/scattermapbox/hover.js

@@ -19,6 +19,8 @@ module.exports = function hoverPoints(pointData, xval, yval) {
        xa = pointData.xa,
        ya = pointData.ya;

+    if(cd[0].placeholder) return;


is there another name we can use beside placeholder, a placeholder for _____? How about emptytrace?.

we could.

Not sure emptytrace is better though. traceWithNoVisibleDataPoint would be the most verbose. But I think placeholder does the trick as discussed in https://github.com/plotly/plotly.js/pull/1004/files#diff-ad4f76ccd6044ed16514297078e13b84R1672

yep placeholder is ok. Perhaps we can expand a little in that original placeholder comment. I still didn't quite understand when they would be necessary

bpostlethwaite · 2016-10-05T21:02:13Z

Aighty all the comments are nonblocking! There are nice tests and this PR looks like a huge win

Nice work!

bpostlethwaite · 2016-10-05T21:02:18Z

💃

etpinard added 13 commits October 4, 2016 17:05

geo: make geo trace module use calcData instead of fullData

b0c8448

geo: rename fullLayout.geo._geo ref -> _subplot

d0e1104

- which is consistent with mapbox subplot, and comptabitible with Fx.hover

util: move GeoJSON coords / type logic in own module

4448eaa

- to be reuse in 'scattergeo'

scattergeo: add proper calc step

52c1b89

- which skips and cast lon/lat values - fixes #963 - note that 'locations[i]' -> feature must still be done at the plot (after the topojson is loaded)

scattermapbox: implement placeholder logic

3d0aeff

- so plotting that data-pt less traces do result in mapbox errors.

fx: add eventData hook to customize event data module-by-module

63ee7f7

scattermapbox: use eventData hook to add lon[i], lat[i] to event data

78d9afa

scattergeo: refactor plot !!

3a0c621

- add support for 'connectgaps' - add support for 'fill: 'toself' - remove all traces of hover / click handlers

choropleth: add dummy hover handler to skip Fx.hover w/o warnings

7e1c66e

- until we make choropleth use fullLayout._hoverlayer for its hover labels.

test: fix geo_legendonly baseline

8d40b38

- the first legend item is now correct!

test: add mocks for 'scattergeo' connectgaps and fill

7498fe6

etpinard added bug something broken feature something new status: reviewable labels Oct 4, 2016

etpinard added this to the v1.18.0 milestone Oct 4, 2016

bpostlethwaite reviewed Oct 5, 2016

View reviewed changes

bpostlethwaite approved these changes Oct 5, 2016

View reviewed changes

doc: expand comment about placeholder calc point

510cb60

etpinard merged commit f058e8b into master Oct 6, 2016

etpinard deleted the geo-line-picking branch October 6, 2016 01:12

etpinard mentioned this pull request Mar 29, 2017

scattergeo does not respect datum position in arrayOk attributes #1529

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multiple scattergeo improvement #1004

Multiple scattergeo improvement #1004

etpinard commented Oct 4, 2016 •

edited

Loading

bpostlethwaite left a comment

bpostlethwaite Oct 5, 2016

etpinard Oct 5, 2016

bpostlethwaite Oct 5, 2016

etpinard Oct 5, 2016 •

edited

Loading

bpostlethwaite Oct 5, 2016

bpostlethwaite Oct 5, 2016

etpinard Oct 5, 2016

bpostlethwaite Oct 5, 2016

bpostlethwaite Oct 5, 2016

etpinard Oct 5, 2016

bpostlethwaite Oct 5, 2016

bpostlethwaite Oct 5, 2016

bpostlethwaite Oct 5, 2016

bpostlethwaite Oct 5, 2016

etpinard Oct 5, 2016 •

edited

Loading

bpostlethwaite Oct 5, 2016

bpostlethwaite commented Oct 5, 2016

bpostlethwaite commented Oct 5, 2016

Multiple scattergeo improvement #1004

Multiple scattergeo improvement #1004

Conversation

etpinard commented Oct 4, 2016 • edited Loading

bpostlethwaite left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etpinard Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bpostlethwaite commented Oct 5, 2016

bpostlethwaite commented Oct 5, 2016

etpinard commented Oct 4, 2016 •

edited

Loading

etpinard Oct 5, 2016 •

edited

Loading

etpinard Oct 5, 2016 •

edited

Loading