-
-
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
@emilykl I'm not worried about the Natural Earth data. sane-topojson is years old and it still parses NE data correctly. The UN data might change, but I tried to reference attributes that would be set by standards (country codes, etc.). Regardless, I don't think it would be very difficult to update the scripts to filter the data correctly. @gvwilson I updated the download script to save into the build folder, so that will get ignored by git. If you're running the build multiple times locally, it will reuse the downloaded files. |
…s-emilykl-2 Updating baselines for new geodata
👍 from me once the image diffs we discussed are addressed -- looks great. |
This reverts commit 4d37b8c.
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