cyipt / actdev

ActDev - Active travel provision and potential in planned and proposed development sites
https://actdev.cyipt.bike
7 stars 3 forks source link

abstr_scenarios.r not compatible for new sites #159

Closed natesheehan closed 3 years ago

natesheehan commented 3 years ago

abstr_scenarios.r needs to be refactored in order to cater to new sites.

The followig lines are the culprit:

sites_df = readr::read_csv("data-small/sites_df_abstr.csv")
sites_df$n_origin_buildings[j] = nrow(houses)

as sites_df_abstr.csv does not contain the new sites.

Quick fix: add new site rows before the merge. Thoughts @Robinlovelace ?

Robinlovelace commented 3 years ago

Quick fix: add new site rows before the merge. Thoughts @Robinlovelace ?

Yes I think that should be a fairly quick fix. Also, is the 'disggregation code' failing for other sites also? May be worth opening an issue on that also although it's not a biggy it's important in rural areas where MSOA centroids are few and far between.

natesheehan commented 3 years ago

New branch for adding abstreet files for the new batch of sites https://github.com/cyipt/actdev/tree/abstreet_new_sites_scenarios

Currently adding the new batch of sites by:

if (new_site) {
  newdf <- data.frame(site_name = site_name,
                      dwellings_when_complete = n_dwellings_site,
                      n_origin_buildings = nrow(houses),
                      n_destination_buildings = nrow(buildings_in_zones)
  )

  sites_df = rbind(newdf,sites_df) %>% arrange(site_name)

  readr::write_csv(sites_df, "data-small/sites_df_abstr.csv")
}
Robinlovelace commented 3 years ago

Looks good to me, look forward to the PR! FYI chatted with @dabreegster and the plan is to get the procgen binary going at some point this week. abstr is in need of TLC but the abstreet scenario building script should work. Feel free to open an issue on that, I recall it was crashing when we tried on the test sites.

natesheehan commented 3 years ago

@Robinlovelace Yep, I believe the issue with abstr-scenarios.r is now fixed.

Going to test by adding a new site from the build.r script and then will open a PR 🚀

natesheehan commented 3 years ago

Closed with PR https://github.com/cyipt/actdev/pull/169