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

SiarheiFedartsou · 2022-10-11T21:04:40Z

Issue

closes #6117

Tasklist

CHANGELOG.md entry (How to write a changelog entry)
update relevant Wiki pages
add tests (see testing documentation)
review
adjust for comments
cherry pick to release branch

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

SiarheiFedartsou · 2022-10-11T21:07:39Z

src/updater/updater.cpp

@@ -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,


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 :) ).

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.

@mjjbell wdyt?

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];

SiarheiFedartsou · 2022-10-11T21:15:25Z

src/updater/updater.cpp

+                // 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";


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

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

SiarheiFedartsou · 2022-10-11T21:16:35Z

features/car/conditional_restrictions.feature

@@ -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`


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

SiarheiFedartsou · 2022-10-11T21:21:10Z

features/car/conditional_restrictions.feature

+        # 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"


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.

github-actions · 2024-07-08T20:31:39Z

This PR seems to be stale. Is it still relevant?

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

9fd452f

SiarheiFedartsou commented Oct 11, 2022

View reviewed changes

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

db82cec

SiarheiFedartsou commented Oct 11, 2022

View reviewed changes

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

4f10b15

SiarheiFedartsou marked this pull request as ready for review October 11, 2022 21:23

SiarheiFedartsou marked this pull request as draft October 11, 2022 21:24

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

dc27a07

SiarheiFedartsou marked this pull request as ready for review October 11, 2022 21:52

github-actions bot added the Stale label Jul 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

SiarheiFedartsou commented Oct 11, 2022 •

edited

Loading

SiarheiFedartsou Oct 11, 2022

SiarheiFedartsou Oct 11, 2022

SiarheiFedartsou Oct 11, 2022

mjjbell Oct 12, 2022

SiarheiFedartsou Oct 11, 2022 •

edited

Loading

mjjbell Oct 12, 2022

SiarheiFedartsou Oct 11, 2022

SiarheiFedartsou Oct 11, 2022

github-actions bot commented Jul 8, 2024

	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);
	}
	}

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

Are you sure you want to change the base?

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

Conversation

SiarheiFedartsou commented Oct 11, 2022 • edited Loading

Issue

Tasklist

Requirements / Relations

SiarheiFedartsou Oct 11, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Oct 11, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Oct 11, 2022

Choose a reason for hiding this comment

mjjbell Oct 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Oct 11, 2022 • edited Loading

Choose a reason for hiding this comment

mjjbell Oct 12, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Oct 11, 2022

Choose a reason for hiding this comment

SiarheiFedartsou Oct 11, 2022

Choose a reason for hiding this comment

github-actions bot commented Jul 8, 2024

SiarheiFedartsou commented Oct 11, 2022 •

edited

Loading

SiarheiFedartsou Oct 11, 2022 •

edited

Loading