Allow abitrary edge weights #2399

TheMarex · 2016-05-12T17:58:08Z

This PR aims to finally implement #77.

Where we are now:

ExtractionWay has a new property weight_per_meter that we pull through the whole pipeline to the server. It is exposed over the fields named weight (analogous to duration).
ProfileProperties has a new property weight_name

TODO

This needs a rebase, probably conflicts with the Datafacade changes.
weight_per_meter is a wrong name. This was an oversight from my part: It actually encodes weight_per_second (analogous to speed being meters_per_second) We need to rename this. /cc @karenzshea
We need to integrate Profile version #3487 to check the profile version:
- The interface between ScriptingEnvironment and EdgeBasedGraphFactory here needs to work with both API version
- The interface between ScriptingEnvironment and ExtractionContainers here also needs to work with both API version.
- Profile API version 0: We expect the current implementation of turn_function and segment_function and silently fallback to weight = duration (this needs wrapper code in ScriptingEnvironment::ProcessTurn and ScriptingEnvironment::ProcessSegment)
- Profile API version 1: We expect turn_function and segment_function to follow the scheme already implemented in this PR

danpat · 2016-05-12T18:57:42Z

Capturing from chat:

what we really want from this is to not need to worry any more about what metric we put in and still get correct durations.

Let's worry about memory consumption later - let's see how useful this feature turns out first before we optimize.
The duration data needs to support turn penalties. If we do time-as-weight routing, we need to support them there too. We can store them at the end of the GetUncompressedWeights array, it'll add 4 bytes for each compressed geometry.
Let's only support a weight_per_distance value from the user, rather than arbitrary total values. Everything gets scaled by the length of the road.
The way_function can return two values per way - speed and weight_per_distance (note that both these fields are per-distance measurements).
The turn_function can return two values - time_penalty and weight_penalty

For traffic data imports, let's look at some use cases:

if we're doing fastest-time routing, we will need to update both the routing weight and the stored durations
if we're doing shortest-distance routing, but adjusting durations for traffic, we need to update the stored durations, but not the weight
if we're trying to support "minimize toll costs" in a situation where tolls are changing constantly (dynamic pricing), but without traffic data, we would need to update the weights, but not the durations

The last case is a bit of a stretch, I couldn't think of a better example. But the point is that we probably need to be able to update both with the CSV import. I think we could do:

update the CSV file to have 4 columns, nodeA,nodeB,speed,weight_per_distance
supply a second CSV file with nodeA,nodeB,weight_per_distance

I think if we simply don't supply data, we can efficiently skip either the duration and/or weight updates.

emiltin · 2016-05-12T20:11:34Z

wow, this has been a long time coming! it will be very useful for bike routing

TheMarex · 2016-05-16T17:08:09Z

nodeA,nodeB,speed,weight_per_distance

We can update the data with regard to the names given in the csv header column.

MoKob · 2016-06-29T15:13:57Z

include/contractor/graph_contractor.hpp

@@ -154,7 +154,7 @@ class GraphContractor
        for (auto diter = input_edge_list.dbegin(); diter != dend; ++diter)
        {
            BOOST_ASSERT_MSG(static_cast<unsigned int>(std::max(diter->weight, 1)) > 0,


Isn't max(weight,1) always >0, especially since we cast to an unsigned?

stwa · 2016-08-01T08:51:08Z

include/extractor/extractor_config.hpp

@@ -71,7 +71,9 @@ struct ExtractorConfig
        rtree_nodes_output_path = basepath + ".osrm.ramIndex";
        rtree_leafs_output_path = basepath + ".osrm.fileIndex";
        edge_segment_lookup_path = basepath + ".osrm.edge_segment_lookup";
-        edge_penalty_path = basepath + ".osrm.edge_penalties";
+        turn_duration_penalties_path = basepath + ".osrm.turn_weight_penalties";
+        turn_weight_penalties_path = basepath + ".osrm.turn_duration_penalties";


Looks like the values are swapped:
turn_weight_penalties → turn_duration_penalties and vice versa

good catch 👍

TheMarex · 2016-08-01T09:40:51Z

Things to do here:

Split interleaved CompressedGeometryContainer serialization into separate vectors
Only load the duration vector if weight doesn't contain the same values (safes memory consumption)
Make duration values in osrm-contract updatable

oxidase · 2016-08-17T16:24:35Z

src/extractor/edge_based_graph_factory.cpp

+                    duration_penalty += profile_properties.traffic_signal_penalty;
+                }
+
+                if (use_turn_function)


use_turn_function is not defined, may the changeset it is incomplete?

oxidase · 2016-08-17T16:36:17Z

Hi, i have almost finished rebase of the PR to master, but some parts are missing. If it is interesting, i can create a duplicate PR with rebased patches.

TheMarex · 2016-08-18T07:26:39Z

@oxidase Let me just give your contributor access so you can push directly to this branch.

oxidase · 2016-08-21T15:38:26Z

Please review a forced rebase of the branch to master. I have modified some commits, changed some properties in profiles and changed TurnPenalty to int16_t to make all tests green.

oxidase · 2017-01-26T21:39:35Z

@TheMarex updated and rebased the brach, but in some places i did not make changes:

traffic_light_penalty is still used in turn_function to have old behavior, because TODO fix is required
MAX_WEIGHT_NAME_LENGTH is still a value in enum
forward_heap_3 and reverse_heap_3 are still in use
FIXME is still there but now related only the situation when source and target phantoms on one edge

TheMarex · 2017-01-26T22:54:27Z

traffic_light_penalty is still used in turn_function to have old behavior, because TODO fix is required

You are right, I confused myself with my own changelog entry. Currently it is:

traffic_light_penalty is deprecated. traffic light penalties now need to be set over in the node function in result.weight_penalty and result.duration_penalty fields

Correct however would be:

properties.traffic_light_penalty is deprecated. Traffic light penalties now need to be set over in the turn function. Each turn with a traffic light is marked with ExtractionTurn::has_traffic_light = true.

This looks good to me. I think we can merge this (wow so weird).

oxidase · 2017-01-26T22:59:20Z

looks like std::size_t is not integral type? will change to static constexpr int MAX_WEIGHT_NAME_LENGTH = 255;

djschilling · 2017-01-27T20:22:37Z

Thanks for all the effort you are putting into this.
This pull request getting merged is really great for us.

danpat · 2017-01-27T21:00:51Z

@djschilling @emiltin and anyone else who has been watching this ticket - we've merge into master but feedback on this feature would be really helpful. Anyone who has the time to test it out, please do and open tickets with any problems/confusion you encounter.

djschilling · 2017-01-27T21:55:43Z

We are using an earlier state of the pull request since the start of the year and it does exactly what it should.
We use it to correctly handle access:destination tags, by putting a high weight to access:destination ways.

One thing that we encountered is a performance loose from about 40%. Its not critical for us, but i think its worth mentioning.
I have no time now to test the currently merged version, so feedback from someone else would probably be good.

http://www.openstreetmap.org/way/24455356

TheMarex changed the title ~~Allow abitrary edge weights~~ [not ready] Allow abitrary edge weights May 12, 2016

TheMarex added this to the 6.0 milestone May 23, 2016

TheMarex force-pushed the feature/edge_weights branch 3 times, most recently from 92bdb88 to b03c3a6 Compare June 12, 2016 19:20

TheMarex force-pushed the feature/edge_weights branch from 45f9d06 to d6754a6 Compare June 22, 2016 11:20

MoKob reviewed Jun 29, 2016
View reviewed changes

TheMarex force-pushed the feature/edge_weights branch from e6181c5 to cf3eee4 Compare July 1, 2016 12:54

stwa reviewed Aug 1, 2016
View reviewed changes

TheMarex force-pushed the feature/edge_weights branch from 3d11c97 to a06f134 Compare August 1, 2016 09:30

oxidase reviewed Aug 17, 2016
View reviewed changes

This was referenced Aug 18, 2016

Distance support for table service #2764

Closed

Fix/1318 #2773

Closed

TheMarex changed the title ~~[not ready] Allow abitrary edge weights~~ [wip] Allow abitrary edge weights Aug 19, 2016

oxidase force-pushed the feature/edge_weights branch from 6a4a2a4 to f316d03 Compare August 21, 2016 15:29

Added weight multipliers for speed and turn updates

3b6f865

oxidase force-pushed the feature/edge_weights branch from e186963 to 8d1f156 Compare January 26, 2017 21:27

oxidase force-pushed the feature/edge_weights branch from 8d1f156 to 28589e7 Compare January 26, 2017 21:45

oxidase added 2 commits January 26, 2017 23:13

Add duration to edges and use it in table plugin

7c95d69

Use duration in SegmentBlock for duration updates

5da9e08

oxidase force-pushed the feature/edge_weights branch 2 times, most recently from 28b1c6e to 02880cf Compare January 26, 2017 22:16

Update review findings

f6dca2b

oxidase force-pushed the feature/edge_weights branch from 02880cf to f6dca2b Compare January 26, 2017 23:01

Smaller grammar and formating fixes for the changelog

d871724

danpat mentioned this pull request Jan 27, 2017

autoroute suggested for reaching residential house protected by private access gate #3616

Closed

TheMarex merged commit a572719 into master Jan 27, 2017

TheMarex added Ready To Merge and removed Review labels Jan 27, 2017

TheMarex mentioned this pull request Jan 27, 2017

Make travel time and edge weights independent #77

Closed

MoKob mentioned this pull request Jan 30, 2017

osrm-extract triggers on demo server #3629

Closed

oxidase mentioned this pull request Feb 1, 2017

Split if in ProcessWay and adjsuted profiles to comform to old behavior. #3632

Merged

2 tasks

oxidase referenced this pull request Feb 1, 2017

Restrict bicycle routing without access for railways

38e89fc

http://www.openstreetmap.org/way/24455356

This was referenced Feb 3, 2017

Reduce memory in stxxl #3646

Merged

remove 4 bytes packing of NodeBasedEdgeWithOSM due to alignment fails #3655

Merged

Fix of an assertion in bicycle profile #3656

Merged

MoKob deleted the feature/edge_weights branch February 14, 2017 11:19

oxidase mentioned this pull request Mar 9, 2017

Compute correct speed values in tile plugin #3790

Merged

2 tasks

danpat mentioned this pull request Apr 24, 2017

Bike profiles route through alleys in Montreal - this is less than desirable #3971

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow abitrary edge weights #2399

Allow abitrary edge weights #2399

TheMarex commented May 12, 2016 •

edited

Loading

danpat commented May 12, 2016 •

edited

Loading

emiltin commented May 12, 2016

TheMarex commented May 16, 2016

MoKob Jun 29, 2016

stwa Aug 1, 2016

TheMarex Aug 1, 2016

TheMarex commented Aug 1, 2016 •

edited

Loading

oxidase Aug 17, 2016

oxidase commented Aug 17, 2016

TheMarex commented Aug 18, 2016

oxidase commented Aug 21, 2016

oxidase commented Jan 26, 2017

TheMarex commented Jan 26, 2017

oxidase commented Jan 26, 2017

djschilling commented Jan 27, 2017

danpat commented Jan 27, 2017

djschilling commented Jan 27, 2017

Allow abitrary edge weights #2399

Allow abitrary edge weights #2399

Conversation

TheMarex commented May 12, 2016 • edited Loading

TODO

danpat commented May 12, 2016 • edited Loading

emiltin commented May 12, 2016

TheMarex commented May 16, 2016

MoKob Jun 29, 2016

Choose a reason for hiding this comment

stwa Aug 1, 2016

Choose a reason for hiding this comment

TheMarex Aug 1, 2016

Choose a reason for hiding this comment

TheMarex commented Aug 1, 2016 • edited Loading

oxidase Aug 17, 2016

Choose a reason for hiding this comment

oxidase commented Aug 17, 2016

TheMarex commented Aug 18, 2016

oxidase commented Aug 21, 2016

oxidase commented Jan 26, 2017

TheMarex commented Jan 26, 2017

oxidase commented Jan 26, 2017

djschilling commented Jan 27, 2017

danpat commented Jan 27, 2017

djschilling commented Jan 27, 2017

TheMarex commented May 12, 2016 •

edited

Loading

danpat commented May 12, 2016 •

edited

Loading

TheMarex commented Aug 1, 2016 •

edited

Loading