Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Include empty rail stops in transfers #6208

Merged
merged 7 commits into from
Nov 18, 2024

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Oct 29, 2024

Summary

Include regular transfers with from/to "empty" rail stops (platforms) when using the OTPFeature#ConsiderPatternsForDirectTransfers. It is common for stops to be assign at realtime for rail, if so turning OTPFeature#IncludeEmptyRailStopsInTransfers on will help to avoid dropping transfers witch is needed when the realtime is applied. We assume the set of truly unused rail stops is limited; Hence the performance overhead of adding them all should be limited.

Note! This is only applied to stops witch has vehicleType Rail. If the input feeds does not have a mode set for the stop
this fix will not work! Consider turning off ConsiderPatternsForDirectTransfers instead or fix the feeds.

Issue

This will help for issue #5461, but is not the ideal fix. The real fix is to generate transfers (both ways) for stops used in realtime - especially stops going from zero patterns to having at least one.

Unit tests

This is hard to test, no test exist for the PatternConsideringNearbyStopFinder as is. I have manually tested the case in issue #5461 - and this fix does work for it.

Documentation

✅ The new feature flag is documented.

Changelog

✅ Minor feature

Bumping the serialization version id

Only a new feature flag is added to the model, so there is no need to bump the ser-ver-id.

@t2gran t2gran added Bug Entur Test This is currently being tested at Entur labels Oct 29, 2024
@t2gran t2gran added this to the 2.7 (next release) milestone Oct 29, 2024
@t2gran t2gran requested a review from a team as a code owner October 29, 2024 19:42
Copy link

codecov bot commented Oct 29, 2024

Codecov Report

Attention: Patch coverage is 72.72727% with 6 lines in your changes missing coverage. Please review.

Project coverage is 69.70%. Comparing base (a9a23fd) to head (b3c63f9).
Report is 161 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...earbystops/PatternConsideringNearbyStopFinder.java 70.00% 2 Missing and 1 partial ⚠️
...nner/apis/transmodel/model/stop/StopPlaceType.java 0.00% 1 Missing ⚠️
...opentripplanner/netex/mapping/FlexStopsMapper.java 0.00% 0 Missing and 1 partial ⚠️
...pentripplanner/transit/model/site/RegularStop.java 75.00% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6208      +/-   ##
=============================================
- Coverage      69.75%   69.70%   -0.05%     
- Complexity     17652    17683      +31     
=============================================
  Files           2006     2008       +2     
  Lines          75525    75812     +287     
  Branches        7731     7761      +30     
=============================================
+ Hits           52679    52847     +168     
- Misses         20134    20251     +117     
- Partials        2712     2714       +2     

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

@t2gran t2gran changed the title Assign platform late Include eEmptyRailStopsInTransfers Oct 29, 2024
@t2gran t2gran changed the title Include eEmptyRailStopsInTransfers Include empty rail stops in transfers Oct 29, 2024
@miklcct
Copy link
Contributor

miklcct commented Oct 30, 2024

Note! This is only applied to stops witch has vehicleType Rail. If the input feeds does not have a mode set for the stop
this fix will not work!

I don't think setting this is possible for GTFS feeds. Is it possible to make this work? My deployment returns vehicleMode = null for unused stops.

@optionsome optionsome self-requested a review October 31, 2024 16:00
@t2gran
Copy link
Member Author

t2gran commented Oct 31, 2024

@miklcct For the record, it is possible to set vehicle_type and OTP do support it trough the Google Extensions: https://developers.google.com/transit/gtfs/reference.

This PR is a temporary fix, and we are removing this feature when the underlying bug is fixed.

@miklcct
Copy link
Contributor

miklcct commented Nov 5, 2024

@miklcct For the record, it is possible to set vehicle_type and OTP do support it trough the Google Extensions: https://developers.google.com/transit/gtfs/reference.

This PR is a temporary fix, and we are removing this feature when the underlying bug is fixed.

I have set the vehicle_type in our GTFS and this pull request behaves as expected. Our journey planner is now returning the expected journeys.

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.

A couple of typos but generally this looks good.

Look forward to the more general solution.

Co-authored-by: Leonard Ehrenfried <[email protected]>
@@ -140,7 +140,7 @@ List<RegularStop> findStopsInFlexArea(
List<RegularStop> stops = stopsSpatialIndex
.query(geometry.getEnvelopeInternal())
.stream()
.filter(stop -> flexibleStopTransitMode == stop.getGtfsVehicleType())
.filter(stop -> flexibleStopTransitMode == stop.getVehicleType())
Copy link
Member

Choose a reason for hiding this comment

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

What is this used for? Vehicle type information is not required, I don't know how much we should rely on that information in general.

Copy link
Member

Choose a reason for hiding this comment

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

I just noticed this is in the netex mapping, perhaps it's required there and this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no clue, I just renamed the very confusing method name - as you noticed, it is used for both GTFS and NeTEx so having GTFS as part of the name is wrong.

Copy link
Member

Choose a reason for hiding this comment

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

It's used to show a stop's mode in the API. However, since it's not required we also deduce it from the patterns that visit the stop:

/**
* For each pattern visiting this {@link StopLocation} return its {@link TransitMode}
*/
private Stream<TransitMode> getPatternModesOfStop(StopLocation stop) {
if (stop.getGtfsVehicleType() != null) {
return Stream.of(stop.getGtfsVehicleType());
} else {
return getPatternsForStop(stop).stream().map(TripPattern::getMode);
}
}

…ation/OTPFeature.java

Co-authored-by: Joel Lappalainen <[email protected]>
@leonardehrenfried
Copy link
Member

Can you update the generated documentation?

@t2gran t2gran merged commit 90a6176 into opentripplanner:dev-2.x Nov 18, 2024
5 checks passed
@t2gran t2gran deleted the assign_platform_late branch November 18, 2024 14:03
t2gran pushed a commit that referenced this pull request Nov 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Entur Test This is currently being tested at Entur
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants