Fix validation

r5-java/src/main/java/propertymap/Router.java

@@ -14,6 +14,8 @@ import com.conveyal.r5.api.util.LegMode;
 import com.conveyal.r5.api.util.TransitModes;
 import com.conveyal.r5.kryo.KryoNetworkSerializer;
 import com.conveyal.r5.profile.StreetMode;
+import com.conveyal.r5.streets.StreetLayer;
+import com.conveyal.r5.streets.VertexStore;
 import com.conveyal.gtfs.model.Stop;
 import com.conveyal.r5.transit.TransitLayer;
 import com.conveyal.r5.transit.TransportNetwork;
@@ -34,18 +36,22 @@ import java.util.List;
 /** R5 routing: network loading, spatial filtering, travel time computation. */
 public class Router {
 
-    // Coarse GB bounding box used to tell "a stop that should have linked to a
+    // A stop counts as "should have linked but didn't" only when a walkable street
-    // street but didn't" (inside GB) from stops that legitimately never link
+    // sits within R5's own link radius of it (streetWithinLinkRadius). This is
-    // (continental coach termini, neutralised out-of-area stops).
+    // deliberately NOT a GB bounding box: the OSM extract is England-only while the
-    private static final double UK_MIN_LAT = 49.0;
+    // BODS GTFS is GB-wide, so ~59k Welsh/Scottish stops legitimately have no street
-    private static final double UK_MAX_LAT = 61.0;
+    // nearby and must not be counted as linking failures.
-    private static final double UK_MIN_LON = -9.0;
+    //
-    private static final double UK_MAX_LON = 2.5;
+    // MAX_UNLINKED_STOP_FRACTION bounds the fraction of stops that DO have a nearby
-    // If more than this fraction of transit stops fail to link to the street
+    // street yet still fail to link — corrupt coordinates or a whole mode displaced
-    // network the build is systemically broken (wrong/incomplete OSM extract, or
+    // (the Tottenham Court Road failure mode). It needs per-stop coordinates and the
-    // corrupt coordinates) rather than the usual tiny residue of unroutable
+    // street index, so it only runs on a fresh build.
-    // stops, so fail loudly instead of silently shipping a half-linked network.
     private static final double MAX_UNLINKED_STOP_FRACTION = 0.10;
+    // CATASTROPHIC_UNLINKED_FRACTION bounds the raw unlinked fraction over ALL stops
+    // and runs on every load (fresh or cached). The legitimate England-vs-GB residue
+    // is ~19%; only a grossly wrong or truncated extract (almost nothing links) blows
+    // past this — the one failure a coordinate-less cached load can still catch.
+    private static final double CATASTROPHIC_UNLINKED_FRACTION = 0.50;
 
     private static final int ZOOM = 9; // R5 enforces range 9-12
     private static final int MAX_GRID_CELLS = 4_900_000; // under R5's 5M limit
@@ -183,7 +189,7 @@ public class Router {
         System.err.printf("  Transit: %,d stops, %,d routes, %,d patterns, %,d services%n",
             stops, routes, patterns, services);
 
-        validateStopLinkage(transitLayer);
+        validateStopLinkage(transitLayer, network.streetLayer);
     }
 
     /**
@@ -192,30 +198,39 @@ public class Router {
      * (R5's "unlinked" sentinel); it is then silently unroutable — no access,
      * egress, or distance table — so trains pass through but nobody can board or
      * alight (the Tottenham Court Road failure mode, upstream of the coordinate
-     * fix). A handful of stops legitimately never link (continental coach termini,
+     * fix).
-     * neutralised out-of-area stops); a large fraction signals a systemic break
-     * (wrong OSM extract, or a whole mode's coordinates corrupt) and fails.
      *
-     * streetVertexForStop is persisted in the network cache, so unlinked stops are
+     * The signal for a systemic break is NOT the raw unlinked count: the BODS feed
-     * counted on both fresh and cached loads. Per-stop coordinates come from the
+     * is GB-wide while the OSM extract is England-only, so every Welsh/Scottish stop
-     * transient stopForIndex, which is only populated on a fresh fromDirectory
+     * (~59k, ~19%) is legitimately unlinkable and must be ignored. Instead we count
-     * build; on a cached load coordinates are unavailable and only counts are logged.
+     * only stops that HAVE a walkable street within R5's link radius yet still fail
+     * to link — the coordinates-corrupt / mode-displaced failure mode — and bound
+     * that fraction (MAX_UNLINKED_STOP_FRACTION). A separate, coarser bound on the
+     * raw unlinked fraction (CATASTROPHIC_UNLINKED_FRACTION) still catches a grossly
+     * wrong or truncated extract where almost nothing links.
+     *
+     * streetVertexForStop is persisted in the network cache, so the raw unlinked
+     * count is available on both fresh and cached loads. Per-stop coordinates and the
+     * street spatial index are only present on a fresh fromDirectory build, so the
+     * precise nearby-street check runs only then; a cached load applies just the
+     * catastrophic bound (the network was already checked precisely when first built).
      */
-    private static void validateStopLinkage(TransitLayer transitLayer) {
+    private static void validateStopLinkage(TransitLayer transitLayer, StreetLayer streetLayer) {
         int n = transitLayer.getStopCount();
         // stopForIndex is transient: on a cached (Kryo) load it comes back empty
         // (size 0), not null, so require it to be fully populated (fresh build)
-        // before indexing into it; otherwise fall back to counts-only.
+        // before indexing into it; otherwise fall back to the catastrophic bound.
         boolean haveCoords = transitLayer.stopForIndex != null
             && transitLayer.stopForIndex.size() == n;
-        int unlinked = 0;
+        int unlinked = 0;            // all unlinked stops, wherever they are
-        int unlinkedInGb = 0;
+        int linkableStops = 0;       // stops with a street within link radius (should link)
+        int unlinkedNearStreets = 0; // ... of those, the ones that still failed to link
         List<String> sample = new ArrayList<>();
         for (int i = 0; i < n; i++) {
-            if (transitLayer.streetVertexForStop.get(i) != -1) {
+            boolean linked = transitLayer.streetVertexForStop.get(i) != -1;
-                continue; // linked
+            if (!linked) {
+                unlinked++;
             }
-            unlinked++;
             if (!haveCoords) {
                 continue;
             }
@@ -223,44 +238,85 @@ public class Router {
             if (stop == null) {
                 continue;
             }
-            double lat = stop.stop_lat;
+            // A linked stop necessarily had a street nearby; only probe the index for
-            double lon = stop.stop_lon;
+            // unlinked ones (also far cheaper than probing all ~316k stops).
-            boolean inGb = lat >= UK_MIN_LAT && lat <= UK_MAX_LAT
+            boolean streetNear = linked
-                && lon >= UK_MIN_LON && lon <= UK_MAX_LON;
+                || streetWithinLinkRadius(streetLayer, stop.stop_lat, stop.stop_lon);
-            if (inGb) {
+            if (streetNear) {
-                unlinkedInGb++;
+                linkableStops++;
+            }
+            if (!linked && streetNear) {
+                unlinkedNearStreets++;
                 if (sample.size() < 25) {
                     String id = transitLayer.stopIdForIndex.get(i);
                     String name = transitLayer.stopNames != null && i < transitLayer.stopNames.size()
                         ? transitLayer.stopNames.get(i) : "";
-                    sample.add(String.format("%s '%s' (%.5f, %.5f)", id, name, lat, lon));
+                    sample.add(String.format("%s '%s' (%.5f, %.5f)",
+                        id, name, stop.stop_lat, stop.stop_lon));
                 }
             }
         }
 
-        double fraction = n == 0 ? 0.0 : (double) unlinked / n;
+        double globalFraction = n == 0 ? 0.0 : (double) unlinked / n;
+        double anomalyFraction = (haveCoords && linkableStops > 0)
+            ? (double) unlinkedNearStreets / linkableStops : 0.0;
+
         if (haveCoords) {
             System.err.printf(
-                "  Stop linkage: %,d/%,d stops NOT linked to street network (%.2f%%), %,d inside GB%n",
+                "  Stop linkage: %,d/%,d stops unlinked (%.2f%%); of the %,d with a street "
-                unlinked, n, fraction * 100.0, unlinkedInGb);
+                    + "within %.0f m, %,d failed to link (%.2f%%). Stops with no nearby street "
+                    + "(e.g. Welsh/Scottish stops absent from an England-only OSM extract) are "
+                    + "expected-unlinked and excluded.%n",
+                unlinked, n, globalFraction * 100.0, linkableStops,
+                StreetLayer.LINK_RADIUS_METERS, unlinkedNearStreets, anomalyFraction * 100.0);
             for (String s : sample) {
-                System.err.println("    unlinked (GB): " + s);
+                System.err.println("    unlinked near streets: " + s);
             }
         } else {
             System.err.printf(
-                "  Stop linkage: %,d/%,d stops NOT linked to street network (%.2f%%) "
+                "  Stop linkage: %,d/%,d stops unlinked (%.2f%%) (coords unavailable on cached "
-                    + "(coords unavailable on cached load)%n",
+                    + "load; the precise nearby-street check ran at build time)%n",
-                unlinked, n, fraction * 100.0);
+                unlinked, n, globalFraction * 100.0);
         }
 
-        if (fraction > MAX_UNLINKED_STOP_FRACTION) {
+        // Catastrophic bound, applied on every load: a grossly wrong or truncated
+        // extract leaves the great majority of stops unlinked. The legitimate
+        // England-vs-GB residue (~19%) is well under this.
+        if (globalFraction > CATASTROPHIC_UNLINKED_FRACTION) {
             throw new IllegalStateException(String.format(
-                "R5 linked only %.1f%% of transit stops to the street network "
+                "R5 left %.1f%% of transit stops unlinked (%,d of %,d), past the catastrophic "
-                    + "(%,d of %,d unlinked). This indicates a systemic linking failure "
+                    + "bound of %.0f%%. The OSM extract is likely wrong or truncated (almost "
-                    + "(wrong/incomplete OSM extract, or corrupt stop coordinates); "
+                    + "nothing links); unlinked stops are silently unroutable.",
-                    + "unlinked stops are silently unroutable.",
+                globalFraction * 100.0, unlinked, n, CATASTROPHIC_UNLINKED_FRACTION * 100.0));
-                (1.0 - fraction) * 100.0, unlinked, n));
         }
+
+        // Precise bound, fresh build only: stops that HAD a nearby street but still
+        // didn't link — corrupt coordinates or a displaced mode (the TCR failure mode).
+        if (haveCoords && anomalyFraction > MAX_UNLINKED_STOP_FRACTION) {
+            throw new IllegalStateException(String.format(
+                "R5 failed to link %.1f%% of the transit stops that have a street within %.0f m "
+                    + "(%,d of %,d). This is a systemic linking failure (corrupt stop coordinates, "
+                    + "or a whole mode displaced), not the expected residue of stops outside the "
+                    + "OSM extract; unlinked stops are silently unroutable.",
+                anomalyFraction * 100.0, StreetLayer.LINK_RADIUS_METERS,
+                unlinkedNearStreets, linkableStops));
+        }
+    }
+
+    /**
+     * True if a walkable street edge sits within R5's stop link radius of (lat, lon).
+     * Mirrors the spatial query R5's own linker makes, so "no edge here" means R5
+     * could not have linked the stop either (it lies outside the OSM extract), while
+     * "edge here but the stop is unlinked" is a genuine anomaly. The probe envelope is
+     * built in WGS84 degrees, then converted to the fixed-point space the edge index
+     * uses — findEdgesInEnvelope does no conversion of its own.
+     */
+    private static boolean streetWithinLinkRadius(StreetLayer streetLayer, double lat, double lon) {
+        double degLat = StreetLayer.LINK_RADIUS_METERS / 111_320.0;
+        double cosLat = Math.max(Math.cos(Math.toRadians(lat)), 1e-6);
+        double degLon = StreetLayer.LINK_RADIUS_METERS / (111_320.0 * cosLat);
+        Envelope env = new Envelope(lon - degLon, lon + degLon, lat - degLat, lat + degLat);
+        return !streetLayer.findEdgesInEnvelope(VertexStore.envelopeToFixed(env)).isEmpty();
     }
 
     static void validateTransitServices(TransportNetwork network, LocalDate date) {