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.

Sign up for GitHub

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

Always use original weights when handling --parse-conditionals-from-now #6399

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Always use original weights when handling --parse-conditionals-from-now #6399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9fd452f
Always use original weights when handling --parse-conditionals-from-now
SiarheiFedartsou Oct 11, 2022
db82cec
Always use original weights when handling --parse-conditionals-from-now
SiarheiFedartsou Oct 11, 2022
4f10b15
Always use original weights when handling --parse-conditionals-from-now
SiarheiFedartsou Oct 11, 2022
dc27a07
Always use original weights when handling --parse-conditionals-from-now
SiarheiFedartsou Oct 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
- Changes from 5.27.0
- Routing:
- CHANGED: Add support for surface=metal,grass_paver,woodchips in bicyle profile. [#6395](https://github.com/Project-OSRM/osrm-backend/pull/6395)
- Tools:
- FIXED: Always use original weights when handling `--parse-conditionals-from-now` option. [#6399](https://github.com/Project-OSRM/osrm-backend/pull/6399)

# 5.27.0
- Changes from 5.26.0
Expand Down
22 changes: 22 additions & 0 deletions features/car/conditional_restrictions.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,28 @@ Feature: Car - Turn restrictions
| b | a | bj,jc,jc,aj,aj |
| b | d | bj,jd,jd |

# here we check that conditional restrictions can be updated via re-run of `contract`/`customize` with updated `--parse-conditionals-from-now`
Copy link
Member Author

Choose a reason for hiding this comment

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

Decided to just extend existing test, but not add new one.

# 10am utc, sat
When I run "osrm-contract {osm_file} --time-zone-file=test/data/tz/{timezone_names}/guinea.geojson --parse-conditionals-from-now=1494064800"
When I run "osrm-customize {osm_file} --time-zone-file=test/data/tz/{timezone_names}/guinea.geojson --parse-conditionals-from-now=1494064800"
Copy link
Member Author

Choose a reason for hiding this comment

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

Also probably we should vendor geojson with time zones to the repo, not sure what can be problem here, but it would ease a lot usage of this feature for beginners.


When I route I should get
| from | to | route |
| b | c | bj,jc,jc |
| b | a | bj,aj,aj |
| b | d | bj,jd,jd |

# 10am utc, wed
When I run "osrm-contract {osm_file} --time-zone-file=test/data/tz/{timezone_names}/guinea.geojson --parse-conditionals-from-now=1493805600"
When I run "osrm-customize {osm_file} --time-zone-file=test/data/tz/{timezone_names}/guinea.geojson --parse-conditionals-from-now=1493805600"

When I route I should get
| from | to | route |
| b | c | bj,jc,jc |
| b | a | bj,jc,jc,aj,aj |
| b | d | bj,jd,jd |


@no_turning @conditionals
Scenario: Car - Conditional restriction with multiple time windows
Given the extract extra arguments "--parse-conditional-restrictions"
Expand Down
13 changes: 11 additions & 2 deletions src/updater/updater.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,8 +583,17 @@ Updater::LoadAndUpdateEdgeExpandedGraph(std::vector<extractor::EdgeBasedEdge> &e
[&] { extractor::files::readNodeData(config.GetPath(".osrm.ebg_nodes"), node_data); },

[&] {
extractor::files::readTurnWeightPenalty(
config.GetPath(".osrm.turn_weight_penalties"), turn_weight_penalties);
// we are going to overwrite this `.turn_weight_penalties` afterwards,
Copy link
Member Author

Choose a reason for hiding this comment

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

This code modifies a couple of other files too and probably we should use similar approach for them too, but it is not so obvious as here: IIUC other files are related to traffic handling, i.e. it is more obvious for users that they need to provide data for all edges while updating speeds(but tbh I just had brief look into that code and not confident in it :) ).

Copy link
Member Author

Choose a reason for hiding this comment

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

On another hand it seems to be good idea to preserve "default" speed weights for segments user didn't provide data for 🤔 So would appreciate opinions here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mjjbell wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

The current behaviour of overwriting the files on updates means that you can make multiple independent changes and they will all apply.

This proposed change would alter that behaviour (only last update is applied), with the alternative being to apply the cumulative changes on every update, which will be slower. So I don't think we should go with this.

For someone who is updating turn weights via traffic updates, we can probably assume they have a model of the data for any time, so aren't too bothered about recovering the original weights.

For conditional penalties, I think what we need is an else case that can set the turn weight penalty back to the previous value.

osrm-backend/src/updater/updater.cpp

Lines 514 to 521 in 2cf9571

for (const auto &penalty : conditional_turns)
{
if (IsRestrictionValid(time_zone_handler, penalty))
{
turn_weight_penalties[penalty.turn_offset] = INVALID_TURN_PENALTY;
updated_turns.push_back(penalty.turn_offset);
}
}

Alternatively, rather then overwriting the weight value when the conditional is applied, we only set it in the local value used to update the edge weights? That way we aren't destroying the value used when the conditional doesn't apply.

osrm-backend/src/updater/updater.cpp

Line 765 in 2cf9571

auto turn_weight_penalty = turn_weight_penalties[edge.data.turn_id];

// so here we backup the original turn penalties if we didn't do that yet in order
// to guarantee that subsequent runs of this code will work on top of original
// weights
auto path = config.GetPath(".osrm.turn_weight_penalties").string() + ".original";
Copy link
Member Author

@SiarheiFedartsou SiarheiFedartsou Oct 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Also this problem made me think about if we can handle all these time based restrictions automatically without forcing users to make error-prone operations to handle this. It feels it should be relatively not so many time based restrictions in the graph and we probably can just handle it dynamically. It seems to be a bit against philosophy of OSRM when it tries to precalculate all weights to be as fast as possible, but at the same time it should add a lot of flexibility:

  • time based restrictions will "just work" without additional tools calls with --parse-conditionals-from-now
  • we potentially will be able to implement "depart at" feature

Copy link
Member

Choose a reason for hiding this comment

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

Can probably be discussed separately. There will be practical as well as philosophical issues.

if (!boost::filesystem::exists(path))
{
boost::filesystem::copy_file(config.GetPath(".osrm.turn_weight_penalties"),
path);
}
extractor::files::readTurnWeightPenalty(path, turn_weight_penalties);
},
[&] {
extractor::files::readTurnDurationPenalty(
Expand Down