naviqore / public-transit-service

Public transit schedule information and connection routing service based on GTFS data and the RAPTOR algorithm.
MIT License
6 stars 1 forks source link

134 api returns empty array when valid trips should exist #144

Closed clukas1 closed 1 month ago

clukas1 commented 1 month ago

Addressing issue raised, now there is more logic to convert gtfs transfers to raptor transfers with clear hierarchies.

In the process of implementation, I've also addressed a bug I found in our old transfer conversion. Where both additional transfers and gtfs transfers were all added (allowing duplicates), which in the end resulted in always using the shortest transfer, regardless where it was defined e062dc8.

clukas1 commented 1 month ago

Thanks, @clukas1, really nice work!

I do have a few style and documentation suggestions.

Please improve the documentation, especially clarifying what "derive" transfers means in the context of parent and child stops. It would be helpful to describe the general idea, similar to what you outlined in the issue response, somewhere in the Javadoc of the method that encapsulates this logic.

Thanks!

Thank you for your review @munterfi!

I've addressed all of your comments, especially re-writing the docstrings. A thing I've noticed while writing, is that we don't cover all potential cases (some parent/sibling transfers) but this might actually also be better this way and this is something the transfer generators are a good alternative for. To clarify some examples:

Stops: (note in this example parent stops also have departures) A with Children A1, A2 B with Children B1, B2 C with Children C1, C2

Defined Transfers: A-A A1-A1 A-B A1-C1

Resulting A1 transfers: A1-A A1-A1 A1-A2 (from A-A) A1-B (from A-B) A1-B1 (from A-B) A1-B2 (from A-B) A1-C1

Not Covered but potentially possible transfers: A1-C (from A1-C1) A1-C2 (from A1-C2)

Hope you agree that adding more sibling parent logic would kind of be over interpreting transfers defined (or not defined) in GTFS.