-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add turn restrictions #95
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.
dig it, look forward to checking out results that honor the real, tangible, physical world a little better 🚗
as for this solution to "multiple frontier models", i'm wondering what your take on this style vs making a "Combined" (or whatever named) variant that encapsulates the idea of multiple models, such as here. something like:
pub struct CombinedFrontierModel {
models: Vec<Box<dyn FrontierModel>>
}
impl FrontierModel for CombinedFrontierModel {
fn valid_frontier(
&self,
_edge: &Edge,
_state: &TraversalState,
_previous_edge: Option<&Edge>,
) -> Result<bool, FrontierModelError> {
// does the logic of stepping through it
}
}
this recursive style means we don't need to push changes into the rest of the program, in particular we don't expose the logic of stepping through frontier models to CompassApp or others.
another reason not to push the &[T]
refactor into the code base: another kind of "combined" that i'm experiencing over in the mep-routee-compass code base is collected as a hashmap, not a vector, because only certain traversal models are valid for a given query. so, that kind of behavior can't be modeled with a &[T]
wrapper unless we somehow pass some index information along with it. the recursive approach is more general, it supports both of these kinds of collections. whatdyathink?
Yeah, that is a much more general way to do it, good call. I'll take a pass at that and update this PR! |
@robfitzgerald - Okay, I've gone in and introduced a new |
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.
thanks for updating and providing that combined variant. i ran the denver test query and saw that, with turn restrictions, there are 490 additional tree edges explored, so it clearly did something. at a glance the logic is simple so i'm guessing it's correct. this is all dancing around the fact that we don't have a real test, but, maybe that's just something we keep in mind in case some results look weird.
i've got a request in here about the CSV deserialization, to make it more explicit about column names, but i feel like that could also become a future issue if you want to punt it.
...-compass/src/app/compass/config/frontier_model/turn_restrictions/turn_restriction_builder.rs
Outdated
Show resolved
Hide resolved
...-compass/src/app/compass/config/frontier_model/turn_restrictions/turn_restriction_builder.rs
Show resolved
Hide resolved
dig it! |
Implements #94 by adding a new turn restriction frontier model which loads a set of edge id tuples that each represent a turn restriction.
This also adds the ability for multiple frontier models to be provided and iterates over each model, excluding an edge from the search if any of the models result in an invalid frontier.