clukas1
commented
2 months ago @munterfi: Thanks for your review, I hope I've addressed your comments well enough. Explaining all the steps in the code is relatively difficult.
Regarding your comment about the QueryConfig. I've spent some time thinking about it and don't think the current or the proposed solution are really ideal.
The reason I wouldn't include it into the QueryConfig is that it is very technical and something a regular user would never set in an user interface. I see some arguments for adding it to the QueryConfig of the raptor package, as the service could dynamically decide which range is appropriate for a request based on distance between start and end stop. Say 15 Minutes for distances < 10 km, 30 min for < 100 km and 1 h for the rest. But this could theoretically also be done by the service in the current implementation using the RaptorConfig reference and updating the raptorRange there before routing.
The main problem is, the problem of the range config value is that is extremely specific to our raptor implementation. So it's not generally applicable for other raptor implementations according to our interface (a problem which also holds true if we ever would implement a McRaptor). And also problematic since the PublicTransitService interface in the service is generally not restricted to only raptor implementations.
If we still keep it in the application config, I would introduce a Strategy pattern, since the check:
if (raptorRange <= 0) {
doRounds(markedStops);
} else {
doRangeRaptor(markedStops);
}could be avoided since it has always the same result for the configured raptor and the corresponding strategy (rangeQuery or simpleQuery) could be set on the raptor router via the constructor.
I honestly don't see a meaningful and easy way to do this.
If we move it to the user specific query config of the service (in this case then not the raptor query config), we could also consider extending the RaptorAlgorithm interface with the methods: earliestArrivalRangeQuery and latestDepartureRangeQuery. Reason behind this idea is that in the paper those are distinct versions of the raptor algorithm and in perspective also a multicriteriaQuery method could be added to the interface. We have the added versions of the arrival and departure cases.
In this case, I'm against extending the interface. I would rather create three raptor implementations (Simple, Range, and MultiCriteria) which all implement the earliest, latest, and isoLine method. And have algorithm specific query configs handled before calling the routing methods. e.g. setRange(x) for range raptor or .setCriteria(y) for multicriteria. Although having a simple implementation probably does not make sense as it would be the range raptor with range 0. Which actually brings us to our current state.
First throw at range raptor. Think it would be interesting to benchmark against a reverse time direction routing request. I.e. for all pareto optimal simple raptor departure requests run reverse arrival requests, this should return the same results as the range raptor and may or may not be faster..