Use board/aligh cost only for transits

[Bartosz-Kruba]

Summary

Ensure that we do not use transfer priority to apply cost on board/alight events that are not transfers.

Issue

This improve/fixes the issue #4078, but make the logic redundant. Ideally we should just have one way of doing this, not one way for NeTEx and one for GTFS. But, porting this to the "GTFS way" have performance challanges that need to be resolved first - NOT before the Transit model refactoring.

Unit tests

Added new unit test cases for changed logic.

Code style

yest

Documentation

No new documentation added since this is just a bugfix

[t2gran]

You need to remove the cost from the last alighting as well. I think the easies way to do that is to do it when traversing the egress, subtracting the stopVisitCost added for the last stop-arrival. That way performing transfers will be correct. Note, you should not remove the cost if the egress have rides in it.

You must also check the PathBuilder and optimized transfers logic as well. I am not sure if it will cause any problems there. Most likely only the PathBuilder need to account for it, since the optimized-transfer do not look at access/egress, but try to optimize where transfers are done.

[t2gran]

I updated the PR issue description.

[t2gran]

Netex doc: https://enturas.atlassian.net/wiki/spaces/PUBLIC/pages/728727661/stops#StopPlace.1

[Bartosz-Kruba]

You need to remove the cost from the last alighting as well. I think the easies way to do that is to do it when traversing the egress, subtracting the stopVisitCost added for the last stop-arrival. That way performing transfers will be correct. Note, you should not remove the cost if the egress have rides in it.

You must also check the PathBuilder and optimized transfers logic as well. I am not sure if it will cause any problems there. Most likely only the PathBuilder need to account for it, since the optimized-transfer do not look at access/egress, but try to optimize where transfers are done.

@t2gran

1. What do you mean by removing cost from the last alighting? Do I not remove extra transfer cost for all alighting events right now?

@Override
  public int transitArrivalCost(
    int boardCost,
    int alightSlack,
    int transitTime,
    int transitFactorIndex,
    int toStop
  ) {
    int cost =
      boardCost +
      transitFactors.factor(transitFactorIndex) *
      transitTime +
      waitFactor *
      alightSlack;

//    if (stopVisitCost != null) {
//      cost += stopVisitCost[toStop];
//    }

    return cost;
  }

I removed the stopVisitCost logic here so that it will never be included for any alighting events, right? My logic is that we should never add this extra transfer cost on alighting events. The only relevant event when this should happen is boarding. I assume that all boarding events mean that traveller is boarding a new vehicle so for me this is a synonym for a transfer boarding (except for first boarding event). And where in the code should be subtract this extra egress cost? Is it DefaultCostCalculator#costEgress? At the moment we are only adding transferCost there, no stopVisitCost

2. Is it correct that at this moment we include stopVisitCost on guaranteed changes? Will it not mess up the logic here so that those changes are no longer guaranteed because of this extra cost? It happens in DefaultCostCalculator#boardingCostConstrainedTransfer

3. Should probably rename stopVisitCost to more relevant name. Maybe stopTransferCost.

4. Could guide me a little bit on where in OptimizeTransferService and PathBuilder I should look?

5. It feels wrong to me that right now we include those costs during Raptor routing stage but not during OptimizeTransfers stage. I guess it is done that way because of performance issues and we cannot implement it right now until we refactor the model?

[t2gran]

1. What do you mean by removing cost from the last alighting? Do I not remove extra transfer cost for all alighting events right now?

Consider journey: Walk 1m ~ A ~ Trip 1 ~ B ~ Walk 2m ~ C ~ Trip 2 ~ D ~ Walk 2m.

We should add stopVisit cost to alighting at stop B and boarding at stop C, but not to boarding at A and alighting at D.

What I mean, is that you can not know if you want to apply the stopVisit cost, when alighting at a stop - so you must always apply it. Then if that stop is a "egress stop" you must subtract that cost from the cost before adding the path the destination pareto set. I think removing the alight cost in the DefaultCostCalculator#costEgress() is a good place to do it.

3. Should probably rename stopVisitCost to more relevant name. Maybe stopTransferCost.

Agree

4. Could guide me a little bit on where in OptimizeTransferService and PathBuilder I should look?

I checked the code, I think these changes will not affect the OptimizedTransfers and Path builder, because the CostCalculator is used to calculate the cost in both.

5. It feels wrong to me that right now we include those costs during Raptor routing stage but not during OptimizeTransfers stage. I guess it is done that way because of performance issues and we cannot implement it right now until we refactor the model?

They are included in the OptimizeTransfers stage.

There is one reminding problem here though. When we apply stop-to-stop cost, we do not consider if there are a trip-to-trip constraint as well. The effect is that both costs are applied. In GTFS this is refered to as transfer specificity. A more specific constraint (trip-to-trip) take precedence over a more generic one(stop-to-stop). I suggest we ignore it for now, and if we can make the general constraint transfers implementation fast enough we can migrate the NeTEx stop-to-stop into that instead. Note! In GTFS the constraint is specified on a pair of stops, while in NeTEx it is specified on a single stop.

[t2gran]

I do not think the stop transfer cost should be applied if there is a more specific transfer trip-to-trip cost like stay-seated, guaranteed or not-allowed. This might be what is causing problems in the optimized transfers but I will have a look at that - and reply to this PR.

[Bartosz-Kruba]

I do not think the stop transfer cost should be applied if there is a more specific transfer trip-to-trip cost like stay-seated, guaranteed or not-allowed. This might be what is causing problems in the optimized transfers but I will have a look at that - and reply to this PR.

@t2gran

Right now in our dataset we are not using any other transfer priority data other then Station.Weighting. So there's no way that something interfers with those costs. I tested a little bit and it looks like costs are applied in OptimizeTransferService indeed but
we have some other problems.
Here's the issue:

I ran all tests on this branch where we had fixed problems with acces/egress costs.
And here's our router-config and build-config

{
  "routingDefaults": {
        "itineraryFilters": {
          "filterItinerariesWithSameFirstOrLastTrip": true
        },
        "transferSlack": 180,
        "waitReluctance": 0.175,
        "walkReluctance": 5,
        "maxDirectStreetDurationSeconds": 3700
      },
      "transit": {
        "pagingSearchWindowAdjustments": ["24h", "0h"]
        ,
        "stopTransferCost": {
          "DISCOURAGED" : 3000,
          "ALLOWED" : 150,
          "RECOMMENDED" : 60,
          "PREFERRED" : 0
        }
      }
}

{  
	"areaVisibility": true,
	"parentStopLinking": true,
	"platformEntriesLinking": true,
	"osmWayPropertySet": "norway",
	"islandWithoutStopsMaxSize": 5,
	"islandWithStopsMaxSize": 5,
	"dataImportReport": true,
	"maxTransferDistance": 5000,
	"maxStopToShapeSnapDistance": 500,
	"netex" : {
		"sharedFilePattern" : "ST_stops.xml",
		"sharedGroupFilePattern" : "(\\w{2})_shared_data.xml",
		"groupFilePattern" : "(\\w{2})_line.*\\.xml",
		"netexFeedId": "ST"
	}
}

We have a search result which containts two service journeys.

Transfer can either happen on
Station A : recommendedInterchange (Höör Station)
or
Station B : interchangeAllowed (Höör Mejerigatan)

So obviously we want the transfer to happen on Station A.

With OptimizeTransfers off

When we are searching with OptimizeTransferService off, we are getting the change on Station A as expected.

        "stopTransferCost": {
          "DISCOURAGED" : 3000,
          "ALLOWED" : 150,
          "RECOMMENDED" : 60,
          "PREFERRED" : 0
        }

We can also get it to change on Station B if we flip costs on recommended and allowed transfers - so it works as it is supposed to.

        "stopTransferCost": {
          "DISCOURAGED" : 3000,
          "ALLOWED" : 60,
          "RECOMMENDED" : 150,
          "PREFERRED" : 0
        }

With OptimizeTransfers on

When we have OptimzeTransferService switched on we cannot get it to change at the right station. It always prefers Station B. If we set costs on allowed interchange high enough then it will add some extra results with travel to Station A by bus and then walk to the final destination. So it looks like it prefers walking the second part of trip instead of taking transfer on the Station A.

        "stopTransferCost": {
          "DISCOURAGED" : 3000,
          "ALLOWED" : 500,
          "RECOMMENDED" : 60,
          "PREFERRED" : 0
        }

If it's needed I can provide NeTEx, OSM that we are using and exact graphQL queries for the search.

[Bartosz-Kruba]

The issue describe above is a separate problem. It will have its own issue.