Feature#480 maximum speed #740

star-pirate · 2020-06-03T11:18:36Z

Pull Request Checklist

Fixes #480.

Information about the changes

Key functionality added:
Reason for change: See User adjustable speed capping #480

Examples and reasons for differences between live ORS routes and those generated from this pull request

Added the option to compute routes using a maximum speed as a parameter/weighting.

Required changes to app.config (if applicable)

Addition of maximum_speed for the appropriate inclusion of edges in the core.

…mumSpeedWeighting in the development branch.

…arams. Everything seems to work now.

…ighting in ORSGraphhopper and ORSWeightingFactory.

aoles

Thanks for addressing my previous comments.

I've added some further points for your consideration.

Cheers!

CHANGELOG.md

...main/java/org/heigit/ors/routing/graphhopper/extensions/weighting/MaximumSpeedWeighting.java

...routeservice/src/main/java/org/heigit/ors/routing/graphhopper/extensions/ORSGraphHopper.java

openrouteservice/src/main/java/org/heigit/ors/routing/RoutingProfile.java

openrouteservice/src/main/java/org/heigit/ors/routing/RoutingRequest.java

aoles

Thanks a lot for taking your time to improving the code, we are almost there!

First of all please make sure the API tests pass. Currently they fail, most probably because of places where still the old parameter name user_speed is used. Please search for any leftover occurrences and prune them.

Apart from this, I believe the handling of the maximum_speed_lower_bound value set through the config file could still be improved - see the inline comments.

Cheers!

PS: Please have a look at any previously unresolved conversations as well, thanks!

openrouteservice/src/main/java/org/heigit/ors/routing/RoutingRequest.java

...routeservice/src/main/java/org/heigit/ors/routing/graphhopper/extensions/ORSGraphHopper.java

aoles · 2020-06-04T22:05:31Z

...g/heigit/ors/routing/graphhopper/extensions/edgefilters/core/MaximumSpeedCoreEdgeFilter.java

+ */
+
+public class MaximumSpeedCoreEdgeFilter implements EdgeFilter {
+    private double maxSpeed =  ((AppConfig.getGlobal().getServiceParameter("routing.profiles.default_params","maximum_speed_lower_bound")) != null)


The handling of maximum_speed_lower_bound value from the config file still needs some refactoring. Currently there are 3 different places over the code where this value is parsed and if missing the hard-coded value of 80 is used. The default value, even if hard-coded, should appear only once in the whole codebase.

I suggest to use RouteProfileConfiguration class to store the value of maximumSpeedLowerBound. Have a look at the implementation of the maximum_distance parameter, it will be probably quite similar.

This change might require, for example, that maxSpeed value is resolved upstream in ORSGraphHopper from CmdArgs set in RoutingProfile, and passed to MaximumSpeedCoreEdgeFilter as a constructor argument.

Update: please use GraphProcessContext rather than CmdArgs for passing the maximum_speed_lower_bound.

The introduction of maximumSpeedLowerBound has made maxSpeed redundant, so please remove the latter.

aoles · 2020-06-04T22:07:20Z

...main/java/org/heigit/ors/routing/graphhopper/extensions/weighting/MaximumSpeedWeighting.java

+    private final Weighting superWeighting;
+    private final DecimalEncodedValue avSpeedEnc;
+    private boolean calculateWeight;
+    private double maxSpeed =  ((AppConfig.getGlobal().getServiceParameter("routing.profiles.default_params","maximum_speed_lower_bound")) != null)


As mentioned before, use RouteProfileConfiguration as a canonical place to store the dafault. maxSpeed could probably be passed via HintsMap.

Actually, validation of maximum_speed could happen upstream in ORSGraphHopper where MaximumSpeedWeighting is created, so there is no need for maxSpeed parameter here.

Which HintsMap exactly?

Change of plans. After looking into this further I think maximum_speed validation should be handled even before in RoutingProfileManager. It should be safe to assume that once MaximumSpeedWeighting is created the value has been already verified so no need to check again.

So Should I wait for further info?

I don't have any more insights for now. My latest comments reflect how I would proceed with the implementation, however, feel free to look around yourself. Maybe there is an even better and cleaner way of doing this?

Moving the validation of maximum_speed upstream to RoutingProfileManager has rendered both maximumSpeedLowerBound and maxSpeed obsolete. If the program execution arrives at this point it is already clear that maximum_speed is above the threshold, so no need of checking this again, right? Please remove both fields from the class.

openrouteservice/src/main/java/org/heigit/ors/api/requests/routing/RouteRequestHandler.java

aoles

Please make sure that your updates are not breaking the code. In fact, currently the API test for maximum speed does not pass. I leave it to you to figure out what the problem is.

Actually, test driven development is a good way of ensuring software requirements. I've added some additional API tests which cover the following checks:

that maximum_speed value below the configured threshold results in an appropriate error,
maximum_speed_lower_bound setting can be profile-specific,
use with unsupported profile raises an error.

As long as you follow my comments from the review and fix the existing problems the first two tests should pass without the need of implementing anything additional.

The last test where maximum_speed is provided to the cycling profile will probably require some additional logic. Take your time. Don't forget to add appropriate CUSTOM_KEYS indicating the profiles for which maximum_speed parameter is valid to the description in @ApiModelProperty. This is important from the perspective of our auto-generated interactive documentation.

Cheers,
Andrzej

aoles · 2020-06-06T11:55:33Z

...main/java/org/heigit/ors/routing/graphhopper/extensions/weighting/MaximumSpeedWeighting.java

+    private final Weighting superWeighting;
+    private final DecimalEncodedValue avSpeedEnc;
+    private boolean calculateWeight;
+    private double maxSpeed =  ((AppConfig.getGlobal().getServiceParameter("routing.profiles.default_params","maximum_speed_lower_bound")) != null)


Moving the validation of maximum_speed upstream to RoutingProfileManager has rendered both maximumSpeedLowerBound and maxSpeed obsolete. If the program execution arrives at this point it is already clear that maximum_speed is above the threshold, so no need of checking this again, right? Please remove both fields from the class.

aoles · 2020-06-06T12:00:25Z

...g/heigit/ors/routing/graphhopper/extensions/edgefilters/core/MaximumSpeedCoreEdgeFilter.java

+ */
+
+public class MaximumSpeedCoreEdgeFilter implements EdgeFilter {
+    private double maxSpeed =  ((AppConfig.getGlobal().getServiceParameter("routing.profiles.default_params","maximum_speed_lower_bound")) != null)


The introduction of maximumSpeedLowerBound has made maxSpeed redundant, so please remove the latter.

...g/heigit/ors/routing/graphhopper/extensions/edgefilters/core/MaximumSpeedCoreEdgeFilter.java

openrouteservice/src/main/java/org/heigit/ors/routing/RoutingRequest.java

openrouteservice/src/main/java/org/heigit/ors/routing/RoutingProfileManager.java

star-pirate · 2020-06-08T09:45:23Z

Please make sure that your updates are not breaking the code. In fact, currently the API test for maximum speed does not pass. I leave it to you to figure out what the problem is.

Actually, test driven development is a good way of ensuring software requirements. I've added some additional API tests which cover the following checks:
* that `maximum_speed` value below the configured threshold results in an appropriate error,

* `maximum_speed_lower_bound` setting can be profile-specific,

* use with unsupported profile raises an error.
As long as you follow my comments from the review and fix the existing problems the first two tests should pass without the need of implementing anything additional.

The last test where maximum_speed is provided to the cycling profile will probably require some additional logic. Take your time. Don't forget to add appropriate CUSTOM_KEYS indicating the profiles for which maximum_speed parameter is valid to the description in @ApiModelProperty. This is important from the perspective of our auto-generated interactive documentation.

Cheers,
Andrzej

I just checked my last commit and there the test is still passing. Did you change anything in your last commit?

So I should make sure that maximum speed is only called in car-ors and driving-hgv ?

aoles · 2020-06-15T09:34:36Z

I just checked my last commit and there the test is still passing. Did you change anything in your last commit?

I've just double checked that with you latest commit e996df7 ResultTest.testMaximumSpeed fails, so maybe you had some local changes which you forgot to commit?

So I should make sure that maximum speed is only called in car-ors and driving-hgv ?

Yes, as discussed before the API parameter maximum_speed should be valid only for these two driving profiles.

Cheers,
Andrzej

…in profile specific parameter section, added error for use with unsupported profiles.

star-pirate · 2020-06-17T11:27:38Z

I just checked my last commit and there the test is still passing. Did you change anything in your last commit?

I've just double checked that with you latest commit e996df7 ResultTest.testMaximumSpeed fails, so maybe you had some local changes which you forgot to commit?

So I should make sure that maximum speed is only called in car-ors and driving-hgv ?

Yes, as discussed before the API parameter maximum_speed should be valid only for these two driving profiles.

Cheers,
Andrzej

Just updated. Everything should be fine now.

aoles

Looks good now, thanks for your contribution! I've fixed some minor outstanding glitches in my last commits.

Cheers,
Andrzej

star-pirate and others added 30 commits December 24, 2019 14:03

Integrating maximum_speed-weighting in development branch.

1b035b2

Continuing integration.

c57f0b9

Added TODO list in files.

84d6857

Working implementation of the MaximumSpeddCoreEdgeFilter and the Maxi…

97586f8

…mumSpeedWeighting in the development branch.

Changes in RoutingProfile.

0ed73c5

Add max_speed in the app.config file under routing.profiles.default_p…

9a9460f

…arams. Everything seems to work now.

Changes in package names and comment additions.

7750fde

Added correct paths for MaximumSpeedCoreEdgeFilter and MaximumSpeedWe…

8d5b356

…ighting in ORSGraphhopper and ORSWeightingFactory.

Changes in .gitignore

e8661fc

Commit for merge.

0890622

Megre with development.

9919410

Commit for testing purposes.

2a242e3

Changes in MaximumSpeedWeighting.

d97e261

Merge with master.

1b1afbb

Minor changes.

276b4f3

Fixed minor problems.

2187468

Fully working MaximumSpeedWeighting.

b7d4608

Working MaximumSpeedWeighting plus a corresponding filter.

7d45c4f

Passed the tests performed using R.

398b467

Change app.config max_speed to maximum_speed.

bbba0a0

Minor fixes.

483caac

Merge branch 'master' into maximum_speed_feature_development

f6e8596

Fixed some errors.

1a1635b

Merge with master.

4f57113

Removed test variables.

683992b

Merge with master.

1e58369

Revert changes in pom.xml.

50a8ca3

Remove app.config.

d24f98d

Require dynamic preprocessed algorithm when speed limit is provided

bb6fb48

Cleanup and refactoring

eecd1e0

aoles requested changes Jun 4, 2020

View reviewed changes

More reviews.

5d55972

star-pirate requested a review from aoles June 4, 2020 13:03

aoles requested changes Jun 4, 2020

View reviewed changes

star-pirate added 2 commits June 5, 2020 18:12

Even more reviews.

2923f36

Revert pom.xml back to master form.

803ebfe

star-pirate requested a review from aoles June 5, 2020 16:14

star-pirate and others added 2 commits June 5, 2020 18:54

Fixed API test and some obsolete user_speed.

e996df7

Add tests checking the requirements of maximum speed feature

f1e9dbb

aoles requested changes Jun 6, 2020

View reviewed changes

star-pirate added 2 commits June 17, 2020 13:25

Fixed ReultsTest.testMaximumSpeed(), moved maximum_speed_lower_bound …

977c6a3

…in profile specific parameter section, added error for use with unsupported profiles.

Fixed mistake in ResultsTest.

f25fdac

star-pirate requested a review from aoles June 17, 2020 11:27

aoles and others added 6 commits June 20, 2020 13:52

Fix broken API tests

11f2865

Fix MaximumSpeedWeighting and refactor API tests

4d005e8

Merge branch 'master' into feature#480-maximum_speed

10a2b7e

Merge branch 'master' into feature#480-maximum_speed

efc0fbc

Add CUSTOM_KEYS metadata to ApiModelProperty annotation

b97490c

Minor tweak to CUSTOM_KEYS metadata

1406052

aoles approved these changes Jun 22, 2020

View reviewed changes

Merge branch 'master' into feature#480-maximum_speed

b96a110

aoles removed the request for review from HendrikLeuschner June 24, 2020 09:18

takb merged commit b9e94ed into master Jun 24, 2020

takb deleted the feature#480-maximum_speed branch June 24, 2020 10:28

takb added the awaiting release ⏳ label Jun 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature#480 maximum speed #740

Feature#480 maximum speed #740

star-pirate commented Jun 3, 2020 •

edited by aoles

Loading

aoles left a comment

aoles left a comment •

edited

Loading

aoles Jun 4, 2020 •

edited

Loading

aoles Jun 5, 2020

aoles Jun 6, 2020

aoles Jun 4, 2020 •

edited

Loading

star-pirate Jun 5, 2020

aoles Jun 5, 2020

star-pirate Jun 5, 2020

aoles Jun 5, 2020

aoles Jun 6, 2020

aoles left a comment

aoles Jun 6, 2020

aoles Jun 6, 2020

star-pirate commented Jun 8, 2020

aoles commented Jun 15, 2020

star-pirate commented Jun 17, 2020

aoles left a comment

Feature#480 maximum speed #740

Feature#480 maximum speed #740

Conversation

star-pirate commented Jun 3, 2020 • edited by aoles Loading

Pull Request Checklist

Information about the changes

Examples and reasons for differences between live ORS routes and those generated from this pull request

Required changes to app.config (if applicable)

aoles left a comment

Choose a reason for hiding this comment

aoles left a comment • edited Loading

Choose a reason for hiding this comment

aoles Jun 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aoles Jun 4, 2020 • edited Loading

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

aoles left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

star-pirate commented Jun 8, 2020

aoles commented Jun 15, 2020

star-pirate commented Jun 17, 2020

aoles left a comment

Choose a reason for hiding this comment

star-pirate commented Jun 3, 2020 •

edited by aoles

Loading

aoles left a comment •

edited

Loading

aoles Jun 4, 2020 •

edited

Loading

aoles Jun 4, 2020 •

edited

Loading