-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
feat: Switch geodata providers #7393
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.
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
base: master
Are you sure you want to change the base?
Conversation
tasks/topojson/get_geodata.mjs
Outdated
import config from './config.json' assert { type: 'json' }; | ||
|
||
try { | ||
// Download data from UN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As these origin URLs may change (link rot), I worry that this piece of the script could become out of date quickly. I'm not sure there's too much we can do here but I might recommend committing the current source file (but not adding it to the dist/
. Maybe someone else has a better idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems reasonable. The script could be updated to look for the file first and only download if it doesn't exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marthacryan do you have an opinion or ideas on this? What's the ideal dev UX here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, in 3b47c67
I added the archive from the UN.
Q for @camdecoster : did the old |
Trying to get
This still produces error messages like the one shown below saying that the JSON files can't be found:
I've tried variations on the path in
Adding an extra |
@gvwilson yes the old maps had 'subunits' layers for a few regions (USA and Brazil did for sure). The UN maps don't include that information. Thanks for looking into the tests. I'm working on an update to get the tests working but I haven't pushed that yet. I'll try to get that out later today. |
@etpinard when you have a chance could you give this PR a look? Would love your eyes. Thank you! In the short term what was the source for US states/Canadian provinces in the old dataset? |
Refactor to simplify scripts Switch to UN/NE geodata hybrid Save UN geodata to archive Remove extra info from topojson Add centroids to geojson Use 'simplify' to create 110m maps
@archmoj I've implemented the changes that you suggested. Could you take another look? Regarding the US/Canada border, that's just how the geodata looks coming from the UN. Manipulating that should be possible, but I'd rather wait until after these maps land to look at adding that. |
Files and size changes:
|
The file names need to be the same for the lookup to work in the library. We could update the lookup function to add a suffix, but that seems like the wrong direction to go. Eventually, the new maps will be standard. For now, we could manually update the CDN to include the new maps with a suffix. We could announce the change and then eventually update the CDN to only include the new maps. Alternatively, we could include the old maps in a legacy folder. |
In regards to new files:
would you consider adding new scopes for plotly.js/src/plots/geo/constants.js Lines 145 to 189 in 5e2163b
|
topojson/bin/process_geodata.mjs
Outdated
// Remove country polygons with bad geometry | ||
`-filter '!["ATA", "FJI", "RUS"].includes(iso3cd)' target=un_polygons`, | ||
'-merge-layers target=un_polygons,antarctica,fiji,russia force name=all_features', | ||
// Erase Caspian Sea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain the need for this process?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UN maps didn't show the outline of the Caspian sea in the country borders, so the script modifies the geodata here to cut the shape out. The NE maps have it cut out, so this changes matches what was provided previously. You can see the source data here without the sea visible in the countries layer (though you can see it in a water bodies layer):

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the note. How about modifying the comment to be
// Subtract Caspian Sea
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine. I'll be a bit more explicit too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Erase Caspian Sea | ||
`-filter 'globalid === "{BBBEF27F-A6F4-4FBC-9729-77B3A8739409}"' target=all_features + name=caspian_sea`, | ||
'-erase source=caspian_sea target=all_features', | ||
// Update country codes for disputed territories at Egypt/Sudan border: https://en.wikipedia.org/wiki/Egypt%E2%80%93Sudan_border |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please clarify why this process is needed? Is the UN data wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are disputed territories. The NE maps don't have them separated out, but the UN maps do. This was causing problems with the script due to how the metadata was assigned. I opted to give them different country codes so that we wouldn't have to decide which country "owns" which territory. I looked into the UN policy on these territories and their position is essentially that they're not taking a position. Switching to the UN data is an attempt to use a trusted source. So, I'm trying to follow the UN lead with this. You can see the difference between the old maps and the new below:
Natural Earth | UN |
---|---|
![]() |
![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you consider adding an image test for this case?
{
"data": [
{
"type": "choropleth",
"name": "Sudan",
"locations": ["SDN"],
"z": [1],
"colorscale": [[0, "blue"], [1, "blue"]],
"showscale": false,
"showlegend": true
},
{
"type": "choropleth",
"name": "Egypt",
"locations": ["EGY"],
"z": [1],
"colorscale": [[0, "orange"], [1, "orange"]],
"showscale": false,
"showlegend": true
}
],
"layout": {
"geo": {
"projection": {
"rotation": {
"lon": 30,
"lat": 20
},
"scale": 7
}
},
"width": 800,
"height": 600
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine with me. Thanks for the config!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you consider adding new scopes for antarctica and oceania here?
@archmoj I'll add the new scopes shortly.
// Erase Caspian Sea | ||
`-filter 'globalid === "{BBBEF27F-A6F4-4FBC-9729-77B3A8739409}"' target=all_features + name=caspian_sea`, | ||
'-erase source=caspian_sea target=all_features', | ||
// Update country codes for disputed territories at Egypt/Sudan border: https://en.wikipedia.org/wiki/Egypt%E2%80%93Sudan_border |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are disputed territories. The NE maps don't have them separated out, but the UN maps do. This was causing problems with the script due to how the metadata was assigned. I opted to give them different country codes so that we wouldn't have to decide which country "owns" which territory. I looked into the UN policy on these territories and their position is essentially that they're not taking a position. Switching to the UN data is an attempt to use a trusted source. So, I'm trying to follow the UN lead with this. You can see the difference between the old maps and the new below:
Natural Earth | UN |
---|---|
![]() |
![]() |
topojson/bin/process_geodata.mjs
Outdated
// Remove country polygons with bad geometry | ||
`-filter '!["ATA", "FJI", "RUS"].includes(iso3cd)' target=un_polygons`, | ||
'-merge-layers target=un_polygons,antarctica,fiji,russia force name=all_features', | ||
// Erase Caspian Sea |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The UN maps didn't show the outline of the Caspian sea in the country borders, so the script modifies the geodata here to cut the shape out. The NE maps have it cut out, so this changes matches what was provided previously. You can see the source data here without the sea visible in the countries layer (though you can see it in a water bodies layer):

topojson/bin/process_geodata.mjs
Outdated
function getCentroid(feature) { | ||
const { type } = feature.geometry; | ||
const projection = geoIdentity(); | ||
const path = geoPath(projection); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is also another variable called path in global scope.
Please rename this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done: 8944398.
|
||
exports.topojson = saneTopojson; | ||
module.exports = { | ||
topojson, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be
topojson, | |
topojson: topojson, |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of ES6, you can do this if the key and value variable have the same name (MDN source).
Description
Adds scripts to build topojson from UN sourced data and updates build process to run these scripts.
Closes #7334
Changes
Testing
npm run build_topojson
and make sure the script completes successfullyNotes
TODO
[ ] Clean up shapefiles after converting to topojsonSaving everything into the build folder makes this unnecessarydraftlogs
:7393_feat.md
properties
info from final maps