Remove safety normalizer #6782

miklcct · 2025-08-06T09:40:50Z

Summary

This removes the safety normalizer. The normalizer class still exists but it now just collects the minimum safety values instead of changing the edge.

The heuristic function is also updated to take the reluctance and minimum safety into account, so it will still be admissible even with a low reluctance (e.g. if the user sets walk reluctance to 0.6).

As mentioned in #4442,

We might need to update all the safety values in way property set sources because the values would now be so different without the normalization
At least cycling reluctance values used by client might have to be changed to have the same reluctance in practice

Our client app has the ability to submit custom reluctance values, and explained to the users what it is supposed to do. For example, our app has the following explanation:

The safety normalizer has resulted in cyclists complaining because it shows the cycling journeys much more stressful then they thought, even if it is on a safe route.

Issue

Fixes #6775

Unit tests

Added extensively

Documentation

Changelog

Bumping the serialization version id

Needed. The minimum safety values for walk and cycle are now stored in the graph.

…nd safety

codecov · 2025-08-06T09:47:18Z

Codecov Report

❌ Patch coverage is 79.72973% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.18%. Comparing base (7d31996) to head (9b738fd).
⚠️ Report is 35 commits behind head on dev-2.x.

Files with missing lines	Patch %	Lines
...r/graph_builder/module/osm/SafetyValueApplier.java	72.72%	9 Missing and 6 partials ⚠️
...ch/strategy/EuclideanRemainingWeightHeuristic.java	78.00%	9 Missing and 2 partials ⚠️
...anner/street/model/StreetLimitationParameters.java	66.66%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #6782      +/-   ##
=============================================
+ Coverage      72.15%   72.18%   +0.02%     
- Complexity     19782    19848      +66     
=============================================
  Files           2151     2156       +5     
  Lines          79981    80097     +116     
  Branches        8061     8075      +14     
=============================================
+ Hits           57713    57816     +103     
- Misses         19422    19432      +10     
- Partials        2846     2849       +3

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

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

# Conflicts: # application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap

optionsome

Removing the safety normalizer in practice reduces the cost of cycling a bit. This effect could obviously be balanced by manually updating the bicycle safety factor values to be slightly higher, but I'm not sure if we want to do that. We could also adjust the default cycling reluctance to be higher but that would only affect those who use the default.

...cation/src/main/java/org/opentripplanner/graph_builder/module/osm/SafetyValueNormalizer.java

optionsome · 2025-08-29T11:33:41Z

...cation/src/main/java/org/opentripplanner/graph_builder/module/osm/SafetyValueNormalizer.java

+ * Further reading: <a href="https://github.com/opentripplanner/OpenTripPlanner/issues/4442">Issue 4442</a>
+ * <a href="https://github.com/opentripplanner/OpenTripPlanner/issues/6775">Issue 6775</a>
 */
 class SafetyValueNormalizer {


Maybe we should rename this as this has now been repurposed. I tried to about a name for this but couldn't come up with one quickly. You can try to come up with a new name or we can decide in a dev meeting.

Maybe we should call this SafetyValueApplier?

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

optionsome · 2025-08-29T11:43:54Z

.../main/java/org/opentripplanner/street/search/strategy/EuclideanRemainingWeightHeuristic.java

-  public EuclideanRemainingWeightHeuristic(Float maxCarSpeed) {
-    this.maxCarSpeed = maxCarSpeed != null ? maxCarSpeed : DEFAULT_MAX_CAR_SPEED;
+  public EuclideanRemainingWeightHeuristic(
+    StreetLimitationParametersService streetLimitationParametersService


I'm not sure if we should pass in this service or just the three parameters that are relevant here.

.../main/java/org/opentripplanner/street/search/strategy/EuclideanRemainingWeightHeuristic.java

optionsome · 2025-08-29T12:33:13Z

...tion/src/main/java/org/opentripplanner/street/service/StreetLimitationParametersService.java

+  StreetLimitationParametersService DEFAULT = new StreetLimitationParametersService() {
+    @Override
+    public float getMaxCarSpeed() {
+      return DEFAULT_MAX_CAR_SPEED;
+    }
+
+    @Override
+    public float getBestWalkSafety() {
+      return 1.0f;
+    }
+
+    @Override
+    public float getBestBikeSafety() {
+      return 1.0f;
+    }
+  };


This is a bit of an unusual approach for constructing defaults. I'm not sure if we are ok with this, we can discuss it in a dev meeting.

...java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/BikeRentalSnapshotTest.snap

application/src/test/java/org/opentripplanner/street/model/_data/SimpleConcreteEdge.java

…each street

# Conflicts: # application/src/main/java/org/opentripplanner/graph_builder/module/osm/SafetyValueNormalizer.java # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap

abyrd · 2025-09-04T13:01:50Z

Commenting after a little discussion in last Tuesday's developer meeting. This PR and the referenced issue #4442 show that normalization could be removed, and how it could be removed while keeping heuristics admissible. But the question arises of why we would want to remove it, and whether the benefits are significant enough to motivate the surrounding changes. Are the reasons still the same as stated in #4442? Namely:

Difficult to know what the safety values will be, as they look different than those in the property set
Safety values always act as reluctance factor, never as a preference

In that ticket, these are described as a "minor inconvenience" and a decision to not change the normalization or heuristics was documented there. In addition, @t2gran commented that he did not want to remove the normalization and found it simpler to reason about.

It seems that a decision to make these changes would be dependent on overriding that past decision. Is there a reason removing normalization has become more pressing now? I see that there is some issue communicating with end users about the meaning, but might that be resolved by rewording the explanations? I also don't think these reluctance parameters were ever meant to be exposed directly to end users - we should make sure this is an intentional decision.

miklcct · 2025-09-04T13:12:36Z

The reasons stated in #4442 have become a serious problem for us when we are trying to introduce the safety feature to our users. All the bicycle journeys got inflated generalized cost resulting in the backend suggesting taking transport even when the user wants to cycle more, because there are a few ways with exceptionally low safety values such as 0.25 due to some combination of tags not foreseen when the mapper was written, resulting in normal roads forming the majority of the map inflated from 1 to 4.

This normalizing has complicated the matter for both the people making the mapper, and the user needing to set an appropriate value, so it should be removed.

The ability to set a custom reluctance factor on a sliding scale is the whole reason why Aubin exists nowadays.

abyrd · 2025-09-04T13:51:15Z

I see that there is some issue communicating with end users about the meaning, but might that be resolved by rewording the explanations?

To clarify what I mean here:
Your screenshot shows the text "1.8x reluctant means that you prefer 18 minutes travelling on a public transport vehicle to 10 minutes of walking." This phrasing may be hard for end users to interpret. The base units for our generalized cost are seconds of seated in-vehicle transit travel time. So I have always explained a reluctance factor of 1.8 as "you consider walking for ten minutes to be as bad as riding for 18 minutes in a bus".

miklcct · 2025-09-04T13:53:19Z

Yes. I may need to adjust the wording however the intention is the same.

The problem with the normalizer here is that, if there exists even one single road in the map with safety value 0.25, all roads with default safety (i.e. 1.0) suddenly becomes 4.0 in practice, so the system thinks that all roads are dangerous and tells the user to take public transport instead, if the user wants to use a safe route.

abyrd · 2025-09-04T14:09:08Z

All the bicycle journeys got inflated generalized cost resulting in the backend suggesting taking transport even when the user wants to cycle more, because there are a few ways with exceptionally low safety values such as 0.25 due to some combination of tags not foreseen when the mapper was written, resulting in normal roads forming the majority of the map inflated from 1 to 4.

It's been a while since I used any safety factors so I wasn't understanding the problem at first. I now see the problem. Although the reluctance factors are multiplicative factors, so have the same relative effect on street routing independent of the cost of a "normal" road", those renormalized bike safety costs are being directly compared against (and combined with) transit costs that are in uninflated seconds of in-vehicle time.

On some level it seems reasonably correct though. Bicycling on the best, safest bicycle path around is the reference point for a good safe bicycle ride, on par with riding inside a transit vehicle, and in a context where such paths exist, bicycling on normal roads with car traffic really is 4 times worse (in reality possibly much worse than that, a quick look at the literature makes me think a factor of 25 is reasonable). I am a little hesitant about the meaning of sub-1.0 factors for safe bike paths, with 1.0 factors for "normal" roads. Doesn't this imply that riding a bicycle on a car road is just as safe as riding inside a transit vehicle?

Again, it's been a long time since I used these factors so I'm looking into it now to refresh my understanding in the interest of reviewing this PR.

abyrd · 2025-09-04T14:18:57Z

the system thinks that all roads are dangerous and tells the user to take public transport instead, if the user wants to use a safe route.

Well, all roads that are not bicycle paths completely separated from traffic are dangerous to bicyclists. Generally vastly more dangerous than being inside a metal transit vehicle. Do we want the safety model to align with people's (often incorrect) intuitive perception of safety, rather than measurable reality? People underestimate how extremely dangerous being in the same space with moving cars is. In the same way people are afraid of flying but vastly more likely to be killed crossing the street on the way to the airport.

miklcct · 2025-09-04T14:21:32Z

We can further discuss this later - when safety factor is 1.0 and bike reluctance is 1.0, shall we treat cycling on normal roads the same as taking transit (making cycling on safe streets preferable to transit), or cycling on safest streets the same as taking transit (making cycling on normal roads worse than taking transit). I can accept both options and would like the community to decide, as I can easily explain to the user.

There is also a problem with the "safest streets" option which is supposed to exaggerate the safety even more than normal - it never works with the normalizer so in effect it is the same as "safe streets", and I have a follow up PR to fix this problem.

miklcct · 2025-09-06T06:35:14Z

An objective measure of safety factors is the death statistics. For example, if a death (someone getting killed) is 5x more likely on a certain class of road compared to riding transit for the same duration, the safety factor of that class of the road would be 5.

The effect is that, if we choose a safe route, we are minimalising the likelihood of losing lives.

If we want to adopt this, not only we still have to remove the normalizer, but we also need to introduce safety factors for car driving as well (because driving a car is also more deadly than taking transit).

optionsome · 2025-09-23T09:59:05Z

Summary of the discussion in the dev meeting @t2gran:

This pr will lower the safety factors that are in use since they are no longer normalized
- This means that the "total" bicycle reluctance will be lower in practice if we don't do anything
There will be a follow up pr that will attempt to adjust the request bicycle reluctancies, but it is not sure yet if that approach will work (safety value is not used in all bicycle routing requests while reluctance can be)
- Other alternative is to adjust to safety factors in the tag mappers

application/src/main/java/org/opentripplanner/graph_builder/module/osm/SafetyValueApplier.java

optionsome · 2025-09-26T10:53:39Z

...cation/src/main/java/org/opentripplanner/graph_builder/module/osm/SafetyValueNormalizer.java

+ * Further reading: <a href="https://github.com/opentripplanner/OpenTripPlanner/issues/4442">Issue 4442</a>
+ * <a href="https://github.com/opentripplanner/OpenTripPlanner/issues/6775">Issue 6775</a>
 */
 class SafetyValueNormalizer {


Maybe we should call this SafetyValueApplier?

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

optionsome · 2025-09-26T11:31:24Z

.../main/java/org/opentripplanner/street/search/strategy/EuclideanRemainingWeightHeuristic.java

+
+  private double getCyclingCostPerDistance(BikePreferences preferences) {
+    return getCostPerDistance(
+      preferences.speed(),


Shouldn't we also scale the speed based on the triangle preferences? It's a bit complex problem what we should and shouldn't take into account here since there is also the slope factor, and we don't really want to overestimate the minimum cost here.

It will be too complicated to take elevation into the account when calculating the heuristic. It may pose a problem when the journey is totally downhill.

If anyone knows how I can guess the minimum for a downhill value I am happy to implement it.

The speed always affects the cost, it is not relevant to the triangle preference.

Again the original concept was: all configurable cost factors should only ever increase cost. So specifically: adding elevation data should only ever increase cost above the baseline, not decrease it. If you are applying cost-transforms that can decrease cost (yield fractional multiplers in [0, 1)), the minimum factor would need to be found and accounted for in the heuristic. Otherwise the heuristic is just not "admissible" and the algorithm is going to return suboptimal paths. Strictly speaking coasting downhill is easier than rolling on perfectly flat ground, but is it more important to model this "cost rebate" of coasting downhill, or to keep your heuristic admissible? Priorities will need to be chosen.

I think our strategy so fare has been to "keep your heuristic admissible" - I do not want to change the strategy, unless there is a very good reason - preferable analyzed and described in a document or issue summary.

# Conflicts: # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap

# Conflicts: # application/src/main/java/org/opentripplanner/routing/algorithm/raptoradapter/router/street/DirectStreetRouter.java # application/src/main/java/org/opentripplanner/routing/impl/GraphPathFinder.java # application/src/test/java/org/opentripplanner/astar/AStarTest.java # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/BikeRentalSnapshotTest.snap # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap # application/src/test/java/org/opentripplanner/street/integration/BikeRentalTest.java

abyrd · 2025-10-15T14:37:35Z

This was discussed at length in the developer meeting on 2025-10-14. Among those with any opinion on the subject, there is support for merging the changes in this PR. But it would need to be combined with additional changes that would adjust any defaults and hard-coded values, making the sequence of commits a pure refactor with no confusing side-effects from the perspective of people deploying OTP. This PR should not be merged into dev until those follow-up changes have also been thought through and approved. So probably two PRs that will be merged at once.

miklcct · 2025-10-15T14:48:19Z

This was discussed at length in the developer meeting on 2025-10-14. Among those with any opinion on the subject, there is support for merging the changes in this PR. But it would need to be combined with additional changes that would adjust any defaults and hard-coded values, making the sequence of commits a pure refactor with no confusing side-effects from the perspective of people deploying OTP. This PR should not be merged into dev until those follow-up changes have also been thought through and approved. So probably two PRs that will be merged at once.

It is impossible to give an accurate number after fixing this because the effect depends on the OSM used. This is why the normalizer was problematic as loading a smaller or larger OSM can result in the same area behave differently.

Meanwhile I am mentioning #6767 here as it is the PR which changes the hardcoded values.

t2gran · 2025-10-16T09:25:12Z

I will need a migration guide, also it is important that this PR is not blocking us from using dev-2.x for a long time - a few days or a week is ok. But, for us it is important that when this is merged any follow up PRs witch affect the configuratio is also merged relativly soon after. The migration guide must be available before any of these PRs are merged.

miklcct · 2025-10-16T12:18:51Z

I will need a migration guide, also it is important that this PR is not blocking us from using dev-2.x for a long time - a few days or a week is ok. But, for us it is important that when this is merged any follow up PRs witch affect the configuratio is also merged relativly soon after. The migration guide must be available before any of these PRs are merged.

I have added documentation of how the safety values actually work in practice, which now become clear when the normalization is removed (they are applied as a multiplier on top of the reluctance).

My aim is to change the values of the tag mappers in #6767 so that the configurations won't need to be changed for similar effect. In this case, it will create an impression that cycling on most urban roads (e.g. secondary or tertiary) is actually undesirable, and I believe this is what the community wants.

remove safety normalizing and update heuristic to handle reluctance a…

d467f91

…nd safety

This was referenced Aug 6, 2025

Exaggerate safest street routing #6785

Draft

Bicycle access is slow #6781

Open

t2gran added this to the 2.8 (next release) milestone Aug 11, 2025

miklcct mentioned this pull request Aug 22, 2025

Limit bike speed to car speed #6770

Merged

miklcct marked this pull request as ready for review August 22, 2025 14:35

miklcct requested a review from a team as a code owner August 22, 2025 14:35

miklcct marked this pull request as draft August 22, 2025 14:39

Merge branch 'dev-2.x' into remove-safety-normalizer

174700d

# Conflicts: # application/src/main/java/org/opentripplanner/graph_builder/module/osm/OsmModule.java # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap

miklcct marked this pull request as ready for review August 22, 2025 14:57

optionsome self-requested a review August 26, 2025 09:20

optionsome reviewed Aug 29, 2025

View reviewed changes

miklcct added 5 commits September 1, 2025 12:59

change the log level to debug when processing walk / bike safety for …

adf5571

…each street

rename the methods from set to init and add init-once check

d875561

improve weight heuristic

6f57ae8

extract variable

1dd473b

Merge branch 'dev-2.x' into remove-safety-normalizer

9fdf06c

# Conflicts: # application/src/main/java/org/opentripplanner/graph_builder/module/osm/SafetyValueNormalizer.java # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap

optionsome requested a review from abyrd September 2, 2025 09:36

t2gran modified the milestones: 2.8, 2.9 (next release) Sep 10, 2025

optionsome self-requested a review September 23, 2025 10:00

optionsome reviewed Sep 26, 2025

View reviewed changes

Merge branch 'dev-2.x' into remove-safety-normalizer

8fa7336

# Conflicts: # application/src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/ElevationSnapshotTest.snap

miklcct mentioned this pull request Sep 30, 2025

Simplify handling of safety values #6767

Open

miklcct added 3 commits October 1, 2025 11:19

add Javadoc to explain the best safety value

5a12491

rename SafetyValueNormalizer to SafetyValueApplier

9b88a48

extract OsmModule.initStreetLimitationParameters()

920a442

miklcct requested a review from optionsome October 1, 2025 10:27

Merge branch 'dev-2.x' into remove-safety-normalizer

c167514

miklcct force-pushed the remove-safety-normalizer branch 2 times, most recently from 7b5cf8b to ad3842b Compare October 16, 2025 12:51

add documentation on safety

9b738fd

miklcct force-pushed the remove-safety-normalizer branch from ad3842b to 9b738fd Compare October 16, 2025 13:58

Remove safety normalizer #6782

Are you sure you want to change the base?

Remove safety normalizer #6782

Uh oh!

Conversation

miklcct commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue

Unit tests

Documentation

Changelog

Bumping the serialization version id

Uh oh!

codecov bot commented Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

optionsome left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

abyrd commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miklcct commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abyrd commented Sep 4, 2025

Uh oh!

miklcct commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abyrd commented Sep 4, 2025

Uh oh!

abyrd commented Sep 4, 2025

Uh oh!

miklcct commented Sep 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miklcct commented Sep 6, 2025

Uh oh!

optionsome commented Sep 23, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

t2gran Oct 13, 2025 • 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!

abyrd commented Oct 15, 2025

Uh oh!

miklcct commented Oct 15, 2025

Uh oh!

t2gran commented Oct 16, 2025

Uh oh!

miklcct commented Aug 6, 2025 •

edited

Loading

codecov bot commented Aug 6, 2025 •

edited

Loading

abyrd commented Sep 4, 2025 •

edited

Loading

miklcct commented Sep 4, 2025 •

edited

Loading

miklcct commented Sep 4, 2025 •

edited

Loading

miklcct commented Sep 4, 2025 •

edited

Loading

t2gran Oct 13, 2025 •

edited

Loading