Skip to content

Conversation

@tkalvas
Copy link
Contributor

@tkalvas tkalvas commented Feb 6, 2025

Summary

The existing State handling code in searches updates the time (integer, seconds) by calculating the time used by each step, rounding it up to the nearest integer, and then updating the total. In well-mapped indoor areas, the steps are small, and there are many of them. In our example problem spot, there are 42 edges in 284 metres. This ends up making the total 35 seconds too long. Our transfers try to be well designed, and we have a 7 minute window for that transfer. It takes 287 seconds to walk that distance, and a 90 second transferSlack. We cannot afford inaccuracies of the order of half a minute.

This PR changes the unit of calculation to milliseconds in the actual traversal, and still uses whole numbers. The granularity of time elsewhere in OTP still mostly stays as one second, to avoid a massive change.

The symmetry of backward and forward changes is preserved. When the same trip is searched backward and forward, the duration is exactly the same both in milliseconds and when rounded to seconds. When a forward search is made, the rounded end time is taken as a start time of a reverse search, the rounded end time of the reverse search is the same as the start time of the forward search, given that that had a millisecond part of zero.

Unit tests

In a large number of tests trip durations have become slightly shorter. This accounts for the large amount of changes to tests.

Changelog

This will be a significant change, because the durations have been incorrect for a long time, and people will notice.

Bumping the serialization version id

Unnecessary.

@tkalvas tkalvas requested a review from a team as a code owner February 6, 2025 10:17
@tkalvas tkalvas marked this pull request as draft February 6, 2025 10:17
… backwards are always the same length, to the millisecond, and also when rounded to seconds
@codecov
Copy link

codecov bot commented Feb 10, 2025

Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 70.35%. Comparing base (7f5c12c) to head (1cdaca7).
Report is 218 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...entripplanner/street/search/state/StateEditor.java 55.55% 1 Missing and 3 partials ⚠️
...tripplanner/street/model/edge/ElevatorHopEdge.java 50.00% 1 Missing and 1 partial ⚠️
...org/opentripplanner/street/search/state/State.java 92.85% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6436      +/-   ##
=============================================
+ Coverage      69.84%   70.35%   +0.51%     
- Complexity     18124    18176      +52     
=============================================
  Files           2069     2062       -7     
  Lines          77268    76935     -333     
  Branches        7855     7764      -91     
=============================================
+ Hits           53965    54128     +163     
+ Misses         20546    20051     -495     
+ Partials        2757     2756       -1     

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

@tkalvas tkalvas marked this pull request as ready for review February 10, 2025 08:41
@leonardehrenfried leonardehrenfried changed the title change time calculations in State to whole milliseconds to control rounding error Use millisecond precision in A* traversal Feb 10, 2025
int seconds = this.travelTime > 0
? this.travelTime
: (int) (preferences.street().elevator().hopTime() * this.levels);
s1.incrementTimeInMilliseconds(1000L * seconds);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the seconds method here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not fixed. Please use the seconds method

@leonardehrenfried
Copy link
Member

I think this PR will be a bit easier to review if you use the seconds method wherever possible and only millis where required.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is one more call to fix.

child.time_ms += (traversingBackward ? -milliseconds : milliseconds);
}

public void incrementTimeInSeconds(long seconds) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw does this need to be long?

@tkalvas tkalvas merged commit 42e108d into opentripplanner:dev-2.x Feb 17, 2025
6 checks passed
@tkalvas tkalvas deleted the state-time-to-milliseconds branch February 17, 2025 10:00
t2gran pushed a commit that referenced this pull request Feb 17, 2025
@vpaturet vpaturet mentioned this pull request Feb 21, 2025
@t2gran t2gran added this to the 2.7 milestone Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants