profiles api v2 by emiltin · Pull Request #4072 · Project-OSRM/osrm-backend

emiltin · 2017-05-22T13:26:45Z

improves the profile API and bumps the version from 1 to 2.

Tasklist

Requirements / Relations

This is a breaking change of the profile API.

oxidase · 2017-05-22T18:29:32Z

@emiltin max_turn_weight is a read-only property that depends on TurnPenalty type and weight_precision. It is used to clip turn penalties in Lua to prevent overflows in conversions double to TurnPenalty. It can be set together with context.properties.weight_precision as

    if( weight_precision != sol::nullopt) {
      context.properties.weight_precision = weight_precision.value();
      context.properties.weight_multiplier = std::pow(10., context.properties.weight_precision);
      context.properties.max_turn_penalty = std::numeric_limits<TurnPenalty>::max() / weight_multiplier;
   }

emiltin · 2017-05-23T09:02:14Z

ok. but is it really needed in the lua profiles then? can the capping be done on the c++ side?

oxidase · 2017-05-23T09:54:58Z

it is needed in profiles as the maximal turn penalty that is used in transit routing restrictions. The turn restriction value can be clipped in c++ by removing assertions, but it should be made explicit with warnings

emiltin · 2017-05-23T10:07:51Z

can the profile use a big constant as the max weight, and then let the c++ side cap it according to the current precision?

oxidase · 2017-05-23T10:11:06Z

@emiltin yes, it is possible /cc @TheMarex

emiltin · 2017-05-23T11:07:25Z

all tests are green. profiles can now use constants.max_turn_weight if needed. it's defined as a constant with the value -1. this assumes that negative turn weights are not valid, is that true?

this PR is a breaking change changes the profile interface a bit.

emiltin · 2017-05-23T11:31:06Z

rebased

emiltin · 2017-05-23T12:38:53Z

travis is failing with link errors:
/home/travis/build/Project-OSRM/osrm-backend/src/extractor/scripting_environment_lua.cpp:406: error: undefined reference to 'osrm::extractor::Sol2ScriptingEnvironment::MAX_TURN_WEIGHT'

it's compiling for me locally with gcc on linux. not sure why it's failing on travis?

oxidase

undefined reference error can be related to different Release/Debug options in sol2:

struct S { static const int a = 1; };
int main() {  return S::a;  }

is ok, but &S::a would require a definition for S::a

oxidase · 2017-05-24T08:08:30Z

include/extractor/scripting_environment_lua.hpp

  public:
    static const constexpr int SUPPORTED_MIN_API_VERSION = 0;
    static const constexpr int SUPPORTED_MAX_API_VERSION = 1;
+    static const constexpr int MAX_TURN_WEIGHT = -1;


Negative turn restrictions are correct values #3683

ok, then of course we cannot use -1 as a special value

oxidase · 2017-05-24T08:13:18Z

src/extractor/scripting_environment_lua.cpp

            turn_function(turn);

+            if( turn.weight == MAX_TURN_WEIGHT )
+              turn.weight = context.properties.GetMaxTurnWeight();


It is better to set here std::min(turn.weight, context.properties.GetMaxTurnWeight()) to avoid MAX_TURN_WEIGHT and print later number of clamped values

you mean unconditionally cap the value? ie:

turn.weight = std::min(turn.weight, context.properties.GetMaxTurnWeight());

what value should the profile use then to indicate max weight?

the only purpose of the turn penalty maximum weight is to prevent int32_t overflows on c++ side. If turn penalties are modified on c++ then side the maximum value is not needed in Lua, but number of modifications must be reported as a warning.

emiltin · 2017-05-24T13:20:05Z

@oxidase adjusted according to your suggestions. let's see what travis thinks.

oxidase · 2017-05-26T10:01:41Z

@emiltin i have pushed some commits directly to the branch to check changes on travis. The main change is in 62a0819 where max_turn_weight is moved to be a member of profiles properties. If it s not ok, you can kick out of the branch these commits.

TheMarex

General idea is fine, however this is a breaking change to the profile API. That means we need to bump the version and offer a fallback using the global properties.

oxidase · 2017-05-26T10:18:27Z

@emiltin @TheMarex if the profile version is increased then 6e056e1 must be kicked out

emiltin · 2017-05-26T21:32:35Z

thanks @oxidase for the commits.
@TheMarex yes, backward compatibility needs to be in place, that's not handled yet.

emiltin · 2017-06-07T09:16:52Z

i think it's best to increase the api version to 2, and then incorporate other relevant changes to the api. it can then be merged at an appropriate osrm version change.

emiltin · 2017-06-07T09:35:36Z

library files like handlers.lua are tied to a specific version of the api, for example because it accceses properties.weight_name.
this means that if you update osrm you also need to update the lua library. it also means that tests for older api version cannot use the library files, since the library files work with the latest api version.

emiltin · 2017-06-07T12:59:40Z

for version 2 of the api we might want to improve the way versioning is handled: #4133

TheMarex

Looks good! The only missing pieces are documentation now:

docs/profile.md needs to be updated
CHANGELOG.md needs an entry for 5.10

emiltin · 2017-07-13T08:07:58Z

is it correctly understood that the weight is just the inverse of the rate? and that setting output.weight in way_function is just a shorthand for setting both forward and backward rates?

should we change output.weigh to output.rate?

emiltin · 2017-07-13T08:25:51Z

seems that weight = distance/rate. i'm trying to explain this in doc/profiles.md

Understanding speed, weight, rate and distance

When computing a route from A to B there can be different measure of what is the best route. Because speeds very on differnt types of roads, the shortest and the fastest route are typically different. But there are many other possible preferences. For example a user might prefer a bicycle route that follow parks or other green areas, even though both duration and distance are a bit longer.

To handle this, OSRM doesn't simply choose the ways with the highest speed. Intead it uses the concept of weight and rate. The rate is an abstract meassure that you can assign to to ways as you like to make some ways preferable to others. Routing will prefer ways with high rate.

The weight of a way is computed as distance / rate. The weight can be though of as the resistance or cost when passing a way. Routing will prefer ways with low weight.

You should set the speed to you best estimante of the actual speed that will be used on a particual way. This will result in the best estimated travel times.

If you want to prefer certain ways due to other factors than the speed, adjust the rate accordingly. If you adjust the speed, the time time estimation will be skewed.

If you set the same rate on all ways, the result will be shortest path routing.
If you set rate = speed on all ways, the result will be fastest path routing.
If you want to prioritize e.g. smaller street, increase the rate on these.

emiltin · 2017-07-13T08:54:02Z

looking at how raster source data is used i'm wondering about this bit:

  local sourceData = sources:query(profile.raster_source, segment.source.lon, segment.source.lat)
  local invalid = sourceData.invalid_data()
  if sourceData.datum ~= invalid then
    -- use value in targetData.datum

wouldn't it be easier if you could just do:

  local sourceDatum = sources:query(profile.raster_source, segment.source.lon, segment.source.lat)
  if sourceDatum ~= nil then
    -- use value in sourceDatum

and maybe we should rename source to something like raster_data? sources seems like a very generic word.

TheMarex · 2017-07-13T09:11:56Z

is it correctly understood that the weight is just the inverse of the rate? and that setting output.weight in way_function is just a shorthand for setting both forward and backward rates?

weight is like the duration value: It is fixed for the whole way. rate on the other hand is like the speed values, it depends on the geometric length on the way. The formula is weight = length / rate (like duration = length / speed).

looking at how raster source data is used i'm wondering about this bit:

I think this is due to the fact that the C++ code will return an integer always. Probably needs a shim in the bindings to make this return nil in lua-land.

and maybe we should rename source to something like raster_data? sources seems like a very generic word.

Hrm yeah what about raster?

emiltin · 2017-07-13T09:20:03Z

makes sense. however i'm confused by your statement that rate depends on the geometric length on the way. isn't it the other way around: weight depends on the length of the way, while rate does not?

.

TheMarex · 2017-07-13T09:23:11Z

@emiltin yes, I meant you need the geometric length to convert rate to the actual value used by OSRM.

emiltin · 2017-07-13T09:24:28Z

alright. if you set the weight, are the rates then ignored?

emiltin · 2017-07-13T09:24:57Z

why is there no forward_weight and backward_weight?

emiltin · 2017-07-13T09:30:11Z

updated docs/profiles.md. might need a few adjustments regarding weight and rate.
also squashed and rebased.

TheMarex · 2017-07-13T11:28:17Z

@emiltin just because we don't have forward_duration and backward_duration. I don't see a problem with adding it, it was just done for consistency I think. If you are up for it, that should be a simple change to ExtractionWay and ExtractorCallbacks::ProcessWay to change it to directional versions.

emiltin · 2017-07-17T11:34:32Z

updated changelog and rebased on master.

TheMarex

Thanks @emiltin for putting in the hard work there. This looks much better both on the code front, as well as on the documentation front. I'm excited to ship this with 5.10.

TheMarex · 2017-07-17T23:00:24Z

docs/profiles.md

+The weight of a way normally computed as length / rate. The weight can be though of as the resistance or cost when passing the way. Routing will prefer ways with low weight.

-As you scroll down the file you'll see local variables, and then local functions, and finally...
+You can also set the weight og a way to a fixed value, In this case it's not calculated based on the length or rate, and the rate is ignored.


typo the weight of a way to a fixed value

TheMarex · 2017-07-17T23:00:43Z

docs/profiles.md

+To handle this, OSRM doesn't simply choose the ways with the highest speed. Intead it uses the concept of `weight` and `rate`. The rate is an abstract meassure that you can assign to  ways as you like to make some ways preferable to others. Routing will prefer ways with high rate.

-Towards the top of the file, a profile (such as [car.lua](../profiles/car.lua)) will typically define various configurations as global variables. A lot of these are look-up hashes of one sort or another.
+The weight of a way normally computed as length / rate. The weight can be though of as the resistance or cost when passing the way. Routing will prefer ways with low weight.


typo can be thought of

emiltin · 2017-07-18T06:28:38Z

i ran the changelog through a spell checker (US english) and corrected a number of errors.

emiltin · 2017-07-18T08:38:14Z

should this wiki page https://github.com/Project-OSRM/osrm-backend/blob/master/docs/profiles.md be updated, or replaced with a reference to the file in the repo?

TheMarex · 2017-07-18T08:54:59Z

@emiltin I think a link to the repo-docs would be best.

emiltin · 2017-07-18T09:31:53Z

ah, the main wiki page already uses a link to the repo file.

Routing on proposed ways was blacklisted only for car profile. This adds the same blacklisting also for foot and bike profiles. It looks like this behavior was added in Project-OSRM/osrm-backend#4258 and then accidentally reverted when pull request with title "profiles api v2" at Project-OSRM/osrm-backend#4072 was merged. This commit restores the original change for bike and foot profiles.

emiltin added Experimental - Do not merge Profile labels May 22, 2017

emiltin force-pushed the refactor/profile_properties branch from fc018a9 to af92624 Compare May 23, 2017 11:03

emiltin force-pushed the refactor/profile_properties branch 3 times, most recently from 10972c4 to f46ec93 Compare May 23, 2017 11:30

emiltin added Review and removed Experimental - Do not merge labels May 23, 2017

oxidase requested changes May 24, 2017

View reviewed changes

oxidase approved these changes May 26, 2017

View reviewed changes

TheMarex requested changes May 26, 2017

View reviewed changes

TheMarex added Review - In feedback and removed Review labels May 29, 2017

emiltin changed the title ~~profiles: move properties into profile table~~ profiles api v2: move properties into profile table Jun 7, 2017

emiltin force-pushed the refactor/profile_properties branch from 58a3ff5 to 6c8b7e4 Compare June 8, 2017 09:22

emiltin removed the Work In Progress label Jul 12, 2017

TheMarex approved these changes Jul 12, 2017

View reviewed changes

TheMarex added the Review - In feedback label Jul 12, 2017

emiltin force-pushed the refactor/profile_properties branch from 2611c38 to b27d45f Compare July 13, 2017 09:29

emiltin force-pushed the refactor/profile_properties branch from 40fa316 to 13a1769 Compare July 17, 2017 11:34

TheMarex approved these changes Jul 17, 2017

View reviewed changes

profiles api v2

3dd01cc

emiltin force-pushed the refactor/profile_properties branch from 13a1769 to 3dd01cc Compare July 18, 2017 06:27

emiltin mentioned this pull request Jul 18, 2017

Process relevant nodes only #4240

Closed

TheMarex merged commit e413b25 into master Jul 18, 2017

MoKob deleted the refactor/profile_properties branch August 4, 2017 09:17

mjjbell mentioned this pull request Oct 11, 2022

OSRM routes through private service road #6397

Open

lliehu mentioned this pull request Apr 24, 2023

Proposed ways are included in routing for bicycle and foot profiles #6614

Closed

lliehu mentioned this pull request Jun 24, 2023

Don't route on proposed ways fossgis-routing-server/cbf-routing-profiles#15

Merged

Uh oh!

Conversation

emiltin commented May 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tasklist

Requirements / Relations

Uh oh!

oxidase commented May 22, 2017

Uh oh!

emiltin commented May 23, 2017

Uh oh!

oxidase commented May 23, 2017

Uh oh!

emiltin commented May 23, 2017

Uh oh!

oxidase commented May 23, 2017

Uh oh!

emiltin commented May 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emiltin commented May 23, 2017

Uh oh!

emiltin commented May 23, 2017

Uh oh!

oxidase left a comment

Choose a reason for hiding this comment

Uh oh!

oxidase May 24, 2017

Choose a reason for hiding this comment

Uh oh!

emiltin May 24, 2017

Choose a reason for hiding this comment

Uh oh!

oxidase May 24, 2017

Choose a reason for hiding this comment

Uh oh!

emiltin May 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oxidase May 24, 2017

Choose a reason for hiding this comment

Uh oh!

emiltin commented May 24, 2017

Uh oh!

oxidase commented May 26, 2017

Uh oh!

TheMarex left a comment

Choose a reason for hiding this comment

Uh oh!

oxidase commented May 26, 2017

Uh oh!

emiltin commented May 26, 2017

Uh oh!

emiltin commented Jun 7, 2017

Uh oh!

emiltin commented Jun 7, 2017

Uh oh!

emiltin commented Jun 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheMarex left a comment

Choose a reason for hiding this comment

Uh oh!

emiltin commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emiltin commented Jul 13, 2017

Understanding speed, weight, rate and distance

Uh oh!

emiltin commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheMarex commented Jul 13, 2017

Uh oh!

emiltin commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TheMarex commented Jul 13, 2017

Uh oh!

emiltin commented May 22, 2017 •

edited

Loading

emiltin commented May 23, 2017 •

edited

Loading

emiltin May 24, 2017 •

edited

Loading

emiltin commented Jun 7, 2017 •

edited

Loading

emiltin commented Jul 13, 2017 •

edited

Loading

emiltin commented Jul 13, 2017 •

edited

Loading

emiltin commented Jul 13, 2017 •

edited

Loading

emiltin commented Jul 13, 2017 •

edited

Loading