fix stop ID vs stop position in pattern conflict

log selected links and return them as info on modification
move stop and trippattern index resolution to SelectedLink
collapse CSV rows for single OD pair into one row

src/main/java/com/conveyal/r5/analyst/cluster/PathResult.java

@@ -10,13 +10,16 @@
 import com.google.common.collect.Multimap;
 import gnu.trove.list.TIntList;
 import gnu.trove.list.array.TIntArrayList;
+import gnu.trove.set.TIntSet;
+import gnu.trove.set.hash.TIntHashSet;
 import org.apache.commons.lang3.ArrayUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.awt.*;
 import java.lang.invoke.MethodHandles;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
 import java.util.Comparator;
 import java.util.List;
@@ -93,38 +96,11 @@ public PathResult(AnalysisWorkerTask task, TransitLayer transitLayer) {
      */
     public void setTarget(int targetIndex, Multimap<PatternSequence, Iteration> patterns) {
 
-        // When selected link analysis is enabled, filter down the PatternSequences to include only those passing
-        // through the selected links.
-        // TODO Maybe selectedLink should be on TransitLayer, and somehow capture the number of filtered iterations.
+        // When selected link analysis is enabled, filter down the PatternSequence-Iteration Multimap to retain only
+        // those keys passing through the selected links.
+        // TODO Maybe selectedLink should be on TransitLayer, and somehow indicate the number of removed iterations.
         if (transitLayer.parentNetwork.selectedLink != null) {
-            final SelectedLink selectedLink = transitLayer.parentNetwork.selectedLink;
-            Multimap<PatternSequence, Iteration> filteredPatterns = HashMultimap.create();
-            for (PatternSequence patternSequence : patterns.keySet()) {
-                // Why do we have some null patterns lists? Walk-only routes with no transit legs?
-                if (patternSequence.patterns == null) {
-                    continue;
-                }
-                boolean retain = false;
-                // Iterate over the three parallel arrays containing TripPattern, board stop, and alight stop indexes.
-                for (int ride = 0; ride < patternSequence.patterns.size(); ride++) {
-                    int pattern = patternSequence.patterns.get(ride);
-                    int board = patternSequence.stopSequence.boardStops.get(ride);
-                    int alight = patternSequence.stopSequence.alightStops.get(ride);
-                    if (selectedLink.includes(pattern, board, alight)) {
-                        retain = true;
-                        // String routeId = transitLayer.tripPatterns.get(pattern).routeId;
-                        // String boardStopName  = transitLayer.stopNames.get(board);
-                        // String alightStopName  = transitLayer.stopNames.get(alight);
-                        // LOG.info("Retaining {} from {} to {}", routeId, boardStopName, alightStopName);
-                        break;
-                    }
-                }
-                if (retain) {
-                    Collection<Iteration> iterations = patterns.get(patternSequence);
-                    filteredPatterns.putAll(patternSequence, iterations);
-                }
-            }
-            patterns = filteredPatterns;
+            patterns = transitLayer.parentNetwork.selectedLink.filterPatterns(patterns);
         }
 
         // The rest of this runs independent of whether a SelectedLink filtered down the patterns-iterations map.
@@ -148,6 +124,35 @@ public ArrayList<String[]>[] summarizeIterations(Stat stat) {
             summary[d] = new ArrayList<>();
             Multimap<RouteSequence, Iteration> iterationMap = iterationsForPathTemplates[d];
             if (iterationMap != null) {
+                // SelectedLink case: collapse all RouteSequences and Iterations for this OD pair into one to simplify.
+                // This could also be done by merging all Iterations under a single RouteSequence with all route IDs.
+                if (transitLayer.parentNetwork.selectedLink != null) {
+                    int nIterations = 0;
+                    TIntSet allRouteIds = new TIntHashSet();
+                    double summedTotalTime = 0;
+                    for (RouteSequence routeSequence: iterationMap.keySet()) {
+                        Collection<Iteration> iterations = iterationMap.get(routeSequence);
+                        nIterations += iterations.size();
+                        allRouteIds.addAll(routeSequence.routes);
+                        summedTotalTime += iterations.stream().mapToInt(i -> i.totalTime).sum();
+                    }
+                    // Many destinations will have no iterations at all passing through the SelectedLink area.
+                    // Skip those to keep the CSV output short.
+                    if (nIterations > 0) {
+                        String[] row = new String[DATA_COLUMNS.length];
+                        Arrays.fill(row, "ALL");
+                        String allRouteIdsPipeSeparated = Arrays.stream(allRouteIds.toArray())
+                                .mapToObj(transitLayer.routes::get)
+                                .map(routeInfo -> routeInfo.route_id)
+                                .collect(Collectors.joining("|"));
+                        row[0] = allRouteIdsPipeSeparated;
+                        row[row.length - 1] = Integer.toString(nIterations);
+                        row[row.length - 2] = String.format("%.1f", summedTotalTime / nIterations / 60d); // Average total time
+                        summary[d].add(row);
+                    }
+                    continue;
+                }
+                // Standard (non SelectedLink) case.
                 for (RouteSequence routeSequence: iterationMap.keySet()) {
                     Collection<Iteration> iterations = iterationMap.get(routeSequence);
                     int nIterations = iterations.size();

src/main/java/com/conveyal/r5/analyst/cluster/SelectedLink.java

@@ -1,22 +1,32 @@
 package com.conveyal.r5.analyst.cluster;
 
+import com.conveyal.r5.analyst.cluster.PathResult.Iteration;
 import com.conveyal.r5.transit.TransitLayer;
 import com.conveyal.r5.transit.TransportNetworkCache;
 import com.conveyal.r5.transit.TripPattern;
+import com.conveyal.r5.transit.path.PatternSequence;
 import com.conveyal.r5.util.TIntIntHashMultimap;
 import com.conveyal.r5.util.TIntIntMultimap;
+import com.google.common.collect.HashMultimap;
+import com.google.common.collect.Multimap;
 import gnu.trove.TIntCollection;
+import gnu.trove.map.TIntObjectMap;
 import gnu.trove.set.TIntSet;
 import org.locationtech.jts.geom.Envelope;
 import org.locationtech.jts.geom.LineString;
 import org.locationtech.jts.geom.Polygon;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
+import java.lang.invoke.MethodHandles;
 import java.util.Arrays;
+import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
 
 import static com.conveyal.r5.common.GeometryUtils.envelopeForCircle;
 import static com.conveyal.r5.common.GeometryUtils.polygonForEnvelope;
+import static com.google.common.base.Preconditions.checkState;
 
 /**
  * For Selected Link Analysis.
@@ -103,45 +113,137 @@
  * I think the only place we can get these bundle scoped feed IDs is from the TransportNetworkConfig JSON file.
  * Perhaps that should be serialized into the TransportNetwork itself (check risk of serializing used Modifications).
  * But in the meantime TNCache has a method to load that configuration.
+ *
+ * Another problem: after realizing that the stop indexes are indexes at the TransitLayer level and not within the
+ * TripPattern, the SelectedLink.hopsInTripPattern was updated to use TransitLayer stop indexes. But this invalidates
+ * the simple range-based comparison hop >= boardStop && hop < alightStop in SelectedLink.includes(). The comparison
+ * doesn't fail hard, but only superficially appears to do anything meaningful.
+ * The lightweight newtype pattern would be really useful here but doesn't exist in Java.
+ * Solutions are either to scan down the stops in the TripPattern (which involves a lot more comparisons and
+ * indirection, and requires access to the TransitLayer) or to change the source data structure so it stores stop
+ * indexes within the TripPattern and defers resolution to TransitLayer indexes until stop name lookup is needed.
+ * This also draws attention to the fact that specifying board and alight locations with TransitLayer stop indexes is
+ * ambiguous because the same stop can appear more than once in a single TripPattern.
+ * Looking at the Path constructor and RaptorState though, it looks like we don't have this information as RaptorStates
+ * are stored in array slots indexed on stop indexes from the TransitLayer (not the TripPattern level ones).
  */
 public class SelectedLink {
 
+    private static final Logger LOG = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
     /**
      * Contains all TripPattern inter-stop hops that pass through the selected link area for fast hash-based lookup.
-     * Keys are the index of a TripPattern in the TransitLayer, and values are the stop indexes in the TransitLayer.
-     * They are coded this way to match how they're coded in PatternSequence and minimize conversions in tight loops.
-     * A hop from stop A to stop B on pattern X is recorded as the mapping X -> A. Note: This is ambiguous if the stop
-     * appears more than once in the pattern, but PatternSequence does not seem to allow otherwise.
+     * Keys are the index of a TripPattern in the TransitLayer, and values are arrays of stop positions within that
+     * TripPattern (NOT the stop index within the TransitLayer). A hop from stop position N to stop position N+1 on
+     * pattern at index X is recorded as the mapping X -> N. There may be several such hops within a single pattern,
+     * thus an array with more than one element, in the case where one or more transit stops on the pattern fall
+     * within the SelectedLink search radius.
      */
-    private final TIntIntMultimap hopsInTripPattern;
+    private final TIntObjectMap<int[]> hopsInTripPattern;
 
-    // FIXME clean up or remove these notes.
-    // Post-process the OneOriginResult to filter paths down to only those passing through the selected links.
-    // The set of routes and stop pairs concerned are precalculated and retained on per regional analysis.
-    // The first thing to do is specify the point of interest on the request. selectedLink: {lat, lon, radiusMeters}
-    // Without precomputing anything ... just do the geometric calculations every time. And memoize the results.
+    /**
+     * The TransitLayer relative to which all TripPattern indexes and stop indexes should be interpreted.
+     * This is the TransitLayer of the TransportNetwork that holds this SelectedLink instance.
+     * It should be treated as strictly read-only.
+     * Applying further scenarios could perhaps cause the two references to diverge, but the information here in the
+     * base TransitLayer should remain fixed and valid for interpreting hopsInTripPattern.
+     */
+    private final TransitLayer transitLayer;
 
-    public SelectedLink(TIntIntMultimap hopsInTripPattern) {
+    public SelectedLink(TransitLayer transitLayer, TIntObjectMap<int[]> hopsInTripPattern) {
+        this.transitLayer = transitLayer;
         this.hopsInTripPattern = hopsInTripPattern;
     }
 
     /**
-     * For a given transit ride from a boardStop to an alightStop on a TripPattern, return whether that ride
-     * passes through this SelectedLink area.
+     * For a given transit ride from a boardStop to an alightStop on a TripPattern, return true if that ride
+     * passes through any of the hops in this SelectedLink area, or false if it's entirely outside the area.
+     * This is complicated by the fact that the boardStop and alightStop are TransitLayer-wide stop indexes,
+     * not indexes of the stop position within the pattern. It is possible (though unlikely) that a board and alight
+     * stop pair could ambiguously refer to more than one sub-segment of the same pattern when one of the stops appears
+     * more than once in the pattern's stop sequence. We find the earliest matching sub-segment in the sequence.
      */
-    public boolean includes (int tripPattern, int boardStop, int alightStop) {
-        TIntCollection hops = hopsInTripPattern.get(tripPattern);
-        if (hops.isEmpty()) {
+    private boolean includes (int pattern, int board, int alight) {
+        int[] hops = hopsInTripPattern.get(pattern);
+        // Short-circuit: bail out early from most comparisons when the trip pattern has no hops in the SelectedLink.
+        if (hops == null) {
             return false;
         }
-        for (int hop : hops.toArray()) {
-            // Hops are identified with the stop index at their beginning so alightStop is exclusive.
-            // (Alighting at a stop does not ride over the hop identified with that stop index.)
-            if (hop >= boardStop && hop < alightStop) {
+        // Less common case: one or more hops in the pattern of this transit leg do fall inside this SelectedLink.
+        // Determine at which positions in the pattern the board and alight stops are located. Begin looking for
+        // the alight position after the board position, imposing order constraints and reducing potential for
+        // ambiguity where stops appear more than once in the same pattern.
+        int boardPos = stopPositionInPattern(pattern, board, 0);
+        int alightPos = stopPositionInPattern(pattern, alight, boardPos + 1);
+        for (int hop : hops) {
+            // Hops are identified with the stop position at their beginning so the alight comparison is exclusive:
+            // a leg alighting at a stop does not ride over the hop identified with that stop position.
+            if (boardPos <= hop && alightPos > hop) {
                 return true;
             }
         }
         return false;
     }
 
+    /**
+     * Translate a stop index within the TransitLayer to a stop position within the TripPattern with the given index.
+     */
+    private int stopPositionInPattern (int patternIndex, int stopIndexInTransitLayer, int startingAtPos) {
+        TripPattern tripPattern = transitLayer.tripPatterns.get(patternIndex);
+        for (int s = startingAtPos; s < tripPattern.stops.length; s++) {
+            if (tripPattern.stops[s] == stopIndexInTransitLayer) {
+                return s;
+            }
+        }
+        String message = String.format("Did not find stop %d in pattern %d", stopIndexInTransitLayer, patternIndex);
+        throw new IllegalArgumentException(message);
+    }
+
+    /**
+     * Check whether the given PatternSequence has at least one transit leg that passes through this SelectedLink area.
+     */
+    private boolean traversedBy (PatternSequence patternSequence) {
+        // Why are some patterns TIntLists null? Are these walk-only routes with no transit legs?
+        if (patternSequence.patterns == null) {
+            return false;
+        }
+        // Iterate over the three parallel arrays containing TripPattern, board stop, and alight stop indexes.
+        for (int ride = 0; ride < patternSequence.patterns.size(); ride++) {
+            int pattern = patternSequence.patterns.get(ride);
+            int board = patternSequence.stopSequence.boardStops.get(ride);
+            int alight = patternSequence.stopSequence.alightStops.get(ride);
+            if (this.includes(pattern, board, alight)) {
+                // logTriple(pattern, board, alight);
+                return true;
+            }
+        }
+        return false;
+    }
+
+    /**
+     * This filters a particular type of Multimap used in PathResult and TravelTimeReducer.recordPathsForTarget().
+     * For a single origin-destination pair, it captures all transit itineraries connecting that origin and destination.
+     * The keys represent sequences of transit rides between specific stops (TripPattern, board stop, alight stop).
+     * The values associated with each key represent individual raptor iterations that used that sequence of rides,
+     * each of which may have a different departure time, wait time, and total travel time. This method returns a
+     * filtered COPY of the supplied Multimap, with all mappings removed for keys that do not pass through this
+     * SelectedLink area. This often yields an empty Multimap, greatly reducing the number of rows in the CSV output.
+     */
+    public Multimap<PatternSequence, Iteration> filterPatterns (Multimap<PatternSequence, Iteration> patterns){
+        Multimap<PatternSequence, Iteration> filteredPatterns = HashMultimap.create();
+        for (PatternSequence patternSequence : patterns.keySet()) {
+            if (this.traversedBy(patternSequence)) {
+                Collection<Iteration> iterations = patterns.get(patternSequence);
+                filteredPatterns.putAll(patternSequence, iterations);
+            }
+        }
+        return filteredPatterns;
+    }
+
+    private void logTriple (int pattern, int boardStop, int alightStop) {
+        String routeId = transitLayer.tripPatterns.get(pattern).routeId;
+        String boardStopName  = transitLayer.stopNames.get(boardStop);
+        String alightStopName  = transitLayer.stopNames.get(alightStop);
+        LOG.info("Route {} from {} to {}", routeId, boardStopName, alightStopName);
+    }
 }