From 72cc982aaf0c44ff041a9d9bfed3e5aa5f6656b5 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 11 Nov 2026 20:41:52 +0000 Subject: [PATCH 1/2] fix(conflate): treat stores inside their brand's OSM host as departments An ATP pharmacy, liquor store or other department-category feature whose position lies inside an OSM supermarket (or other host-category store) of its own brand:wikidata is now a department, however far the spider put the ATP supermarket. Before, such a feature was only a department if the ATP host was within 75 m, so six albertsons and safeway pharmacies 77 to 235 m from their ATP supermarket still matched the supermarket's building as `same`. Fixes #930. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Ttfb2HyX6m2N4gcDLAazrJ --- docs/notes/2026-11-09-departments.md | 7 +- src/pipeline/conflate/located_in.rs | 126 +++++++++++++++++++++++++++ src/pipeline/conflate/mod.rs | 78 ++++++++++++++--- 3 files changed, 200 insertions(+), 11 deletions(-) diff --git a/docs/notes/2026-11-09-departments.md b/docs/notes/2026-11-09-departments.md index 66c371c..5cc9836 100644 --- a/docs/notes/2026-11-09-departments.md +++ b/docs/notes/2026-11-09-departments.md @@ -48,7 +48,12 @@ in `src/pipeline/conflate/mod.rs`. (supermarket, hypermarket, department store, furniture, wholesale) is within 75 m. Spiders often put a department at the entrance and the store at its center, so the distance has to allow for large - buildings. + buildings. It is also a department if its position lies inside an + OSM feature of a host category and of its own `brand:wikidata`, + however far the ATP store is. In the production run of 2026-11-10, + six pharmacies of `albertsons` and `safeway` were 77 to 235 m from + their ATP supermarket but inside its OSM building, and matched the + supermarket ([#930](https://github.com/brawer/osmdiffs/issues/930)). 2. **Keep it off the host.** A department doesn't match OSM features in a host category, nor those in another department category: the Publix pharmacy must not take the Publix Liquors either. Café, fast diff --git a/src/pipeline/conflate/located_in.rs b/src/pipeline/conflate/located_in.rs index cadcd1a..e3d6108 100644 --- a/src/pipeline/conflate/located_in.rs +++ b/src/pipeline/conflate/located_in.rs @@ -13,6 +13,10 @@ //! `brand:wikidata` in a [HOST_CATEGORIES] category, such as the //! pharmacy and the liquor store that the `publix_us` spider lists //! next to each Publix supermarket. +//! A department-category ATP feature inside an OSM feature of a +//! [HOST_CATEGORIES] category and of its own `brand:wikidata` is a +//! department too, however far the ATP host is; see +//! [enclosing_host]. //! - An ATP feature with `located_in:wikidata` or `located_in`, which //! some spiders set, such as `starbucks_us` for its cafés in Target //! stores. @@ -261,6 +265,47 @@ pub(super) fn find_departments(reader: &PlaceReader) -> Result Option<(&'static str, Vec)> { + let tags = || place.tags.iter().map(|(k, v)| (k.as_str(), v.as_str())); + let own = main_category(tags()) + .and_then(|c| DEPARTMENT_CATEGORIES.iter().find(|d| **d == c).copied())?; + let brands: Vec = brand_ids(tags()).collect(); + (!brands.is_empty()).then_some((own, brands)) +} + +/// The host of a department-category ATP feature of main category +/// `own` and `brand:wikidata` `brands`, at `point`, if the OSM feature +/// with `tags` is a [HOST_CATEGORIES] store of one of these brands and +/// its `geometry` contains `point`. `geometry` is only called for such +/// a store. Spiders sometimes put the +/// pharmacy of a large supermarket more than [DEPARTMENT_METERS] from +/// the supermarket, but still inside its building; see +/// [brawer/osmdiffs#930](https://github.com/brawer/osmdiffs/issues/930). +pub(super) fn enclosing_host<'a>( + own: &'static str, + brands: &[u64], + tags: impl Iterator + Clone, + geometry: impl FnOnce() -> Option, + point: &geo::Point, +) -> Option { + use geo::Contains; + let category = main_category(tags.clone()) + .and_then(|c| HOST_CATEGORIES.iter().find(|h| **h == c).copied())?; + let ids: Vec = brand_ids(tags).filter(|id| brands.contains(id)).collect(); + if ids.is_empty() || !geometry().is_some_and(|g| g.contains(point)) { + return None; + } + Some(Host::Department { + brands: ids, + category, + own, + }) +} + /// The category in [DEPARTMENT_CATEGORIES] or [HOST_CATEGORIES] that /// equals `category`. fn known_category(category: &str) -> Option<&'static str> { @@ -358,6 +403,87 @@ mod tests { assert!(!same_business_name("", "rewe")); } + #[test] + fn test_enclosing_host() { + let building: geo::Geometry = geo::Polygon::new( + geo::LineString::from(vec![(0.0, 0.0), (0.002, 0.0), (0.002, 0.002), (0.0, 0.002)]), + vec![], + ) + .into(); + let inside = geo::Point::new(0.0019, 0.0019); + let outside = geo::Point::new(0.003, 0.001); + let supermarket = [("shop", "supermarket"), ("brand:wikidata", "Q7;Q9")]; + assert_eq!( + enclosing_host( + "amenity=pharmacy", + &[7], + supermarket.iter().copied(), + || Some(building.clone()), + &inside + ), + Some(Host::Department { + brands: vec![7], + category: "shop=supermarket", + own: "amenity=pharmacy", + }) + ); + let host = |tags: &[(&'static str, &'static str)], brands: &[u64], p: &geo::Point| { + enclosing_host( + "amenity=pharmacy", + brands, + tags.iter().copied(), + || Some(building.clone()), + p, + ) + }; + assert_eq!(host(&supermarket, &[7], &outside), None); + assert_eq!(host(&supermarket, &[8], &inside), None); + let mall = [("shop", "mall"), ("brand:wikidata", "Q7")]; + assert_eq!(host(&mall, &[7], &inside), None); + let pharmacy = [("amenity", "pharmacy"), ("brand:wikidata", "Q7")]; + assert_eq!(host(&pharmacy, &[7], &inside), None); + let node: geo::Geometry = geo::Point::new(0.0018, 0.0019).into(); + assert_eq!( + enclosing_host( + "amenity=pharmacy", + &[7], + supermarket.iter().copied(), + || Some(node.clone()), + &inside + ), + None + ); + } + + #[test] + fn test_department_brands() { + let place = |tags: &[(&str, &str)]| { + let coord = geo::Coord { x: 8.5, y: 47.4 }; + Place::new( + &coord, + "test_spider".to_string(), + MatchMask::SHOP, + tags.iter() + .map(|(k, v)| (k.to_string(), v.to_string())) + .collect(), + &geo::Point::from(coord).into(), + crate::utils::UtcTimestamp( + time::UtcDateTime::from_unix_timestamp(1_770_000_000).unwrap(), + ), + ) + .unwrap() + }; + assert_eq!( + department_brands(&place(&[("amenity", "pharmacy"), ("brand:wikidata", "Q7")])), + Some(("amenity=pharmacy", vec![7])) + ); + assert_eq!(department_brands(&place(&[("amenity", "pharmacy")])), None); + assert_eq!( + department_brands(&place(&[("shop", "supermarket"), ("brand:wikidata", "Q7")])), + None + ); + } + #[test] fn test_known_category() { assert_eq!(known_category("amenity=pharmacy"), Some("amenity=pharmacy")); diff --git a/src/pipeline/conflate/mod.rs b/src/pipeline/conflate/mod.rs index 513a223..988cb9a 100644 --- a/src/pipeline/conflate/mod.rs +++ b/src/pipeline/conflate/mod.rs @@ -70,7 +70,7 @@ mod writer; use deletion::{AtpCoverage, Candidate, Reason, deletion_reason, local_keys, place_country}; use diagnostics::{Diagnostics, Missing}; use local_coverage::{LocalCoverage, LocalKey, MIN_LOCAL_SHARE}; -use located_in::{Host, find_departments, located_in_tag}; +use located_in::{Host, department_brands, enclosing_host, find_departments, located_in_tag}; use s2_util::MergedCellRanges; use writer::{MatchInfo, ParquetRow, ParquetWriter}; @@ -266,8 +266,20 @@ fn produce_rows( elapsed_seconds = start.elapsed().as_secs_f64(); "conflate.departments: done" ); + // Departments that [find_departments] missed, but whose position is + // inside an OSM store of their brand, by ordinal; see + // [enclosing_host]. + let enclosing_hosts: Mutex> = Mutex::new(HashMap::new()); let host_of = |ordinal: u64, atp: &Place| { - located_in_tag(atp).or_else(|| departments.get(&ordinal).cloned()) + located_in_tag(atp) + .or_else(|| departments.get(&ordinal).cloned()) + .or_else(|| { + enclosing_hosts + .lock() + .expect("enclosing_hosts lock") + .get(&ordinal) + .cloned() + }) }; let located_in_rows = AtomicU64::new(0); // Sends the row of an ATP feature without a match, and returns @@ -323,7 +335,7 @@ fn produce_rows( return Ok(()); }; has_matcher[i].store(matcher.counts_for_coverage(), Ordering::Relaxed); - let host = host_of(batch_start + i as u64, atp); + let mut host = host_of(batch_start + i as u64, atp); let keep = if matcher.is_exclusive() { EXCLUSIVE_CANDIDATES } else { @@ -342,24 +354,70 @@ fn produce_rows( // OSM features of the kind that the matcher found no // evidence for, for the diagnostics. let mut rejected = Vec::new(); - for (lo, hi) in MergedCellRanges::new(covering) { - for r in osm.index.query(lo..=hi, atp.mask) { - let feature = osm.index.get_feature(r)?; - osm_candidates_decoded.fetch_add(1, Ordering::Relaxed); + // A department-category feature that isn't known to be in a + // host yet may still be inside an OSM store of its brand, so + // its candidates are kept until all have been seen; see + // [enclosing_host]. + let maybe_enclosed = match host { + None => department_brands(atp), + Some(_) => None, + }; + let point = maybe_enclosed.as_ref().map(|_| { + let ll = s2::latlng::LatLng::from(s2::cellid::CellID(atp.s2_cell_id)); + geo::Point::new(ll.lng.deg(), ll.lat.deg()) + }); + let mut held = Vec::new(); + let mut score_candidate = + |r: LocalFeatureRef, feature: &Feature, host: Option<&Host>| -> Result<()> { let candidate = OsmCandidate { - feature: &feature, + feature, strings: &osm.strings, }; - if host.as_ref().is_some_and(|h| h.excludes(candidate.tags())) { - continue; + if host.is_some_and(|h| h.excludes(candidate.tags())) { + return Ok(()); } let score = matcher.score_osm_candidate(&candidate); if score <= 0.0 && rejected.len() < diagnostics::MAX_REJECTED { rejected.push(r); } keep_best(&mut best, keep, score, r); + Ok(()) + }; + for (lo, hi) in MergedCellRanges::new(covering) { + for r in osm.index.query(lo..=hi, atp.mask) { + let feature = osm.index.get_feature(r)?; + osm_candidates_decoded.fetch_add(1, Ordering::Relaxed); + let (Some((own, brands)), Some(point)) = (&maybe_enclosed, &point) else { + score_candidate(r, &feature, host.as_ref())?; + continue; + }; + if host.is_none() { + let candidate = OsmCandidate { + feature: &feature, + strings: &osm.strings, + }; + host = enclosing_host( + own, + brands, + candidate.tags(), + || candidate.geometry().ok(), + point, + ); + } + held.push((r, feature)); } } + for (r, feature) in &held { + score_candidate(*r, feature, host.as_ref())?; + } + if maybe_enclosed.is_some() + && let Some(host) = &host + { + enclosing_hosts + .lock() + .expect("enclosing_hosts lock") + .insert(batch_start + i as u64, host.clone()); + } // An exclusive matcher without candidates has nothing to // compete for, so its row can go out right away; that keeps From f5bee4389c9186440d8612432bc51f8070d01f78 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 12 Nov 2026 09:29:08 +0000 Subject: [PATCH 2/2] docs(conflate): clarify why department candidates are held Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01Ttfb2HyX6m2N4gcDLAazrJ --- src/pipeline/conflate/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/pipeline/conflate/mod.rs b/src/pipeline/conflate/mod.rs index 988cb9a..5d4bd74 100644 --- a/src/pipeline/conflate/mod.rs +++ b/src/pipeline/conflate/mod.rs @@ -356,8 +356,8 @@ fn produce_rows( let mut rejected = Vec::new(); // A department-category feature that isn't known to be in a // host yet may still be inside an OSM store of its brand, so - // its candidates are kept until all have been seen; see - // [enclosing_host]. + // its candidates are held and scored only once all have been + // seen, when the host is known; see [enclosing_host]. let maybe_enclosed = match host { None => department_brands(atp), Some(_) => None,