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.

Sign up for GitHub

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

Jump to bottom

Not allowed transfers and support for GTFS transfer points #3792

Merged
merged 26 commits into from
Jan 26, 2022
Merged

Not allowed transfers and support for GTFS transfer points #3792

merged 26 commits into from
Jan 26, 2022

Conversation

t2gran
Copy link
Member

@t2gran t2gran commented Dec 17, 2021 •
edited
Loading

Summary

Explain in one or two sentences what this PR achieves.

Issue

Link to or create an issue that describes the relevant feature or bug.
You need not create an issue for small bugfixes and code cleanups, but in that case do describe the problem clearly and completely in the "summary" section above.
In the linked issue (or summary section for smaller PRs) please describe:

  • Motivation (problem or need encountered)
  • How the code works
  • Technical approach and any design considerations or decisions

Remember that the PR will be reviewed by another developer who may not be familiar with your use cases or the code you're modifying. It generally takes much less effort for the author of a PR to explain the background and technical details than for a reviewer to infer or deduce them. PRs may be closed if they or their linked issues do not contain sufficient information for a reviewer to proceed.

Add GitHub keywords to this PR's description, for example:

closes #45

Unit tests

Write a few words on how the new code is tested.

  • Were unit tests added/updated?
  • Was any manual verification done?
  • Any observations on changes to performance?
  • Was the code designed so it is unit testable?
  • Were any tests applied to the smallest appropriate unit?
  • Do all tests pass the continuous integration service?

Code style

Have you followed the suggested code style?

Documentation

  • Have you added documentation in code covering design and rationale behind the code?
  • Were all non-trivial public classes and methods documented with Javadoc?
  • Were any new configuration options added? If so were the tables in the configuration documentation updated?

Changelog

The changelog file
is generated from the pull-request title, make sure the title describe the feature or issue fixed.
To exclude the PR from the changelog add [changelog skip] in the title.

@t2gran t2gran added this to the 2.1 milestone Dec 17, 2021
@t2gran t2gran added New Feature Regression This feature is no longer working. labels Dec 17, 2021
@t2gran t2gran mentioned this pull request Dec 17, 2021
Open
@t2gran
feat: Extend internal model to allow implementing not-allowed transfers.
4a931a5
@t2gran
feat: Extend Raptor with support for not-allowed transfers.
e472a11
@t2gran
feat: Extend Raptor and optimized transfers with support for not-allo…
4f44616 
…wed transfers.
@t2gran
feat: Extend ConstrainedBoardingSearch with support for not-allowed t…
cca1168 
…ransfers.
@t2gran
refactor: Make transfer constraint none null.
ad6c2cc 
Use a constant, REGULAR_TRANSFER instead of null to represent transfer constraint in boarding events. This enable the
constrained transfer search to return a regular transfer and avoid null checking.
@t2gran
test: Add test on ConstrainedBoardingSearch
268b079 
One of the test is disabled, because it fails. I will fix the bug and enable the test in a later commit.
@t2gran
refactor: remove unused method in ConstrainedBoardingSearchStrategy
072731f
@t2gran
refactor: Improve TransferPoints toString()
ddaa114
@t2gran
refactor: Cleanup T2 equals() and add test
2be63f4
@t2gran
refactor: cleanup TripPattern
539bf9a 
When TripTimes are added to the trip, it trip times are also added to the "local list of trips" - witch is a aggregated temporary list, this has of cause no effect.
@t2gran
refactor: StopTransferPriorityMapper renamed from TransferPriorityMapper
29be403 
When TripTimes are added to the trip, it trip times are also added to the "local list of trips" - witch is a aggregated temporary list, this has of cause no effect.
@t2gran
refactor: rename local variable in RangeRaptorWorker
c706ecf
@t2gran
refactor: improve encapsulation in StopIndexForRaptor
0a0cf45
@t2gran t2gran marked this pull request as ready for review December 20, 2021 14:36
@t2gran t2gran requested a review from a team as a code owner December 20, 2021 14:36
leonardehrenfried
leonardehrenfried requested changes Dec 21, 2021
View reviewed changes
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.

I've made a few comments about names.

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Dec 21, 2021 •
edited
Loading

I ran this with the GTFS Flex example feed and there I saw a NPE when starting up OTP:

11:17:28.093 ERROR (OTPMain.java:59) An uncaught error occurred inside OTP: null
java.lang.NullPointerException: null
	at org.opentripplanner.routing.algorithm.raptor.transit.constrainedtransfer.TransferIndexGenerator.findTPoints(TransferIndexGenerator.java:134)
	at org.opentripplanner.routing.algorithm.raptor.transit.constrainedtransfer.TransferIndexGenerator.findTPoints(TransferIndexGenerator.java:101)
	at org.opentripplanner.routing.algorithm.raptor.transit.constrainedtransfer.TransferIndexGenerator.generateTransfers(TransferIndexGenerator.java:53)
	at org.opentripplanner.routing.algorithm.raptor.transit.mappers.TransitLayerMapper.map(TransitLayerMapper.java:84)
	at org.opentripplanner.routing.algorithm.raptor.transit.mappers.TransitLayerMapper.map(TransitLayerMapper.java:56)
	at org.opentripplanner.standalone.server.Router.startup(Router.java:92)
	at org.opentripplanner.standalone.OTPMain.startOTPServer(OTPMain.java:167)
	at org.opentripplanner.standalone.OTPMain.main(OTPMain.java:52)

So it seems that you can have a stop with which doesn't have a pattern.

@t2gran t2gran marked this pull request as draft January 6, 2022 14:38
@t2gran
refactor: Encapsulate StopPattern in TripPattern
594f8f4 
The StopPattern improve the memory footprint allowing multiple
TripPatterns using the same sequence of stops to ref the same
StopPattern.

review: Get approval for API change in DigitransitStopPropertyMapper
@t2gran
feat: Support for GTFS Trip, Route, Stop & Station Transfers
1390922 
- The Transfer report is improved
- OTP serialization version id incremented
@t2gran t2gran marked this pull request as ready for review January 11, 2022 14:02
t2gran and others added 3 commits January 13, 2022 11:23
@t2gran
fix: NPE in TransferIndexGenerator
4950704
@t2gran @leonardehrenfried
Apply suggestions from code review
4e4339a 
Co-authored-by: Leonard Ehrenfried <[email protected]>
@t2gran
refactor: Review
2a08080 
  - refactor: Remove unused code in TransferMapper
  - refactor: Rename applyToAllTrips to appliesToAllTrips
@t2gran t2gran requested a review from hannesj January 13, 2022 14:31
@leonardehrenfried
Copy link
Member

I'm going to turn off the codecov bot.

@leonardehrenfried
Copy link
Member

I've committed the configuration file directly to dev-2.x. If you merge again, the codecov bot will disappear.

@leonardehrenfried
Copy link
Member

leonardehrenfried commented Jan 21, 2022 •
edited
Loading

I tested the not-allowed-transfers part of this PR and it does exactly what it's supposed to do.

Regular Boston feed

Screenshot from 2022-01-21 14-50-50

Modified Boston feed where with a not-allowed-transfer at the previous transfer stop

Screenshot from 2022-01-21 14-50-54

This leads to a transfer to a different stop. 👍

hannesj
hannesj reviewed Jan 25, 2022
View reviewed changes
...in/java/org/opentripplanner/routing/algorithm/raptor/transit/mappers/AccessEgressMapper.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/model/transfer/TransferPoint.java Outdated Show resolved Hide resolved
...pplanner/routing/algorithm/raptor/transit/constrainedtransfer/ConstrainedBoardingSearch.java Outdated Show resolved Hide resolved
...ext/java/org/opentripplanner/ext/vectortiles/layers/stops/DigitransitStopPropertyMapper.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/model/transfer/TransferPoint.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/model/transfer/TransferPoint.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/model/transfer/TransferPoint.java Outdated Show resolved Hide resolved
src/main/java/org/opentripplanner/model/transfer/TransferPoint.java Outdated Show resolved Hide resolved
@t2gran
Merge remote-tracking branch 'otp/dev-2.x' into otp2_not_allowed_tran…
6028793 
…sfers

# Conflicts:
#	src/main/java/org/opentripplanner/routing/stoptimes/StopTimesHelper.java
#	src/test/java/org/opentripplanner/routing/algorithm/mapping/__snapshots__/TransitSnapshotTest.snap
@t2gran
review: Cleanup AccessEgressMapper
244b30e
@t2gran
feat: Abort constrained transfer search after 5 normal boardings found
3e447f4
@t2gran t2gran merged commit 540df18 into opentripplanner:dev-2.x Jan 26, 2022
@t2gran t2gran deleted the otp2_not_allowed_transfers branch January 26, 2022 18:23
t2gran pushed a commit that referenced this pull request Jan 26, 2022
Add changelog entry for #3792 [ci skip]
d0e6587
@t2gran t2gran mentioned this pull request Feb 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonardehrenfried leonardehrenfried leonardehrenfried approved these changes

@hannesj hannesj hannesj approved these changes

@abyrd abyrd Awaiting requested review from abyrd

Assignees
No one assigned
Labels
New Feature Regression This feature is no longer working.
Projects
None yet
Milestone
2.1
Development

Successfully merging this pull request may close these issues.
3 participants
@t2gran @leonardehrenfried @hannesj