Remove configurable car speed and determine it in graph build #5657

optionsome · 2024-02-02T18:10:14Z

Summary

This removes car.speed from router-config.json. The graph wide max car speed is determined during the graph build. Maximum possible car speed can be set in a OSM mapper and that is used as the upper limit for car speeds when reading in OSM ways (previously there was no upper limit during graph build).

Issue

Closes #5656

Unit tests

Added

Documentation

Updated

Changelog

From title

…ristics

codecov · 2024-02-02T18:18:22Z

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (c893a8b) 67.68% compared to head (65cbff9) 67.69%.
Report is 27 commits behind head on dev-2.x.

Files	Patch %	Lines
...ner/standalone/configure/ConstructApplication.java	0.00%	3 Missing ⚠️
...ner/graph_builder/module/osm/OsmModuleBuilder.java	33.33%	2 Missing ⚠️
.../openstreetmap/tagmapping/ConstantSpeedMapper.java	0.00%	2 Missing ⚠️
...n/java/org/opentripplanner/standalone/OTPMain.java	0.00%	2 Missing ⚠️
...pplanner/standalone/configure/LoadApplication.java	0.00%	2 Missing ⚠️
...rvice/StreetLimitationParametersServiceModule.java	0.00%	2 Missing ⚠️
...rg/opentripplanner/graph_builder/GraphBuilder.java	0.00%	1 Missing ⚠️
..._builder/module/configure/GraphBuilderModules.java	0.00%	1 Missing ⚠️
...tripplanner/openstreetmap/tagmapping/UKMapper.java	0.00%	1 Missing ⚠️
...nner/openstreetmap/wayproperty/WayPropertySet.java	96.42%	0 Missing and 1 partial ⚠️
... and 2 more

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5657      +/-   ##
=============================================
+ Coverage      67.68%   67.69%   +0.01%     
- Complexity     16361    16381      +20     
=============================================
  Files           1889     1893       +4     
  Lines          71785    71862      +77     
  Branches        7404     7410       +6     
=============================================
+ Hits           48586    48646      +60     
- Misses         20690    20708      +18     
+ Partials        2509     2508       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/main/java/org/opentripplanner/street/model/StreetLimitationParametersServiceModule.java

src/main/java/org/opentripplanner/openstreetmap/tagmapping/FinlandMapper.java

src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java

src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModuleBuilder.java

src/test/java/org/opentripplanner/graph_builder/module/osm/OsmModuleTest.java

leonardehrenfried

Other than the nullability issue I think this looks good.

t2gran

There is a major problem with the solution to remove the car speed from here and creating a separate service/repository for this popperty just because the source is not the request or the configuration. The root of the problem is lack of separation "use of parameters" and "source of parameters". This PR illustrate some of the problems we get because the lack of separation of these two things.

Choosing between two evals I think letting the parameter stay where it is is the least confusing, I am sorry for the extra work. If you want @optionsome we can look at these together.

For the future we need to solve this issue, but doing the DI first will help. Example:

If the EuclideanRemainingWeightHeuristic was injected, and not created inside the GraphPathFinder, then the maxCarSpeed would not need to be passed into the GrpahPathFinder.

src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java

t2gran · 2024-02-13T15:45:22Z

src/main/java/org/opentripplanner/openstreetmap/tagmapping/GermanyMapper.java

    props.setCarSpeed("highway=motorway", 33.33f); // = 120kph. Varies between 80 - 120 kph depending on road and season.
    props.setCarSpeed("highway=motorway_link", 15); // = 54kph
    props.setCarSpeed("highway=trunk", 27.27f); // 100kph


There are a lot trailing comments on magic numbers in these files.

Our code conventions say comments should be ABOVE the line it comments on, trailing comments are not allowed.

Magic number should be replaced by constants. In this case I would create two classes: OsmMetricConstants and OsmImperialConstants with e.g. public static final float SPEED_80_KPH = 22.22f.

Is is NOT required to fix this in this PR since it existed in the code from before.

Yes I'm aware of this code style practice but didn't want to edit these files too much within this pr.

t2gran · 2024-02-13T15:48:43Z

src/main/java/org/opentripplanner/openstreetmap/wayproperty/WayPropertySet.java

+  public Float defaultCarSpeed;
+  /**
+   * The maximum automobile speed that can be defined through OSM speed limit tagging. Car speed
+   * defaults for different way types can be higher than this.
+   */
+  public Float maxPossibleCarSpeed;
+  /**
+   * The maximum automobile speed that has been used. This can be used in heuristics later on to
+   * determine the minimum travel time.
+   */
+  public float maxUsedCarSpeed = 0f;


public -> private

This whole class is a mess. I wasn't sure if the solution would have been to add getters/setters for all of these fields or continue on the bad path. I guess I could add the getters and setters.

t2gran · 2024-02-13T15:50:50Z

src/main/java/org/opentripplanner/openstreetmap/wayproperty/WayPropertySet.java

+    if (way.hasTag("maxspeed:motorcar")) {
+      speed = getMetersSecondFromSpeed(way.getTag("maxspeed:motorcar"));
+    }


t2gran · 2024-02-13T15:54:01Z

src/main/java/org/opentripplanner/openstreetmap/tagmapping/NorwayMapper.java

+    props.setCarSpeed("highway=residential", 13.89f); // 50 km/h
+    props.setCarSpeed("highway=service", 13.89f); // 50 km/h


Thank you for fixing this. In Norwegian t = timen (hour)

t2gran · 2024-02-13T16:20:42Z

src/main/java/org/opentripplanner/routing/impl/GraphPathFinder.java

  public GraphPathFinder(@Nullable TraverseVisitor<State, Edge> traverseVisitor) {
-    this(traverseVisitor, null);
+    this(traverseVisitor, null, 40f);


40 is a magic number

This is used in a couple of places, I'm not sure where to put this default.

src/main/java/org/opentripplanner/routing/impl/GraphPathFinder.java

t2gran · 2024-02-13T16:28:23Z

src/main/java/org/opentripplanner/street/model/StreetLimitationParameters.java

+@Singleton
+public class StreetLimitationParameters implements Serializable {


@singleton on a Parameters class look a bit strange.

optionsome · 2024-02-16T10:13:35Z

We went through the remaining concerts with @t2gran today. We decided to keep the existing data model/service structure and expand it in the future to include more stuff from the Graph class.

optionsome · 2024-02-22T09:55:11Z

I forgot to test that the constant speed mapper works earlier and noticed a couple of issues that I have now fixed.

Replace determine max car speed during graph build and use it for heu…

cb4a34e

…ristics

optionsome added improvement bump serialization id Add this label if you want the serialization id automatically bumped after merging the PR config change labels Feb 2, 2024

optionsome requested a review from a team as a code owner February 2, 2024 18:10

leonardehrenfried added this to the 2.5 (next release) milestone Feb 5, 2024

leonardehrenfried requested review from leonardehrenfried and t2gran February 6, 2024 09:26

leonardehrenfried marked this pull request as draft February 6, 2024 09:57

optionsome added 5 commits February 8, 2024 14:24

Add service for street limitation parameters

8640e08

Add tests for WayPropertySet

479aa20

Create own package for services

a7733a2

Add tests for service

4e92d83

Clarify comment

567ddaa

optionsome marked this pull request as ready for review February 8, 2024 13:28