-
Notifications
You must be signed in to change notification settings - Fork 3
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
LA programming #170
LA programming #170
Conversation
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.
Looking good. Minor comment about adding the la geojson file made.
code/add_site.R
Outdated
# Create intersection between site and ONS data | ||
la_site = sf::st_intersection(la_ew, site) | ||
site$main_local_authority = la_site$LAD19NM | ||
file.remove("la_boundaries.geojson") |
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.
No need to remove it: I suggest adding that file to the project, e.g. to the geojsons
folder. That also increases resilience: the arcgis endpoint will likely change at some point in the future.
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.
Agreed. Suggested changes are in the latest push
@@ -29,6 +29,7 @@ if (new_site) { | |||
# [7] "geometry" | |||
site = sf::read_sf("map.geojson") | |||
sf::st_crs(site) = 4326 | |||
site$main_local_authority = add_la(site) |
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.
👍 nice and simple.
@@ -38,7 +39,7 @@ if (new_site) { | |||
new_cols[] = NA | |||
sites = rbind(sites, | |||
sf::st_sf(cbind(sf::st_drop_geometry(site), new_cols), | |||
geometry = site$geometry)) | |||
geometry = site$geometry)) %>% arrange(site_name) |
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.
👍
Save la districts to geojson folder
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.
Looks fine, pending the question about
la_ew = sf::read_sf("geojsons/la_boundaries.geojson")
This is good to go!
code/add_site.R
Outdated
method = 'wininet') | ||
# Read input data | ||
la_ew = sf::read_sf("la_boundaries.geojson") %>% sf::as_Spatial() %>% sf::st_as_sf() | ||
la_ew = sf::read_sf("geojsons/la_boundaries.geojson") %>% sf::as_Spatial() %>% sf::st_as_sf() |
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.
What's wrong with just:
la_ew = sf::read_sf("geojsons/la_boundaries.geojson")
?
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.
Have pushed this change with an edit to the add site tutorial 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.
Going to merge and delete
I have updated all sites (except great-kneighton & trumpington meadows, something funky going on there, continuing to investigate) to programmatically add
main_local_authority
toall-sites.geojson
. This was achieved through the following reproducible example:I have slightly refactored the
add_site.r
andbuild.r
file in order to automate the LA column.