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

a prototype of creating AccessModels #113

Merged
merged 19 commits into from
Mar 22, 2024
Merged

Conversation

robfitzgerald
Copy link
Collaborator

@robfitzgerald robfitzgerald commented Feb 6, 2024

this PR makes AccessModel a top-level Model module (routee-compass-core::model::access). the access_edge method has been removed from traversal models. a few default implementations are in place:

  • no_access_model: a simple noop model
  • combined: more than one access model
  • turn_delay: assigns turn delay times from a table based on the heading of the transition

an example configuration for a turn_delay access model:

[access]
type = "turn_delay"
edge_heading_input_file = "../data/tomtom_denver/edges-headings-enumerated.csv.gz"
[access.turn_delay_model]
type = "tabular_discrete"
time_unit = "seconds"
[access.turn_delay_model.table]
no_turn = 0.0
slight_right = 0.5
right = 1.0
sharp_right = 1.5
slight_left = 1.0
left = 2.5
sharp_left = 3.5
u_turn = 9.5

wanted to leave a variation of turn delays that incorporates road classes but decided to let that sit and left a comment as a placeholder for it.

along the way, i fixed some missing feature injections. SpeedTraversalModel::state_features() wasn't returning it's features and EnergyTraversalModel::state_features() wasn't returning the features of it's underlying time model (touches #145).

i've completed this implementation but i'm unsure if the results are correct, and it may be connected with the discussion around how we pass around units. that said, i did see that my output.json's geometry properties did include access costs, which implies the logic was being called. leaving follow-up to our next dev check-in.

Closes #104.
Closes #109.

@robfitzgerald robfitzgerald requested a review from nreinicke March 22, 2024 18:53
@robfitzgerald robfitzgerald marked this pull request as ready for review March 22, 2024 19:01
@nreinicke
Copy link
Collaborator

This looks great. I just tested out on the osm data and without a turn delay model I get a time of 10.6 minutes and with a turn delay model I get a time of 10.9 seconds so it seems like it's being incorporated!

I did have to change the dataset script to use arrival_heading and departure_heading and so we should probably fix that really quick (I can push a commit here if you want). Other than that it looks good to me!

@robfitzgerald
Copy link
Collaborator Author

You sure you pulled down the latest headings dataset via lfs?

@nreinicke
Copy link
Collaborator

You sure you pulled down the latest headings dataset via lfs?

I was actually testing on the OSM dataset that gets built from our generate_dataset script. I think we just need to update how we're writing our headings file to match the new format (previously I used start_heading and end_heading but now the rust code expects arrival_heading and destination_heading).

@robfitzgerald
Copy link
Collaborator Author

previously I used start_heading and end_heading but now the rust code expects arrival_heading and destination_heading

right. this was a conscious choice at some point. but, it also is a choice that was made within the fever dream of working on routee while you were on PTO. it wasn't necessary, and now i'm having trouble convincing myself that this was helpful. but i recall i was confused by the meaning of "start" and "end" too. maybe there's a better choice? but i'll update the osm generation code for now so it works!

Copy link
Collaborator

@nreinicke nreinicke left a comment

Choose a reason for hiding this comment

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

Yeah I think it's fine to use the arrival destination distinction, thanks for pushing up that fix!

@robfitzgerald robfitzgerald merged commit e853897 into main Mar 22, 2024
5 checks passed
@robfitzgerald robfitzgerald deleted the rjr/headings-module branch March 22, 2024 22:26
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.

abstraction around access modeling, starting with turn costs [Discussion] Turn Cost Abstracion
2 participants