Too small interval in PT transfers

[Octanas]

I’ve been having some problems with the PT routing but have been able to get around them. This one is tricky though.

In this particular case, the departure time of the second transport is just 44 seconds after the arrival time of the first transport, and since the stops aren’t really that close, it would be impossible in a real life scenario to do that transfer on time.

...
{
“type”: “pt”,
“departure_location”: “Carcavelos”,
“geometry”: {…},
“distance”: 0.0,
“feed_id”: “gtfs_0”,
“is_in_same_vehicle_as_previous”: false,
“trip_headsign”: “Cascais - Cais do Sodré Cais do Sodré”,
“travel_time”: 1590000,
“stops”: […],
“trip_id”: “LCP:27028974”,
“route_id”: “LCP:401”,
“arrival_time”: “2020-08-24T15:44:00.000+0000”,
“departure_time”: “2020-08-24T15:17:30.000+0000”
},
{
“type”: “pt”,
“departure_location”: “CORPO SANTO”,
“geometry”: {…},
“distance”: 0.0,
“feed_id”: “gtfs_0”,
“is_in_same_vehicle_as_previous”: false,
“trip_headsign”: “735 - Hosp. Sta. Maria ALAMEDA D. A. HENRIQUES”,
“travel_time”: 213000,
“stops”: […],
“trip_id”: “LCS:26991318-0”,
“route_id”: “LCS:191142”,
“arrival_time”: “2020-08-24T15:47:55.000+0000”,
“departure_time”: “2020-08-24T15:44:22.000+0000”
},
...

The transfers.txt file specifically states a min_transfer_time of 303 seconds for a transfer between those two stops, which is clearly not being respected. Is this a bug?

[Octanas]

I believe I fixed the issue. The transfers.txt parsing is fine, min_transfer_time gets saved in memory. However, when adding the transfers to the departure timeline (insertInboundTransfers function in GtfsReader.java), every transfer object has min_transfer_time value 0. I traced the problem back to getTransfersToStop function in Transfers.java.

In the GTFS data I used, transfers.txt only has from_stop_id, to_stop_id, transfer_type and min_transfer_time info. For that reason, the condition hasNoRouteSpecificArrivalTransferRules(fromStop) is always true and thus the min_transfer_time never gets assigned for the Transfer objects being returned to insertInboundTransfers function in GtfsReader.java.

My solution is just assigning the min_transfer_time in the transfers array (which always has only one item in my case, there is always only 1 transfer between two stops):

if (hasNoRouteSpecificArrivalTransferRules(fromStop)) {
       Transfer myRule = new Transfer();
       myRule.from_stop_id = fromStop;
       myRule.to_stop_id = toStopId;

       // NEW CODE
       
       if(transfers.size() == 1)
            myRule.min_transfer_time = transfers.get(0).min_transfer_time;
       
       // END OF NEW CODE

       result.add(myRule);
}

I can fork these changes to this repo, but first I'd like to know if this doesn't break anything. Is this supposed to be like this or is it something that you missed? What is exactly the purpose of the hasNoRouteSpecificArrivalTransferRules(fromStop) condition?

[michaz]

Investigating..

[michaz]

Ah yes. Wow, this code is wrong.

This was my rushed attempt at a fix for a serious performance problem -- the regular case (no route-specific exceptions) was always rolled out to one rule for every route. That was correct but slow.

Now it's faster but wrong.

There will probably be more wrong cases. I have to rewrite this and test it properly. I think the current structure doesn't work anymore, it will always be one special rule on top of another. (Help appreciated!)

But your solution is clearly less wrong, so I would merge a pull request right away. Thanks a lot!

If you look at TransfersTest:

    public void testTransfersByToRoute() {
        assertTrue(anotherSampleFeed.hasNoRouteSpecificArrivalTransferRules("MUSEUM"), "Transfer model says we don't have route-dependent arrival platform at from-stop");
        assertTrue(anotherSampleFeed.hasNoRouteSpecificDepartureTransferRules("NEXT_TO_MUSEUM"), "Transfer model says we don't have route-dependent departure platform at to-stop");
        List<Transfer> transfersToStop = anotherSampleFeed.getTransfersToStop("NEXT_TO_MUSEUM", null);
        assertEquals(2, transfersToStop.size());
        Transfer transfer = transfersToStop.get(0);
        assertEquals("MUSEUM", transfer.from_stop_id);
        assertEquals("NEXT_TO_MUSEUM", transfer.to_stop_id);
        Assertions.assertNull(transfer.from_route_id);
        Assertions.assertNull(transfer.to_route_id);
        // add condition here

This is where you could add a test that the minimum transfer time is correctly processed -- that transfer should have one, and it's wrong now. With your fix, it should be correct (non-zero).

[Octanas]

Thanks for the feedback, I'll merge the changes ASAP.