From 57fed90f1f216c4bf09c008fb673cf79e2902ffe Mon Sep 17 00:00:00 2001 From: Jay Ravani Date: Thu, 1 Oct 2026 19:59:40 +0200 Subject: [PATCH] fix: let -link-pylovo re-link and report already-linked buildings -link-pylovo batches only buildings without a building_link row, so a run after a small import touches just the new buildings. Two things were wrong with that. When every building was already linked, the run logged "No LOD2 or LOD3 buildings with footprints found", although the buildings and footprints exist. And nothing could re-link after the PyLovo data changed: the link script deletes and re-inserts the rows of its batch, but the Go batching never gave it a linked building, so the only route was truncating building_link by hand. A run that finds every building linked now says so, with the count, and points at the new -relink flag. `-link-pylovo -relink` batches every building, linked or not (RunPyLovoRelink, GetGridBatches with includeLinked), so the script's delete-and-re-insert recomputes the links against the current PyLovo data. -relink on its own is rejected. The script header and the link docs now say which run re-links. Closes #154 --- README.md | 1 + cmd/c2t/main.go | 9 +- docs/code/pylovo-link/index.md | 9 +- internal/flags/flags.go | 2 + internal/process/feature_extraction.go | 77 +++++++++++++---- internal/process/pylovo_grid_batches_test.go | 20 +++-- internal/process/pylovo_link_build_test.go | 82 +++++++++++++++++++ .../link/pylovo/01_build_pylovo_link.sql | 5 +- 8 files changed, 181 insertions(+), 24 deletions(-) diff --git a/README.md b/README.md index 8c185bd..5d47afb 100644 --- a/README.md +++ b/README.md @@ -62,6 +62,7 @@ python -m venv .venv | `-import-data` | Import new 3D city data into an existing database, skipping files already imported | | `-extract-features` | Run the feature extraction pipeline over buildings not yet processed | | `-link-pylovo` | Link 3D buildings to PyLovo res/oth via IoU spatial join (after `-extract-features`) | +| `-relink` | With `-link-pylovo`, also re-link buildings that already have a link, e.g. after the PyLovo data changed | | `-version` / `-v` | Print version and exit | --- diff --git a/cmd/c2t/main.go b/cmd/c2t/main.go index 618812c..d0ede6a 100644 --- a/cmd/c2t/main.go +++ b/cmd/c2t/main.go @@ -23,6 +23,9 @@ func main() { fmt.Printf("%s (commit %s, built %s)\n", version.Version, version.Commit, version.Date) os.Exit(0) } + if f.Relink && !f.LinkPylovo { + utils.Error.Fatal("-relink only applies together with -link-pylovo") + } // Start timing startTime := time.Now() @@ -98,7 +101,11 @@ func main() { if f.LinkPylovo { utils.Info.Println(flagMessages.LinkPylovo.Progress) - if err := process.RunPyLovoLinkBuild(&config, pool); err != nil { + link := process.RunPyLovoLinkBuild + if f.Relink { + link = process.RunPyLovoRelink + } + if err := link(&config, pool); err != nil { utils.Error.Fatalf(flagMessages.LinkPylovo.Error+": %v", err) } utils.Info.Println(flagMessages.LinkPylovo.Success) diff --git a/docs/code/pylovo-link/index.md b/docs/code/pylovo-link/index.md index ee5bd28..8ccd0bf 100644 --- a/docs/code/pylovo-link/index.md +++ b/docs/code/pylovo-link/index.md @@ -54,10 +54,15 @@ flowchart TD [Feature extraction](https://thd-spatial-ai.github.io/city2tabula/installation/setup/) must have run first so that `lod2_building` is populated with footprint geometries. ```bash -# Run PyLovo link +# Link buildings that have no building_link row yet ./c2t -link-pylovo + +# Re-link every building, e.g. after the PyLovo data changed +./c2t -link-pylovo -relink ``` +`-link-pylovo` skips buildings that already have a `building_link` row, so a run after a small import links only the new buildings. When every building is already linked it reports that and changes nothing. `-relink` recomputes the links of all buildings against the current PyLovo data. + --- ## Configuration @@ -164,7 +169,7 @@ DROP ROLE c2t_fdw_reader; ## Output: `city2tabula.building_link` -One row per 3D building that has a footprint geometry and a valid `object_id`. The pipeline is idempotent, so re-running `-link-pylovo` after updated PyLovo data will overwrite existing rows for the affected buildings. +One row per 3D building that has a footprint geometry and a valid `object_id`. Re-running `-link-pylovo` adds rows for new buildings only; `-link-pylovo -relink` overwrites the existing rows (see [Running the link step](#running-the-link-step)). | Column | Type | Description | |---|---|---| diff --git a/internal/flags/flags.go b/internal/flags/flags.go index 1262dec..f42327a 100644 --- a/internal/flags/flags.go +++ b/internal/flags/flags.go @@ -9,6 +9,7 @@ type Flags struct { ResetC2T bool ExtractFeatures bool LinkPylovo bool + Relink bool ShowVersion bool ShowV bool Bbox string @@ -23,6 +24,7 @@ func ParseFlags() *Flags { flag.BoolVar(&f.ResetC2T, "reset-city2tabula", false, "Reset only City2TABULA schemas (preserve CityDB)") flag.BoolVar(&f.ExtractFeatures, "extract-features", false, "Run the feature extraction pipeline over buildings not yet processed. Safe to re-run; already-processed buildings are skipped") flag.BoolVar(&f.LinkPylovo, "link-pylovo", false, "Link 3D buildings to PyLovo res/oth via IoU spatial join (requires -extract-features to have run first)") + flag.BoolVar(&f.Relink, "relink", false, "With -link-pylovo, also re-link buildings that already have a building_link row, e.g. after the PyLovo data changed") flag.BoolVar(&f.ShowVersion, "version", false, "print version and exit") flag.BoolVar(&f.ShowV, "v", false, "print version and exit (shorthand)") flag.StringVar(&f.Bbox, "bbox", "", "Bounding box spatial filter for -import-data, format xmin,ymin,xmax,ymax[,srid] (passed through to citydb-tool)") diff --git a/internal/process/feature_extraction.go b/internal/process/feature_extraction.go index a344d6b..7066439 100644 --- a/internal/process/feature_extraction.go +++ b/internal/process/feature_extraction.go @@ -172,7 +172,20 @@ func EnableCorrectionTriggers(pool *pgxpool.Pool, cfg *config.Config, lodSchema // Buildings are batched by spatial grid cell (default 1 km²) so each batch covers a // compact geographic area. This keeps the PyLovo bounding-box pre-filter tight and // avoids scanning the full PyLovo table for every batch. +// +// Only buildings without a building_link row are linked, so a run after an +// on-request import touches just the new buildings. RunPyLovoRelink re-links all. func RunPyLovoLinkBuild(cfg *config.Config, pool *pgxpool.Pool) error { + return runPyLovoLink(cfg, pool, false) +} + +// RunPyLovoRelink is RunPyLovoLinkBuild over every building, linked or not, so +// existing building_link rows are recomputed against the current PyLovo data. +func RunPyLovoRelink(cfg *config.Config, pool *pgxpool.Pool) error { + return runPyLovoLink(cfg, pool, true) +} + +func runPyLovoLink(cfg *config.Config, pool *pgxpool.Pool, relink bool) error { if err := setupPylovoFDW(context.Background(), pool, cfg); err != nil { return fmt.Errorf("failed to set up PyLovo FDW: %w", err) } @@ -187,28 +200,59 @@ func RunPyLovoLinkBuild(cfg *config.Config, pool *pgxpool.Pool) error { break } } - n, err := runPyLovoLinkForLOD(cfg, pool, lod, remaining) + n, err := runPyLovoLinkForLOD(cfg, pool, lod, remaining, relink) if err != nil { return err } linked += n } + if linked > 0 { + return nil + } - if linked == 0 { - utils.Warn.Println("No LOD2 or LOD3 buildings with footprints found. Nothing to link.") + linkable, err := countLinkableBuildings(pool, cfg) + if err != nil { + return err + } + if linkable > 0 && !relink { + utils.Warn.Printf("All %d buildings with footprints already have a building_link row. Nothing to link; run -link-pylovo -relink to re-link them.", linkable) + return nil } + utils.Warn.Println("No LOD2 or LOD3 buildings with footprints found. Nothing to link.") return nil } -// runPyLovoLinkForLOD links the unlinked buildings of one LOD schema, at most -// buildingLimit of them when it is above 0, and returns how many it batched. -func runPyLovoLinkForLOD(cfg *config.Config, pool *pgxpool.Pool, lod, buildingLimit int) (int, error) { +// countLinkableBuildings counts the LOD2 and LOD3 buildings that have the +// footprint and object_id the link needs, linked or not. +func countLinkableBuildings(pool *pgxpool.Pool, cfg *config.Config) (int, error) { + total := 0 + for _, lod := range []int{2, 3} { + schema, err := lodSchema(cfg, lod) + if err != nil { + return 0, err + } + var n int + q := fmt.Sprintf(`SELECT count(*) FROM %s.%s_building + WHERE building_footprint_geom IS NOT NULL AND object_id IS NOT NULL`, + cfg.DB.Schemas.City2Tabula, schema) + if err := pool.QueryRow(context.Background(), q).Scan(&n); err != nil { + return 0, fmt.Errorf("failed to count linkable LOD%d buildings: %w", lod, err) + } + total += n + } + return total, nil +} + +// runPyLovoLinkForLOD links the buildings of one LOD schema, the unlinked ones only +// unless relink is set, at most buildingLimit of them when it is above 0, and +// returns how many it batched. +func runPyLovoLinkForLOD(cfg *config.Config, pool *pgxpool.Pool, lod, buildingLimit int, relink bool) (int, error) { schema, err := lodSchema(cfg, lod) if err != nil { return 0, err } - batches, err := GetGridBatches(pool, cfg.DB.Schemas.City2Tabula, schema, cfg.City2Tabula.LinkGridSize, buildingLimit) + batches, err := GetGridBatches(pool, cfg.DB.Schemas.City2Tabula, schema, cfg.City2Tabula.LinkGridSize, buildingLimit, relink) if err != nil { return 0, fmt.Errorf("failed to build LOD%d spatial grid batches: %w", lod, err) } @@ -235,17 +279,22 @@ func runPyLovoLinkForLOD(cfg *config.Config, pool *pgxpool.Pool, lod, buildingLi // GetGridBatches divides one LOD schema's buildings into spatial batches using a square grid. // Each returned slice contains the building_feature_ids that fall within one grid cell. -// Buildings with no footprint geometry or no object_id are excluded, as are buildings -// already present in building_link — this is what makes re-running -link-pylovo after -// a small on-request import cheap: only newly imported buildings get re-batched, -// mirroring the excludeProcessedBuildingIDs pattern used by feature extraction. +// Buildings with no footprint geometry or no object_id are excluded, and so are +// buildings already present in building_link unless includeLinked is set. Skipping +// them is what makes re-running -link-pylovo after a small on-request import cheap: +// only newly imported buildings get re-batched, mirroring the +// excludeProcessedBuildingIDs pattern used by feature extraction. // If buildingLimit > 0, at most that many buildings are included in total. // Exported so integration tests (package process_test) can drive it directly. -func GetGridBatches(pool *pgxpool.Pool, c2tSchema, lodSchema string, gridSizeM, buildingLimit int) ([][]int64, error) { +func GetGridBatches(pool *pgxpool.Pool, c2tSchema, lodSchema string, gridSizeM, buildingLimit int, includeLinked bool) ([][]int64, error) { limitClause := "" if buildingLimit > 0 { limitClause = fmt.Sprintf("LIMIT %d", buildingLimit) } + unlinkedClause := "AND bl.object_id IS NULL" + if includeLinked { + unlinkedClause = "" + } // ST_SquareGrid requires PostGIS >= 3.1. // Buildings are grouped by grid cell; cells with no buildings are excluded. @@ -257,7 +306,7 @@ func GetGridBatches(pool *pgxpool.Pool, c2tSchema, lodSchema string, gridSizeM, ON bl.object_id = ab.object_id AND bl.country_code = ab.country_code WHERE ab.building_footprint_geom IS NOT NULL AND ab.object_id IS NOT NULL - AND bl.object_id IS NULL + %s %s ), extent AS ( @@ -274,7 +323,7 @@ func GetGridBatches(pool *pgxpool.Pool, c2tSchema, lodSchema string, gridSizeM, JOIN grid g ON ST_Intersects(b.geom, g.cell) GROUP BY g.cell HAVING count(*) > 0 - `, c2tSchema, lodSchema, c2tSchema, limitClause) + `, c2tSchema, lodSchema, c2tSchema, unlinkedClause, limitClause) rows, err := pool.Query(context.Background(), q, gridSizeM) if err != nil { diff --git a/internal/process/pylovo_grid_batches_test.go b/internal/process/pylovo_grid_batches_test.go index 82fabc4..66bd9a3 100644 --- a/internal/process/pylovo_grid_batches_test.go +++ b/internal/process/pylovo_grid_batches_test.go @@ -71,7 +71,7 @@ func TestGetGridBatches_SingleLargeCellCoversAllEligibleBuildings(t *testing.T) } const hugeGridSizeM = 100_000 // 100km: comfortably covers the whole fixture extent - batches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, hugeGridSizeM, 0) + batches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, hugeGridSizeM, 0, false) if err != nil { t.Fatalf("GetGridBatches: %v", err) } @@ -96,7 +96,7 @@ func TestGetGridBatches_BuildingLimitCapsTotal(t *testing.T) { cfg, _ := setupCorrectionAuditFixture(t) const limit = 10 - batches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, limit) + batches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, limit, false) if err != nil { t.Fatalf("GetGridBatches: %v", err) } @@ -142,7 +142,7 @@ func TestGetGridBatches_ExcludesAlreadyLinkedBuildings(t *testing.T) { t.Fatalf("failed to seed building_link row: %v", err) } - batches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, 0) + batches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, 0, false) if err != nil { t.Fatalf("GetGridBatches: %v", err) } @@ -154,6 +154,16 @@ func TestGetGridBatches_ExcludesAlreadyLinkedBuildings(t *testing.T) { if len(got) != len(want)-1 { t.Errorf("expected %d buildings (all eligible minus the linked one), got %d", len(want)-1, len(got)) } + + // includeLinked is what -link-pylovo -relink uses: the linked building is back. + all, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, 0, true) + if err != nil { + t.Fatalf("GetGridBatches with includeLinked: %v", err) + } + if got := batchUnion(all); !got[linkedFeatureID] || len(got) != len(want) { + t.Errorf("includeLinked: expected all %d eligible buildings including %d, got %d (linked present: %v)", + len(want), linkedFeatureID, len(got), got[linkedFeatureID]) + } } // TestGetGridBatches_SmallerGridProducesMoreCells is a coarse but real correctness @@ -165,12 +175,12 @@ func TestGetGridBatches_SmallerGridProducesMoreCells(t *testing.T) { cfg, _ := setupCorrectionAuditFixture(t) ctx := context.Background() - bigCellBatches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, 0) + bigCellBatches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 100_000, 0, false) if err != nil { t.Fatalf("GetGridBatches (large grid): %v", err) } - smallCellBatches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 50, 0) + smallCellBatches, err := process.GetGridBatches(testPool, cfg.DB.Schemas.City2Tabula, cfg.DB.Schemas.Lod2, 50, 0, false) if err != nil { t.Fatalf("GetGridBatches (small grid): %v", err) } diff --git a/internal/process/pylovo_link_build_test.go b/internal/process/pylovo_link_build_test.go index 098480f..8ad494f 100644 --- a/internal/process/pylovo_link_build_test.go +++ b/internal/process/pylovo_link_build_test.go @@ -6,7 +6,10 @@ import ( "context" "testing" + "bytes" "github.com/thd-spatial-ai/city2tabula/internal/process" + "github.com/thd-spatial-ai/city2tabula/internal/utils" + "strings" ) // pylovoLinkFixtureBuildings picks 3 distinct, real object_ids from the already- @@ -177,3 +180,82 @@ func TestRunPyLovoLinkBuild_PrefersResOverOth(t *testing.T) { t.Errorf("expected res to be preferred over oth when both match, got pylovo_table=%v osm_id=%v", got.pylovoTable, got.osmID) } } + +// captureWarn redirects utils.Warn into a buffer for the rest of the test. +func captureWarn(t *testing.T) *bytes.Buffer { + t.Helper() + var buf bytes.Buffer + prev := utils.Warn.Writer() + utils.Warn.SetOutput(&buf) + t.Cleanup(func() { utils.Warn.SetOutput(prev) }) + return &buf +} + +// TestRunPyLovoLinkBuild_ReportsAlreadyLinked pins the message of a run that +// finds every building already linked: it must say so, not claim the buildings +// have no footprints. +func TestRunPyLovoLinkBuild_ReportsAlreadyLinked(t *testing.T) { + cfg, _ := setupCorrectionAuditFixture(t) + ctx := context.Background() + cfg.DB.Schemas.Pylvo = "public" + cfg.City2Tabula.LinkGridSize = 1000 + seedPylovoTestTables(t, ctx) + + if err := process.RunPyLovoLinkBuild(cfg, testPool); err != nil { + t.Fatalf("first RunPyLovoLinkBuild: %v", err) + } + warn := captureWarn(t) + if err := process.RunPyLovoLinkBuild(cfg, testPool); err != nil { + t.Fatalf("second RunPyLovoLinkBuild: %v", err) + } + if got := warn.String(); !strings.Contains(got, "already have a building_link row") { + t.Errorf("second run should report the buildings as already linked, logged:\n%s", got) + } +} + +// TestRunPyLovoRelink_UpdatesExistingLinks pins the re-link path: after the PyLovo +// row a building matched is replaced, a plain link run leaves the old link alone +// and RunPyLovoRelink replaces it with the new match. +func TestRunPyLovoRelink_UpdatesExistingLinks(t *testing.T) { + cfg, _ := setupCorrectionAuditFixture(t) + ctx := context.Background() + cfg.DB.Schemas.Pylvo = "public" + cfg.City2Tabula.LinkGridSize = 1000 + seedPylovoTestTables(t, ctx) + building, _, _ := pylovoLinkFixtureBuildings(t, ctx) + + insertPylovoRowFromBuilding(t, ctx, "res", "RES-OLD", building) + if err := process.RunPyLovoLinkBuild(cfg, testPool); err != nil { + t.Fatalf("first RunPyLovoLinkBuild: %v", err) + } + if got := readBuildingLink(t, ctx, building); got.osmID == nil || *got.osmID != "RES-OLD" { + t.Fatalf("expected %s linked to RES-OLD after the first run, got %s", building, osmIDString(got.osmID)) + } + + if _, err := testPool.Exec(ctx, `DELETE FROM public.res WHERE osm_id = 'RES-OLD'`); err != nil { + t.Fatalf("remove RES-OLD: %v", err) + } + insertPylovoRowFromBuilding(t, ctx, "res", "RES-NEW", building) + + if err := process.RunPyLovoLinkBuild(cfg, testPool); err != nil { + t.Fatalf("second RunPyLovoLinkBuild: %v", err) + } + if got := readBuildingLink(t, ctx, building); got.osmID == nil || *got.osmID != "RES-OLD" { + t.Errorf("a plain link run must leave the existing link alone, got %s", osmIDString(got.osmID)) + } + + if err := process.RunPyLovoRelink(cfg, testPool); err != nil { + t.Fatalf("RunPyLovoRelink: %v", err) + } + if got := readBuildingLink(t, ctx, building); got.osmID == nil || *got.osmID != "RES-NEW" { + t.Errorf("expected RunPyLovoRelink to re-link %s to RES-NEW, got %s", building, osmIDString(got.osmID)) + } +} + +// osmIDString renders a nullable osm_id for test messages. +func osmIDString(id *string) string { + if id == nil { + return "" + } + return *id +} diff --git a/sql/scripts/link/pylovo/01_build_pylovo_link.sql b/sql/scripts/link/pylovo/01_build_pylovo_link.sql index 4c219a3..5ab1078 100644 --- a/sql/scripts/link/pylovo/01_build_pylovo_link.sql +++ b/sql/scripts/link/pylovo/01_build_pylovo_link.sql @@ -11,8 +11,9 @@ -- Match confidence = intersection area / area of the smaller footprint (IoU proxy). -- Threshold: >= 0.5 → match_type 1 (complete), < 0.5 → no OSM match → match_type 2. -- --- Existing rows for buildings in {building_ids} are deleted and re-inserted so --- this script is safe to re-run after updated PyLovo data. +-- Existing rows for buildings in {building_ids} are deleted and re-inserted. +-- -link-pylovo batches only buildings without a row; -link-pylovo -relink batches +-- every building, which re-links them after the PyLovo data changed. -- -- When {pylovo_schema} is a postgres_fdw foreign schema (PYLOVO_FDW_HOST set), the -- pre-filter must reach PyLovo, which holds every country in one table. The bbox