Skip to content

lad regions#548

Draft
Robinlovelace wants to merge 51 commits intomainfrom
lad-regions
Draft

lad regions#548
Robinlovelace wants to merge 51 commits intomainfrom
lad-regions

Conversation

@Robinlovelace
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy link
Copy Markdown
Contributor Author

Failing for Clackmannshire @wangzhao0217

Comment thread _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
Copy Markdown
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
Copy Markdown
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.

@Robinlovelace
Copy link
Copy Markdown
Contributor Author

Plan for this: use this approach in January, deploy results on test website, and then merge if good, finally 🚀

Comment thread route_ids.csv Outdated
@@ -1,4 +1,9 @@
nrow,plan,purpose,region,date,id
<<<<<<< HEAD
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Please tidy-up when you get a chance 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

2 participants