Skip to content
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

Fix centroid calculation. Bump turf to v6.5 #7115

Merged
merged 15 commits into from
Aug 19, 2024
Merged

Fix centroid calculation. Bump turf to v6.5 #7115

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
828fa90
draftlog
birkskyum Aug 19, 2024
917c05d
Bump turf
birkskyum Aug 19, 2024
715e4ff
fix computeBbox
birkskyum Aug 19, 2024
a937945
remove .default
birkskyum Aug 19, 2024
d205bc9
update test
birkskyum Aug 19, 2024
b978c7a
fix single polygon case
birkskyum Aug 19, 2024
3418503
gracefully handle empty coordinate arrays
birkskyum Aug 19, 2024
90ee3da
update draftlog
birkskyum Aug 19, 2024
f4de36e
fix multipolygon case
birkskyum Aug 19, 2024
8a5f221
fix multipolygon test
birkskyum Aug 19, 2024
9c618e3
add thanks message similar to other draftlog
birkskyum Aug 19, 2024
dd05e55
update draftlog
birkskyum Aug 19, 2024
912244b
fix choropleth hover
birkskyum Aug 19, 2024
8e81005
update geo baselines
birkskyum Aug 19, 2024
a6e151a
turf 6.5
birkskyum Aug 19, 2024
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
2 changes: 2 additions & 0 deletions draftlogs/7115_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Update turf.js to v7 [[#7115](https://github.com/plotly/plotly.js/pull/7115)]

birkskyum marked this conversation as resolved.
Show resolved Hide resolved
113 changes: 62 additions & 51 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.

6 changes: 3 additions & 3 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@
"@plotly/d3-sankey": "0.7.2",
"@plotly/d3-sankey-circular": "0.33.1",
"@plotly/mapbox-gl": "1.13.4",
"@turf/area": "^6.4.0",
"@turf/bbox": "^6.4.0",
"@turf/centroid": "^6.0.2",
"@turf/area": "^7.1.0",
"@turf/bbox": "^7.1.0",
"@turf/centroid": "^7.1.0",
"base64-arraybuffer": "^1.0.2",
"canvas-fit": "^1.5.0",
"color-alpha": "1.0.4",
Expand Down
18 changes: 11 additions & 7 deletions src/lib/geo_location_utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@

var d3 = require('@plotly/d3');
var countryRegex = require('country-regex');
var turfArea = require('@turf/area');
var turfCentroid = require('@turf/centroid');
var turfBbox = require('@turf/bbox');
var { area: turfArea } = require('@turf/area');
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering how these imports are transpiled in to the dist?
May I ask you to test the dist from CI on the plotly.py side?

Copy link
Contributor Author

@birkskyum birkskyum Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Unless minified will they be kept as a destructuring assignment (plotly-geo.js bundle):

var {
  area: turfArea
} = __webpack_require__(9532);

Not sure how to run it in python

var { centroid: turfCentroid } = require('@turf/centroid');
var { bbox: turfBbox } = require('@turf/bbox');

var identity = require('./identity');
var loggers = require('./loggers');
Expand Down Expand Up @@ -226,7 +226,11 @@ function extractTraceFeature(calcTrace) {
};

// Compute centroid, add it to the properties
fOut.properties.ct = findCentroid(fOut);
if (fOut.geometry.coordinates.length > 0) {
fOut.properties.ct = findCentroid(fOut);
} else {
fOut.properties.ct = [NaN, NaN];
}

// Mutate in in/out features into calcdata
cdi.fIn = fIn;
Expand Down Expand Up @@ -291,7 +295,7 @@ function findCentroid(feature) {

for(var i = 0; i < coords.length; i++) {
var polyi = {type: 'Polygon', coordinates: coords[i]};
var area = turfArea.default(polyi);
var area = turfArea(polyi);
if(area > maxArea) {
maxArea = area;
poly = polyi;
Expand All @@ -301,7 +305,7 @@ function findCentroid(feature) {
poly = geometry;
}

return turfCentroid.default(poly).geometry.coordinates;
return turfCentroid(poly).geometry.coordinates;
}

function fetchTraceGeoData(calcData) {
Expand Down Expand Up @@ -362,7 +366,7 @@ function fetchTraceGeoData(calcData) {
// TODO `turf/bbox` gives wrong result when the input feature/geometry
// crosses the anti-meridian. We should try to implement our own bbox logic.
function computeBbox(d) {
return turfBbox.default(d);
return turfBbox(d);
}

module.exports = {
Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/choroplethmap_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -489,7 +489,7 @@ describe('Test choroplethmap convert:', function() {
var opts = convertModule.convert(calcTrace);

expect(opts.geojson.features[0].geometry.coordinates).toBe(coordsIn);
expect(calcTrace[0].ct).toEqual([100.4, 0.4]);
expect(calcTrace[0].ct).toEqual([100.5, 0.5]);
});

it('should find correct centroid (multi-polygon case)', function() {
Expand All @@ -498,7 +498,7 @@ describe('Test choroplethmap convert:', function() {
var coordsIn = [
[
// this one has the bigger area
[[30, 20], [45, 40], [10, 40], [30, 20]]
[[30, 20], [47, 40], [10, 33], [30, 20]]
],
[
[[15, 5], [40, 10], [10, 20], [5, 10], [15, 5]]
Expand All @@ -511,7 +511,7 @@ describe('Test choroplethmap convert:', function() {
var opts = convertModule.convert(calcTrace);

expect(opts.geojson.features[0].geometry.coordinates).toBe(coordsIn);
expect(calcTrace[0].ct).toEqual([28.75, 30]);
expect(calcTrace[0].ct).toEqual([29, 31]);
});
});

Expand Down Expand Up @@ -639,7 +639,7 @@ describe('Test choroplethmap hover:', function() {
return fig;
},
nums: '### 100',
name: '-86.7 | 32.0 ###',
name: '-86.7 | 31.9 ###',
evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}]
}];

Expand Down
8 changes: 4 additions & 4 deletions test/jasmine/tests/choroplethmapbox_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -490,7 +490,7 @@ describe('Test choroplethmapbox convert:', function() {
var opts = convertModule.convert(calcTrace);

expect(opts.geojson.features[0].geometry.coordinates).toBe(coordsIn);
expect(calcTrace[0].ct).toEqual([100.4, 0.4]);
expect(calcTrace[0].ct).toEqual([100.5, 0.5]);
});

it('should find correct centroid (multi-polygon case)', function() {
Expand All @@ -499,7 +499,7 @@ describe('Test choroplethmapbox convert:', function() {
var coordsIn = [
[
// this one has the bigger area
[[30, 20], [45, 40], [10, 40], [30, 20]]
[[30, 20], [47, 40], [10, 33], [30, 20]]
],
[
[[15, 5], [40, 10], [10, 20], [5, 10], [15, 5]]
Expand All @@ -512,7 +512,7 @@ describe('Test choroplethmapbox convert:', function() {
var opts = convertModule.convert(calcTrace);

expect(opts.geojson.features[0].geometry.coordinates).toBe(coordsIn);
expect(calcTrace[0].ct).toEqual([28.75, 30]);
expect(calcTrace[0].ct).toEqual([29, 31]);
});
});

Expand Down Expand Up @@ -640,7 +640,7 @@ describe('Test choroplethmapbox hover:', function() {
return fig;
},
nums: '### 100',
name: '-86.7 | 32.0 ###',
name: '-86.7 | 31.9 ###',
evtPts: [{location: 100, z: 10, pointNumber: 0, curveNumber: 0}]
}];

Expand Down