Always use original weights when handling --parse-conditionals-from-now #6399
Conversation
| @@ -583,8 +583,15 @@ 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, | |||
There was a problem hiding this comment.
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 :) ).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
| // we are going to overwrite this `.turn_weight_penalties` afterwards, | ||
| // 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"; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Can probably be discussed separately. There will be practical as well as philosophical issues.
| @@ -353,6 +353,18 @@ 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` | |||
There was a problem hiding this comment.
Decided to just extend existing test, but not add new one.
| # here we check that conditional restrictions can be updated via re-run of `contract`/`customize` with updated `--parse-conditionals-from-now` | ||
| # 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" |
There was a problem hiding this comment.
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.
Issue
closes #6117
Tasklist
Requirements / Relations
Link any requirements here. Other pull requests this PR is based on?