From 68097386df567d5d6e403fd6afb68a4434f56a05 Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Tue, 14 Jul 2026 16:59:59 +0100 Subject: [PATCH] Fix validation --- r5-java/src/main/java/propertymap/Router.java | 148 ++++++++++++------ 1 file changed, 102 insertions(+), 46 deletions(-) diff --git a/r5-java/src/main/java/propertymap/Router.java b/r5-java/src/main/java/propertymap/Router.java index 4b65616..d2e5acb 100644 --- a/r5-java/src/main/java/propertymap/Router.java +++ b/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 - // street but didn't" (inside GB) from stops that legitimately never link - // (continental coach termini, neutralised out-of-area stops). - private static final double UK_MIN_LAT = 49.0; - private static final double UK_MAX_LAT = 61.0; - private static final double UK_MIN_LON = -9.0; - private static final double UK_MAX_LON = 2.5; - // If more than this fraction of transit stops fail to link to the street - // network the build is systemically broken (wrong/incomplete OSM extract, or - // corrupt coordinates) rather than the usual tiny residue of unroutable - // stops, so fail loudly instead of silently shipping a half-linked network. + // A stop counts as "should have linked but didn't" only when a walkable street + // sits within R5's own link radius of it (streetWithinLinkRadius). This is + // deliberately NOT a GB bounding box: the OSM extract is England-only while the + // BODS GTFS is GB-wide, so ~59k Welsh/Scottish stops legitimately have no street + // nearby and must not be counted as linking failures. + // + // MAX_UNLINKED_STOP_FRACTION bounds the fraction of stops that DO have a nearby + // street yet still fail to link — corrupt coordinates or a whole mode displaced + // (the Tottenham Court Road failure mode). It needs per-stop coordinates and the + // street index, so it only runs on a fresh build. 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, - * neutralised out-of-area stops); a large fraction signals a systemic break - * (wrong OSM extract, or a whole mode's coordinates corrupt) and fails. + * fix). * - * streetVertexForStop is persisted in the network cache, so unlinked stops are - * counted on both fresh and cached loads. Per-stop coordinates come from the - * transient stopForIndex, which is only populated on a fresh fromDirectory - * build; on a cached load coordinates are unavailable and only counts are logged. + * The signal for a systemic break is NOT the raw unlinked count: the BODS feed + * is GB-wide while the OSM extract is England-only, so every Welsh/Scottish stop + * (~59k, ~19%) is legitimately unlinkable and must be ignored. Instead we count + * 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 unlinkedInGb = 0; + int unlinked = 0; // all unlinked stops, wherever they are + 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 sample = new ArrayList<>(); for (int i = 0; i < n; i++) { - if (transitLayer.streetVertexForStop.get(i) != -1) { - continue; // linked + boolean linked = transitLayer.streetVertexForStop.get(i) != -1; + if (!linked) { + unlinked++; } - unlinked++; if (!haveCoords) { continue; } @@ -223,44 +238,85 @@ public class Router { if (stop == null) { continue; } - double lat = stop.stop_lat; - double lon = stop.stop_lon; - boolean inGb = lat >= UK_MIN_LAT && lat <= UK_MAX_LAT - && lon >= UK_MIN_LON && lon <= UK_MAX_LON; - if (inGb) { - unlinkedInGb++; + // A linked stop necessarily had a street nearby; only probe the index for + // unlinked ones (also far cheaper than probing all ~316k stops). + boolean streetNear = linked + || streetWithinLinkRadius(streetLayer, stop.stop_lat, stop.stop_lon); + if (streetNear) { + 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", - unlinked, n, fraction * 100.0, unlinkedInGb); + " Stop linkage: %,d/%,d stops unlinked (%.2f%%); of the %,d with a street " + + "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%%) " - + "(coords unavailable on cached load)%n", - unlinked, n, fraction * 100.0); + " Stop linkage: %,d/%,d stops unlinked (%.2f%%) (coords unavailable on cached " + + "load; the precise nearby-street check ran at build time)%n", + 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 " - + "(%,d of %,d unlinked). This indicates a systemic linking failure " - + "(wrong/incomplete OSM extract, or corrupt stop coordinates); " - + "unlinked stops are silently unroutable.", - (1.0 - fraction) * 100.0, unlinked, n)); + "R5 left %.1f%% of transit stops unlinked (%,d of %,d), past the catastrophic " + + "bound of %.0f%%. The OSM extract is likely wrong or truncated (almost " + + "nothing links); unlinked stops are silently unroutable.", + globalFraction * 100.0, unlinked, n, CATASTROPHIC_UNLINKED_FRACTION * 100.0)); } + + // 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) {