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.

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

lad regions #548

Draft
wants to merge 44 commits into
base: main
Choose a base branch
from
Draft

lad regions #548

wants to merge 44 commits into from

Conversation

Robinlovelace
Copy link
Contributor

  • Update gitignore
  • comment-out everything after school routing, for tests
  • Testing pattern in targets
  • Working dynamic branching (it seems)!
  • Dynamic branching for commuting too
  • Add garbage_collection
  • Try lads as regions

@Robinlovelace
Copy link
Contributor Author

The last commit above actually fixes an error and should be ported to main ASAP:

image

@Robinlovelace Robinlovelace marked this pull request as ready for review October 31, 2024 22:11
@Robinlovelace
Copy link
Contributor Author

Hi @wangzhao0217 I think this is the way forward. Advantages of using LADs as regions for build:

  • More consistent size
  • Allows greater parallelisation
  • Easier debugging and testing for a single LAD

There may also be some disadvantages. We will have to make changes to build.R but I think it could actually make things in that script quicker and simpler.

@Robinlovelace Robinlovelace marked this pull request as draft November 1, 2024 08:26
@Robinlovelace
Copy link
Contributor Author

Any update on this @wangzhao0217 ? I should be able to have a bash later this evening or in the next few days.

@Robinlovelace Robinlovelace mentioned this pull request Nov 6, 2024
Copy link
Contributor Author

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

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

Great progress...

@@ -1,11 +1,12 @@
corenet_build_OS = function(os_scotland, osm_scotland, la_names) {

message("Generate the city's coherent network for each region with growing")
message("Generate the coherent network for each LA")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


for (la_name in la_names) {
for (la_name in la_names[2:32]) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 testing at scale!

route_ids.csv Outdated
@@ -1,4 +1,10 @@
nrow,plan,purpose,region,date,id
4140,ebike,commute,dumfries_and_galloway,2024-11-31,11332
987,ebike,school,dumfries_and_galloway,NA,11328
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the NAs?

@Robinlovelace
Copy link
Contributor Author

Failing for Clackmannshire @wangzhao0217

_targets.R Outdated
Comment on lines 253 to 271
tar_target(r_commute,
rs_commute[[1]]$routes,
pattern = map(rs_commute),
iteration = "list"
),
tar_target(r_commute_fastest, {
rs_commute[[1]][[1]]$routes
}),
tar_target(r_commute_quietest, {
rs_commute[[3]][[1]]$routes
}),
tar_target(r_commute_ebike, {
rs_commute[[4]][[1]]$routes
}),
tar_target(r_commute_balanced, {
rs_commute[[2]][[1]]$routes
}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This undoes the conciseness benefits of using dynamic branching, right?

Copy link
Contributor Author

@Robinlovelace Robinlovelace left a comment

Choose a reason for hiding this comment

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

Few more changes to minimize difference compared with main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants