Add a multiplier to the matrix #5298

ghoshkaj · 2018-12-08T01:17:02Z

Issue

Route optimization solvers often require custom inputs when feeding in the matrix. This is a simple dumb multiplier of table duration values.

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?

danpaz · 2018-12-08T01:31:47Z

src/engine/plugins/table.cpp

@@ -127,6 +128,12 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms,
                        result_tables_pair.second[table_index] = distance_estimate;
                    }
                }
+                if (params.scale_factor > 1.0 &&
+                    result_tables_pair.first[table_index] != MAXIMAL_EDGE_DURATION)


Hmm should scale_factor also scale the fallback speed result?

Yes fallback speeds should mirror real speeds. Scaling one and not the other is questionable when feeding into a optimzation solver.

danpat · 2018-12-08T04:54:07Z

include/engine/api/table_parameters.hpp

@@ -135,6 +140,9 @@ struct TableParameters : public BaseParameters
        if (fallback_speed < 0)
            return false;

+        if (scale_factor < 1)


Docs say > 0, but code enforces > 1.

I think any value should be allowed - scale_factor=-0.5 is weird, but possible to imagine.

So we shouldn't really enforce anything here?

Yeah - I can’t think of a reason to restrict this, who knows how people might want to use it.

@danpat I'm actually going to clamp to >0. See #5298 (comment) for overflow checks and #5298 (comment) for why doubling the number of overflow checks would trigger computation growth.

danpat · 2018-12-08T04:55:18Z

include/nodejs/node_osrm_support.hpp

+            Nan::ThrowError("scale_factor must be a number");
+            return table_parameters_ptr();
+        }
+        else if (scale_factor->NumberValue() < 1)


Same again - docs say > 0, code enforces >= 1 (not the same as above btw, which does > 1, not >=).

Should allow any number I think, no need to restrict this, it doesn't trigger any computation growth if it's large.

danpat · 2018-12-08T04:56:09Z

src/engine/plugins/table.cpp

@@ -96,15 +96,16 @@ Status TablePlugin::HandleRequest(const RoutingAlgorithmsInterface &algorithms,
    }

    // Scan table for null results - if any exist, replace with distance estimates
-    if (params.fallback_speed > 0)
+    if (params.fallback_speed > 0 || params.scale_factor > 1.0)


Should be != 1

danpat · 2018-12-08T04:59:48Z

src/engine/plugins/table.cpp

+                    result_tables_pair.first[table_index] != MAXIMAL_EDGE_DURATION)
+                {
+                    result_tables_pair.first[table_index] =
+                        result_tables_pair.first[table_index] * (double)params.scale_factor;


Should probably check for overflow here, and clamp the result to some very large number. - if value * scale_factor ends up > MAXIMAL_EDGE_DURATION (i.e. overflows), we should set value to MAXIMAL_EDGE_DURATION-1.

The value of MAXIMAL_EDGE_DURATION is reserved to indicate "noroute", so we have to clamp slightly lower than that.

Something like:

double result = result_tables_pair.first[table_index] * params.scale_factor; if (double > MAXIMAL_EDGE_DURATION) { result_tables_pair.first[table_index] = MAXIMAL_EDGE_DURATION - 1; } else { result_tables_pair.first[table_index] = result; }

Also note - no need to cast params.scale_factor to double - it's already one of those. Also, it's preferred in C++ to use static_cast<double>(thingy) instead of C-style (double)thing casts - the C++ style has better defined behaviour, which makes it easier to debug if something goes wrong.

@danpat after applying a clamp, should we return an error? Or return the raw really large value? For instance in this test case below, the really large unexplained number gets piped through.

Also, we need a clamp for a negative multiplier too, yes?

✔ Given the profile "testbot" ✔ And the partition extra arguments "--small-component-size 1 --max-cell-sizes 2,4,8,16" ✔ Given the query options | scale_factor | 2147483647 | ✔ Given the node map """ a b """ ✔ And the ways | nodes | | ab | ✖ When I request a travel time matrix I should get | | a | b | | a | 0 | 20 | | b | 20 | 0 | Failures: 1) Scenario: Testbot - Travel time matrix of minimal network with scale factor - features/testbot/duration_matrix.feature:611 Step: When I request a travel time matrix I should get - features/testbot/duration_matrix.feature:624 Step Definition: features/step_definitions/distance_matrix.js:10 Message: Tables were not identical: | | a | b | | a | 0 | (-) 20 | | a | 0 | (+) 214748364.6 | | b | (-) 20 | 0 | | b | (+) 214748364.6 | 0 |

Hm I think we should clamp down the scale_factor itself. Try to clamp the result down after an overflow has occured is too late, and to check every element before doing the multiplication to prevent an overflow will be expensive.

This is the kind of check that would happen before every multiplication:

EdgeDuration diff = MAXIMAL_EDGE_DURATION / result_tables_pair.first[table_index]; if (params.scale_factor > diff) { // overflow will happen for std::lround(result_tables_pair.first[table_index] * params.scale_factor) so error } result_tables_pair.first[table_index] = std::lround(result_tables_pair.first[table_index] * params.scale_factor);

I've gone with return the raw really large value.

jcoupey · 2018-12-08T15:43:40Z

Is there a reason to forbid scale_factor values under 1 in table_service.cpp? Being able to shorten durations could also be useful.

ghoshkaj · 2018-12-08T15:49:03Z

@jcoupey there is not a reason. I'm removing the greater than 1 restriction now.

jcoupey · 2018-12-08T17:41:40Z

@ghoshkaj I think you want to keep a parameters.scale_factor > 0 check in table_service.cpp.

ghoshkaj · 2018-12-08T17:52:29Z

@ghoshkaj I think you want to keep a parameters.scale_factor > 0 check in table_service.cpp.

@jcoupey I'm coming to same conclusion as I'm trying to figure out checking for overflows. Doing two multiplications for each overflow is quite expensive and I don't think a negative durations matrix is useful.

So @danpat I'm going with @jcoupey's suggestion of clamping parameters.scale_factor > 0.

ghoshkaj · 2018-12-08T19:33:24Z

Okay I ran a number of requests with 1280 coordinates so 1280*1280 elements. It took exactly 3 seconds. I don't see any significant user of resources in the TableRequest Handler with coming from the division or the rounding.

I've confirmed that the values are being properly scaled.

ghoshkaj · 2018-12-08T19:41:02Z

Without scale_factor the same request takes 2.98 s.

danpat · 2018-12-08T19:52:16Z

src/server/service/table_service.cpp

@@ -61,6 +61,11 @@ std::string getWrongOptionHelp(const engine::api::TableParameters &parameters)
        help = "fallback_speed must be > 0";
    }

+    if (parameters.scale_factor < 0)


This tests for < 0 but the error message says it must be > 0.

The if should probably be <= to match the message.

danpat · 2018-12-09T07:12:50Z

@ghoshkaj So we're failing on coverage.

One more request: add some tests that exercise the failure conditions - scale_factor=0 and scale_factor=-1. This should increase coverage, and verify that our limit rules on the parameter are correct.

* add a multiplier to the matrix * add rounding * remove scale_factor restrictions * clamp for overflow error * update check to match error message * enforce clamping on < 0 and increase test coverage * add an invalid scale_factor value to node tests * increase test coverage * changelog

- Changes from 5.20.0 - Features: - ADDED: all waypoints in responses now contain a distance property between the original coordinate and the snapped location. [Project-OSRM#5255](Project-OSRM#5255) - ADDED: if `fallback_speed` is used, a new structure `fallback_speed_cells` will describe which cells contain estimated values [Project-OSRM#5259](Project-OSRM#5259) - Table: - ADDED: new parameter `scale_factor` which will scale the cell `duration` values by this factor. [Project-OSRM#5298](Project-OSRM#5298) - FIXED: only trigger `scale_factor` code to scan matrix when necessary. [Project-OSRM#5303](Project-OSRM#5303)

- Changes from 5.20.0 - Features: - ADDED: all waypoints in responses now contain a distance property between the original coordinate and the snapped location. [Project-OSRM#5255](Project-OSRM#5255) - ADDED: if `fallback_speed` is used, a new structure `fallback_speed_cells` will describe which cells contain estimated values [Project-OSRM#5259](Project-OSRM#5259) - REMOVED: we no longer publish Node 4 or 6 binary modules (they are still buildable from source) [Project-OSRM#5314](Project-OSRM#5314) - Table: - ADDED: new parameter `scale_factor` which will scale the cell `duration` values by this factor. [Project-OSRM#5298](Project-OSRM#5298) - FIXED: only trigger `scale_factor` code to scan matrix when necessary. [Project-OSRM#5303](Project-OSRM#5303) - FIXED: fix bug in reverse offset calculation that sometimes lead to negative (and other incorrect) values in distance table results [Project-OSRM#5315](Project-OSRM#5315) - Docker: - FIXED: use consistent boost version between build and runtime [Project-OSRM#5311](Project-OSRM#5311) - FIXED: don't override default permissions on /opt [Project-OSRM#5311](Project-OSRM#5311) - Matching: - CHANGED: matching will now consider edges marked with is_startpoint=false, allowing matching over ferries and other previously non-matchable edge types. [Project-OSRM#5297](Project-OSRM#5297) - Profile: - ADDED: Parse `source:maxspeed` and `maxspeed:type` tags to apply maxspeeds and add belgian flanders rural speed limit. [Project-OSRM#5217](Project-OSRM#5217) - CHANGED: Refactor maxspeed parsing to use common library. [Project-OSRM#5144](Project-OSRM#5144)

ghoshkaj force-pushed the ghoshkaj-scale_factor branch from e9390bc to 5a7f05d Compare December 8, 2018 01:28

danpaz reviewed Dec 8, 2018

View reviewed changes

danpat reviewed Dec 8, 2018

View reviewed changes

ghoshkaj force-pushed the ghoshkaj-scale_factor branch from e9a5471 to 8786ad4 Compare December 8, 2018 18:55

danpat reviewed Dec 8, 2018

View reviewed changes

ghoshkaj force-pushed the ghoshkaj-scale_factor branch from b9ccb53 to b805233 Compare December 9, 2018 23:54

danpat approved these changes Dec 10, 2018

View reviewed changes

ghoshkaj added 9 commits December 10, 2018 11:33

add a multiplier to the matrix

ffd5344

add rounding

e3c2427

add rounding

1421758

remove scale_factor restrictions

cbda366

clamp for overflow error

1856bb1

update check to match error message

8873d5b

enforce clamping on < 0 and increase test coverage

742bf21

add an invalid scale_factor value to node tests

95b60b9

increase test coverage

c23461b

ghoshkaj force-pushed the ghoshkaj-scale_factor branch from 8df15e5 to c23461b Compare December 10, 2018 16:33

changelog

f95034b

ghoshkaj merged commit 2e17f30 into master Dec 10, 2018

ghoshkaj deleted the ghoshkaj-scale_factor branch December 10, 2018 18:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a multiplier to the matrix #5298

Add a multiplier to the matrix #5298

ghoshkaj commented Dec 8, 2018 •

edited

Loading

danpaz Dec 8, 2018

ghoshkaj Dec 8, 2018

danpat Dec 8, 2018

ghoshkaj Dec 8, 2018

danpat Dec 8, 2018

ghoshkaj Dec 8, 2018

danpat Dec 8, 2018

danpat Dec 8, 2018

danpat Dec 8, 2018

danpat Dec 8, 2018

ghoshkaj Dec 8, 2018

ghoshkaj Dec 8, 2018

ghoshkaj Dec 8, 2018 •

edited

Loading

ghoshkaj Dec 8, 2018

jcoupey commented Dec 8, 2018 •

edited

Loading

ghoshkaj commented Dec 8, 2018

jcoupey commented Dec 8, 2018

ghoshkaj commented Dec 8, 2018 •

edited

Loading

ghoshkaj commented Dec 8, 2018 •

edited

Loading

ghoshkaj commented Dec 8, 2018

danpat Dec 8, 2018

danpat commented Dec 9, 2018

Add a multiplier to the matrix #5298

Add a multiplier to the matrix #5298

Conversation

ghoshkaj commented Dec 8, 2018 • edited Loading

Issue

Tasklist

Requirements / Relations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghoshkaj Dec 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcoupey commented Dec 8, 2018 • edited Loading

ghoshkaj commented Dec 8, 2018

jcoupey commented Dec 8, 2018

ghoshkaj commented Dec 8, 2018 • edited Loading

ghoshkaj commented Dec 8, 2018 • edited Loading

ghoshkaj commented Dec 8, 2018

Choose a reason for hiding this comment

danpat commented Dec 9, 2018

ghoshkaj commented Dec 8, 2018 •

edited

Loading

ghoshkaj Dec 8, 2018 •

edited

Loading

jcoupey commented Dec 8, 2018 •

edited

Loading

ghoshkaj commented Dec 8, 2018 •

edited

Loading

ghoshkaj commented Dec 8, 2018 •

edited

Loading