Skip to content

Upgrade d3 geo projections #5112

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 19 commits into from
Jun 30, 2021
Merged

Upgrade d3 geo projections #5112

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
9f679fd
apply latest d3-geo v1 and d3-geo-projection v2
archmoj Jul 29, 2020
f17f3bf
update two baselines
archmoj Jul 30, 2020
a7735e6
remove tests related to unhandled bugs in the previous d3-geo functions
archmoj Sep 1, 2020
7730d43
link to d3 v3 without geo
archmoj Jan 14, 2021
8dd4ba0
add comment about the potential projections to add
archmoj Jan 14, 2021
7e36f6c
Merge remote-tracking branch 'origin' into new-d3-geo
archmoj Jan 14, 2021
557ebde
remove unused geo/projections.js file
archmoj Jan 14, 2021
1b4fd15
Merge remote-tracking branch 'origin/master' into new-d3-geo
archmoj Jan 20, 2021
f46e4af
Revert "remove tests related to unhandled bugs in the previous d3-geo…
archmoj Jan 21, 2021
d5d352c
no longer need to add fitExtend from d3 v4
archmoj Jan 21, 2021
b9be8c1
Merge remote-tracking branch 'origin/master' into new-d3-geo
archmoj May 22, 2021
d5883d2
no longer need to check for NaN with d3-geo
archmoj May 22, 2021
647b0dc
adjust geo test - previous test used to fail locally on master too
archmoj May 22, 2021
a046871
clear fallback to default view - no longer NaNs in d3-geo
archmoj May 22, 2021
98c19b8
fixup geo requires
archmoj May 25, 2021
905eeb0
Merge remote-tracking branch 'origin/master' into new-d3-geo
archmoj Jun 26, 2021
0a7ed9f
regenerate two baselines using kaleido
archmoj Jun 26, 2021
be527cf
use plotly/d3 v3.7.0 which no longer has geo part
archmoj Jun 30, 2021
32aeac6
Merge remote-tracking branch 'origin/master' into new-d3-geo
archmoj Jun 30, 2021
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
24 changes: 21 additions & 3 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@
]
},
"dependencies": {
"@plotly/d3": "^3.5.18",
"@plotly/d3": "git://github.com/plotly/d3.git#e792de9ae52d956d18582cac8a49ea6a03c7e010",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

plotly/d3@master...plotly:drop-geo

"@plotly/d3-sankey": "0.7.2",
"@plotly/d3-sankey-circular": "0.33.1",
"@plotly/point-cluster": "^3.1.9",
Expand All @@ -73,6 +73,8 @@
"convex-hull": "^1.0.3",
"country-regex": "^1.1.0",
"d3-force": "^1.2.1",
"d3-geo": "^1.12.1",
"d3-geo-projection": "^2.9.0",
"d3-hierarchy": "^1.1.9",
"d3-interpolate": "^1.4.0",
"d3-time-format": "^2.2.3",
Expand Down
103 changes: 101 additions & 2 deletions src/plots/geo/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

// projection names to d3 function name
exports.projNames = {
// d3.geo.projection
'equirectangular': 'equirectangular',
'mercator': 'mercator',
'orthographic': 'orthographic',
Expand All @@ -32,7 +31,107 @@ exports.projNames = {
'albers usa': 'albersUsa',
'winkel tripel': 'winkel3',
'aitoff': 'aitoff',
'sinusoidal': 'sinusoidal'
'sinusoidal': 'sinusoidal',
/*
// potential projections that could be added to the API

'airy': 'airy',
// 'albers': 'albers',
'armadillo': 'armadillo',
'august': 'august',
'baker': 'baker',
'berghaus': 'berghaus',
'bertin1953': 'bertin1953',
'boggs': 'boggs',
'bonne': 'bonne',
'bottomley': 'bottomley',
'bromley': 'bromley',
// 'chamberlin': 'chamberlin',
'chamberlin africa': 'chamberlinAfrica',
'collignon': 'collignon',
'craig': 'craig',
'craster': 'craster',
'cylindrical equal area': 'cylindricalEqualArea',
'cylindrical stereographic': 'cylindricalStereographic',
'eckert1': 'eckert1',
'eckert2': 'eckert2',
'eckert3': 'eckert3',
'eckert5': 'eckert5',
'eckert6': 'eckert6',
'eisenlohr': 'eisenlohr',
'fahey': 'fahey',
'foucaut': 'foucaut',
'foucaut sinusoidal': 'foucautSinusoidal',
'gilbert': 'gilbert',
'gingery': 'gingery',
'ginzburg4': 'ginzburg4',
'ginzburg5': 'ginzburg5',
'ginzburg6': 'ginzburg6',
'ginzburg8': 'ginzburg8',
'ginzburg9': 'ginzburg9',
'gringorten': 'gringorten',
'guyou': 'guyou',
'hammer retroazimuthal': 'hammerRetroazimuthal',
'healpix': 'healpix',
'hill': 'hill',
'homolosine': 'homolosine',
'hufnagel': 'hufnagel',
'hyperelliptical': 'hyperelliptical',
'lagrange': 'lagrange',
'larrivee': 'larrivee',
'laskowski': 'laskowski',
'littrow': 'littrow',
'loximuthal': 'loximuthal',
// 'modified stereographic': 'modifiedStereographic',
'modified stereographic alaska': 'modifiedStereographicAlaska',
'modified stereographic gs48': 'modifiedStereographicGs48',
'modified stereographic gs50': 'modifiedStereographicGs50',
'modified stereographic miller': 'modifiedStereographicMiller',
'modified stereographic lee': 'modifiedStereographicLee',
'mt flat polar parabolic': 'mtFlatPolarParabolic',
'mt flat polar quartic': 'mtFlatPolarQuartic',
'mt flat polar sinusoidal': 'mtFlatPolarSinusoidal',
'natural earth1': 'naturalEarth1',
'natural earth2': 'naturalEarth2',
'nell hammer': 'nellHammer',
'nicolosi': 'nicolosi',
'patterson': 'patterson',
'polyconic': 'polyconic',
'rectangular polyconic': 'rectangularPolyconic',
'satellite': 'satellite',
'sinu mollweide': 'sinuMollweide',
'times': 'times',
// 'two point azimuthal': 'twoPointAzimuthal',
// 'two point azimuthalUsa': 'twoPointAzimuthalUsa',
// 'two point equidistant': 'twoPointEquidistant',
// 'two point equidistantUsa': 'twoPointEquidistantUsa',
'van der grinten': 'vanDerGrinten',
'van der grinten2': 'vanDerGrinten2',
'van der grinten3': 'vanDerGrinten3',
'van der grinten4': 'vanDerGrinten4',
// 'wagner': 'wagner',
'wagner4': 'wagner4',
'wagner6': 'wagner6',
// 'wagner7': 'wagner7',
'wiechel': 'wiechel',
'winkel3': 'winkel3',

// 'interrupt': 'interrupt',
'interrupted homolosine': 'interruptedHomolosine',
'interrupted sinusoidal': 'interruptedSinusoidal',
'interrupted boggs': 'interruptedBoggs',
'interrupted sinu mollweide': 'interruptedSinuMollweide',
'interrupted mollweide': 'interruptedMollweide',
'interrupted mollweide hemispheres': 'interruptedMollweideHemispheres',
'interrupted quartic authalic': 'interruptedQuarticAuthalic',

'polyhedral butterfly': 'polyhedralButterfly',
'polyhedral collignon': 'polyhedralCollignon',
'polyhedral waterman': 'polyhedralWaterman',

'gringorten quincuncial': 'gringortenQuincuncial',
'peirce quincuncial': 'peirceQuincuncial',
*/
};

// name of the axes
Expand Down
18 changes: 12 additions & 6 deletions src/plots/geo/geo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@
/* global PlotlyGeoAssets:false */

var d3 = require('@plotly/d3');
var geo = require('d3-geo');
var geoPath = require('d3-geo').geoPath;
var geoDistance = require('d3-geo').geoDistance;
var geoProjection = require('d3-geo-projection');

var Registry = require('../../registry');
var Lib = require('../../lib');
Expand All @@ -33,8 +37,6 @@ var geoUtils = require('../../lib/geo_location_utils');
var topojsonUtils = require('../../lib/topojson_utils');
var topojsonFeature = require('topojson-client').feature;

require('./projections')(d3);

function Geo(opts) {
this.id = opts.id;
this.graphDiv = opts.graphDiv;
Expand Down Expand Up @@ -656,7 +658,7 @@ proto.render = function() {
}
};

// Helper that wraps d3.geo[/* projection name /*]() which:
// Helper that wraps d3[geo + /* Projection name /*]() which:
//
// - adds 'fitExtent' (available in d3 v4)
// - adds 'getPath', 'getBounds' convenience methods
Expand All @@ -671,7 +673,11 @@ function getProjection(geoLayout) {
var projLayout = geoLayout.projection;
var projType = projLayout.type;

var projection = d3.geo[constants.projNames[projType]]();
var projName = constants.projNames[projType];
// uppercase the first letter and add geo to the start of method name
projName = 'geo' + projName.charAt(0).toUpperCase() + projName.slice(1);
var projFn = geo[projName] || geoProjection[projName];
var projection = projFn();

var clipAngle = geoLayout._isClipped ?
constants.lonaxisSpan[projType] / 2 :
Expand All @@ -694,7 +700,7 @@ function getProjection(geoLayout) {

if(clipAngle) {
var r = projection.rotate();
var angle = d3.geo.distance(lonlat, [-r[0], -r[1]]);
var angle = geoDistance(lonlat, [-r[0], -r[1]]);
var maxAngle = clipAngle * Math.PI / 180;
return angle > maxAngle;
} else {
Expand All @@ -703,7 +709,7 @@ function getProjection(geoLayout) {
};

projection.getPath = function() {
return d3.geo.path().projection(projection);
return geoPath().projection(projection);
};

projection.getBounds = function(object) {
Expand Down
Binary file modified test/image/baselines/geo_fitbounds-locations.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified test/image/baselines/geo_stereographic.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
100 changes: 0 additions & 100 deletions test/jasmine/tests/geo_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1353,46 +1353,6 @@ describe('Test geo interactions', function() {
.then(done, done.fail);
});

it('should clear hover label when cursor slips off subplot', function(done) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why did these 2 tests get removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I recall (as mentioned in the commit message) there used to be a bug in d3.geo where a function returned NaN positions. And we guarded against it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But these tests are just using the high-level API, not d3 geo itself. If they started failing, that means something changed about the user-facing behavior. What changed? Are we comfortable with that change? I'd much prefer if we can adapt the tests to the new version so we can see the difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. Pointing to the fact that rebase somehow forgot to remove geo/projections.
It is now fixed in 557ebde.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This test should still be able to pass though. Perhaps the exact pixel positions need to change because there's some small change in how the projection is applied? If I try to manually follow what's going on in the test (I believe it's hovering on the point off the coast of Africa, then slowly mousing off the edge of the globe) the behavior looks identical before/after the change. So we should be able to bring the test back, but if the exact values need to change a little (px = 390, py = 290 and if(px < 402) that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Adjusted in 647b0dc.
In fact with previous positions the tests didn't pass locally. But now the test now passes both locally and on the CI.

var gd = createGraphDiv();
var fig = Lib.extendDeep({}, require('@mocks/geo_orthographic.json'));

function _assert(msg, hoverLabelCnt) {
expect(d3SelectAll('g.hovertext').size())
.toBe(hoverLabelCnt, msg);
}

var px = 390;
var py = 290;
var cnt = 0;

Plotly.newPlot(gd, fig).then(function() {
gd.on('plotly_unhover', function() { cnt++; });

mouseEvent('mousemove', px, py);
_assert('base state', 1);

return new Promise(function(resolve) {
var interval = setInterval(function() {
px += 2;
mouseEvent('mousemove', px, py);

if(px < 402) {
_assert('- px ' + px, 1);
expect(cnt).toBe(0, 'no plotly_unhover event so far');
} else {
_assert('- px ' + px, 0);
expect(cnt).toBe(1, 'plotly_unhover event count');

clearInterval(interval);
resolve();
}
}, 100);
});
})
.then(done, done.fail);
});

it('should not confuse positions on either side of the globe', function(done) {
var gd = createGraphDiv();
var fig = Lib.extendDeep({}, require('@mocks/geo_orthographic.json'));
Expand Down Expand Up @@ -1421,66 +1381,6 @@ describe('Test geo interactions', function() {
.then(done, done.fail);
});

it('should plot to scope defaults when user setting lead to NaN map bounds', function(done) {
var gd = createGraphDiv();

spyOn(Lib, 'warn');

Plotly.newPlot(gd, [{
type: 'scattergeo',
lon: [0],
lat: [0]
}], {
geo: {
projection: {
type: 'kavrayskiy7',
rotation: {
lat: 38.794799,
lon: -81.622334,
}
},
center: {
lat: -81
},
lataxis: {
range: [38.794799, 45.122292]
},
lonaxis: {
range: [-82.904731, -81.622334]
}
},
width: 700,
heigth: 500
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

This case looks like it's actually broken in this PR. If I create the above plot on latest I get:

Screen Shot 2021-01-20 at 12 51 26 PM

But on this branch I get no warning and a completely blank plot.

Copy link
Contributor Author

@archmoj archmoj May 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The default view is the fallback resulted from catching d3-v3.geo error.
We get a blank view here as we are zoomed into the graph with that particular setup.
One could zoom out by clicking on the modebar.
So it looks like we could remove the fallback and the related test by a046871.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK, I guess this makes sense. The map is still in a somewhat broken state at that point: if you scroll-zoom until you can see the map appear from above, you can drag it up and down but not left/right. If you then (or instead) use the zoom out button, now you can drag left/right. Can we detect and fix this in the scroll zoom handler perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created an issue (see #5787) to track this edge case so that we could move on.

.then(function() {
var geoLayout = gd._fullLayout.geo;
var geo = geoLayout._subplot;

expect(geoLayout.projection.rotation).toEqual({
lon: 0, lat: 0, roll: 0,
});
expect(geoLayout.center).toEqual({
lon: 0, lat: 0
});
expect(geoLayout.lonaxis.range).toEqual([-180, 180]);
expect(geoLayout.lataxis.range).toEqual([-90, 90]);

expect(geo.viewInitial).toEqual({
'fitbounds': false,
'projection.rotation.lon': 0,
'center.lon': 0,
'center.lat': 0,
'projection.scale': 1
});

expect(Lib.warn).toHaveBeenCalledTimes(1);
expect(Lib.warn).toHaveBeenCalledWith(
'Invalid geo settings, relayout\'ing to default view.'
);
})
.then(done, done.fail);
});

it('should get hover right for choropleths involving landmasses that cross antimeridian', function(done) {
var gd = createGraphDiv();

Expand Down