-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Handle conditional access tags for date ranges #6984
base: master
Are you sure you want to change the base?
Handle conditional access tags for date ranges #6984
Conversation
Thanks @ivarbrek! Could you please also extend tests? Feel free to ask for a help if it is not clear what to update… |
I've tried to add a few test cases. However, the tests seem to fail in the How do I proceed? |
@ivarbrek this test works locally, right? Try to bump cache key version here
|
6f59642
to
fe02369
Compare
fe02369
to
323e584
Compare
Finally got the tests running locally 😊 And think it should be good now! Who can I tag for review @SiarheiFedartsou? |
Happy to help with the review |
| primary | yes | no @ 2002 Jan 7 - 2002 Feb 8 | x | | ||
| primary | yes | no @ 2002 Jan 07 - 2002 Feb 08 | x | | ||
| primary | yes | no @ 2090 Jan 7 - 2100 Feb 8 | x | | ||
| primary | yes | no @ 2020 Jan 7 - 2050 Feb 8 | | |
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.
Will this test break in 24 years? 😉
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.
Yep 😄 I can extend the ranges so they don't break that soon, but I don't see any simple way to get completely rid of this problem.
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.
Ah, this can be solved doing something similar to this. I'll try to do that.
profiles/lib/access_conditional.lua
Outdated
|
||
local start_date = parse_date(start_date_str) | ||
local end_date = parse_date(end_date_str) | ||
local current_date = os.time() |
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.
This probably needs some explanation that the extraction result depends on the date the extraction is run. Also, the date is not checked once but for each case if I am not mistaken. A little of folks run data updates over night and this may lead to edge cases.
Consider pulling the date once at the start of extraction. Also, consider implementing an override, ie. supporting the use case of extracting data ahead of time.
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.
Good points 👍 Note that the current suggestion excludes date ranges that span less than a week, partly to avoid weird cases like this. But that choice can be discussed I think.
Anyways, I'll see if I manage to implement an overrride option in a reasonable way.
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.
Found a relevant example here, will try to implement something similar 👍
I've looked into this a bit more now, and here's my summary/understanding of the current situation: Some background I found out: Next steps
What I don't like about the two latter options is that we duplicate the date configuration we already have in the contract step. Would be interested to hear your take on that and your suggested next steps @DennisOSRM and @SiarheiFedartsou. |
@ivarbrek having it in osrm-customize/osrm-contract is indeed more preferable. In practice you would need to re-run this logic many times per day for the same dataset and such kind of things are done with these tools in OSRM(e.g. live traffic or conditional turn restrictions you mentioned are implemented using it) |
Allright, so we are maybe just closing this then? |
Issue
OSRM currently don't support conditional access tags. This is a very useful feature, especially in the context of roads being closed due to construction work or events.
Several people have requested this feature (#4231, #4300, #5801, #6706)
This PR takes a simple and lightweight approach to this problem, and handles it through the profile libs.
Note that we here only handle conditional time ranges, and require them to span for more than a week, using the format as described in the OSM wiki, e.g.
motor_vehicle:conditional=no @ 2018 May 22-2018 Oct 7
.Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?