Skip to content

Conversation

@sylvansson
Copy link
Contributor

Summary:

This PR fixes a bug where we calculate the distance between two stops incorrectly if there's a GeoJSON location between them. We currently fall back to Null Island (0, 0) when we can't get the coordinates of a stop time's stop (in this case, because there's no stop id for a GeoJSON location) which messes up our calculations and can lead to false positives for the fast_travel_between_far_stops notice. See this comment.

We also address false negatives for the fast_travel_between_consecutive_stops notice due to only comparing consecutive entries in stop_times.txt.

I'll write some tests once we're happy with the approach.

Closes #2031

Expected behavior:

  • We don't get a fast_travel_between_far_stops notice when two stops are within a reasonable distance but there's a GeoJSON location between them.
  • We get a fast_travel_between_consecutive_stops notice when two consecutive stops are too far apart and there's a GeoJSON location between them.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Add or update any needed documentation to the repo
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

@sylvansson sylvansson marked this pull request as draft June 21, 2025 03:02
@sylvansson
Copy link
Contributor Author

I see some of the tests are failing. I need to look into that.

@sylvansson sylvansson force-pushed the 2031-fix-distances-between-stops branch from 2191bf8 to 56d34ba Compare July 8, 2025 02:40
Copy link
Contributor Author

@sylvansson sylvansson left a comment

Choose a reason for hiding this comment

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

I think this is ready for review, sorry for the delay.

GtfsTime.fromString("08:02:30")));
assertThat(generateNotices(ImmutableList.of(route), ImmutableList.of(trip), stopTimes, stops))
.containsExactly(
createFastTravelBetweenConsecutiveStopsNotice(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we now skip stops when we can't compute the distance or speed, we sometimes effectively check non-consecutive stops, which feels slightly misleading. If needed I can adjust the logic to only emit a notice for strictly consecutive entries.

cc: @davidgamez @emmambd

@sylvansson sylvansson marked this pull request as ready for review July 18, 2025 03:13
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.

fast_travel_between_far_stops incorrectly calculates distance when Flex location is between two stops

1 participant