SPB Git

spb/wp38_uqo

Public
14.1 KB · 90 lines markdown
1# Referee 3 — Data engineering and reproducibility (WP38)23*Adversarial audit of the pipeline, 2026-11-01. Scope: code under `src/`, `Makefile`, config, ledger/manifest, cached artefacts, and the consistency between text and outputs. Nothing outside `reports/` was modified; no pipeline step or LLM call was rerun. All numbers below were measured with `.venv/bin/python` on the artefacts present at commit `47d768d`.*45## 1. Five most serious threats67### T1 — Match-validation precision is mis-computed (and the paper repeats the wrong number)8`src/validation/match_validation.py:90-92` builds `cons = llm_a.where(llm_a == llm_b)` (NaN when annotators disagree) and then reports `yes = (cons[tier] == "yes").mean()`. A NaN compares as `False`, so the denominator is **all 60 pairs**, while the `n` printed beside it is the **consensus count** (56/54/52/48). The same bug affects `no` and `precision_by_tier_comparable_only`.910Check run: recomputed from `data/processed/match_validation_sample.csv`.1112| tier | consensus n | yes | reported `yes` (= yes/60) | correct yes/consensus |13|---|---|---|---|---|14| A | 56 | 55 | 0.917 | **0.982** |15| B | 54 | 54 | 0.900 | **1.000** |16| C | 52 | 46 | 0.767 | **0.885** |17| D | 48 | 10 | 0.167 | **0.208** |1819`paper/sections/data.tex:24` states "among consensus pairs the share judged the same product is 0.92 … 0.90 … 0.77 … 0.17", `reports/phase2_data_audit.md` §2.5, `reports/limitations.md` L-10 ("precision 0.90–0.92") and the abstract ("matching precision 0.90–0.92") all inherit the mismatch. The direction is conservative, but the text describes a statistic that is not what the code computes.2021### T2 — The audit sample is not reproducible from the pipeline and leaks across the clean/sibling boundary22`category_audit.sample()` draws 25 listings per category from `f_analysis_assort` with seed 20261101 and then refuses to overwrite. The sample on disk contains **1 listing (uid `09cadb97…`, store `cafewilliamspartivento.com`) that is outside the current assortment sample** because that store is a non-representative sibling (`f_sibling_dup = True`). The sample was therefore drawn *before* `src/clean/siblings.py` existed in the `clean` chain (ledger: `clean.listings` 17:48:50Z, `category_audit.annotate` 17:56:56Z, `clean.siblings` first at 17:52:34Z, second `clean.listings` 17:52:57Z — the sample itself is not ledgered). Re-running `make clean` + `category_audit sample` from a fresh clone will produce a different 500-listing sample, so the 0.46 precision, the 474-listing classifier hold-out and A-002's primacy decision cannot be regenerated bit-for-bit. Neither `category_audit_sample.csv`, `match_validation_sample.csv` nor `category_train_sample.csv` is registered in `logs/manifest.json` (59 entries, none under `data/processed/*.csv`), so there is no checksum tying the published figures to the cached labels.2324Related leakage: uids are disjoint between audit and training (0 overlap, verified), but **92.2 % of audit stores also appear in the training sample**. With merchants publishing near-duplicate titles, the "held-out" 0.88 accuracy is a within-merchant estimate, not the merchant-holdout the mandate (Phase 5) asks for.2526### T3 — H4 is built on survivors only; its docstring and "exits" are false by construction27`src/models/h4_dynamics.py:47` reads `li[li.f_published | (li.active == 0)]` with the comment "all live-store listings incl. inactive". But `listings_clean.parquet` is filtered to `f_published & f_live_store` in `src/clean/listings.py:44`; verified: 390 108 rows, `active` ∈ {1} only, and **0 rows with `last_seen` before the outcome window** (2026-09-28). Consequently (i) baseline portfolios (W36–W39) are reconstructed from listings that survived to the snapshot, so a category whose listings were all withdrawn before the snapshot looks "absent at baseline" and any later listing becomes a spurious "entry"; (ii) `region_level_exits` is `[]` in both namespaces because every baseline listing is, by construction, still present in W40–W44. The 73 618 inactive listings (source audit §0.2.3) exist in `listings_raw.parquet` and were simply never loaded.2829### T4 — Reproducibility claims in the paper/README do not match what is versioned30- `paper/sections/reproducibility.tex` says "the labelled audit samples … the run ledger with every API call and its cost, and the SHA-256 of every output are in the repository". `.gitignore` lines 4 and 31 ignore `data/processed/*` and `logs/`; `git ls-files logs data/processed` returns only `data/processed/.gitkeep`. The ledger records aggregate token counts per annotation run, not per call, and no cost.31- `README.md` TL;DR reports "Cramér's V 0.107 vs 0.075 … trimmed-listing p = 0.46 … ρ = −0.20, CI [−0.62; 0.34]". The rule namespace gives V = 0.1049 / 0.0747 / p_T = 0.492; the model namespace (declared primary) gives 0.1114 / 0.0858 / 0.445 and ρ = 0.00 [−0.63; 0.60]. **0.107 matches neither**; the README mixes a stale H1 run with rule-label H2 while the paper abstract correctly uses the model namespace.32- `.gitignore` ignores `data/processed/*` but not `data/processed_model/` (asymmetric glob), so **19 label-dependent artefacts are tracked**, including `store_panel.parquet` (1 557 merchant domains with FSA centroid `lat/lng` and `median_price`) and `h5_groups.parquet` (2 203 listing-level prices). The paper states the catalogue data are not redistributed.33- Neither the `category_model` steps (`sample|annotate|embed|train`) nor `src.models.summary` (hypothesis registry) appear in the `Makefile`, so `WP38_LABEL=model make features …` fails on a fresh clone (no `category_model.parquet`), and `make all` fails at `validate` without the cached CSVs that are not in git.34- Stale documentation: `reports/data_dictionary.md` still defines `region_final` as "region_db, else region_pip" and `region_source` as "db | pip | unknown" (A-001 made `city` the first rule, 393 stores); `data/raw/README.md` points external data to `data/raw/external/`; `reports/00_source_audit.md` §0.1.1 says "No empirical result depends on an LLM" although the primary labelling (A-002) rests on claude-sonnet-5-5 labels and OpenAI embeddings.3536### T5 — Label namespaces and sensitivity samples: one silent misnomer, one silent fallback, no test coverage37- `src/clean/stores.py:98` defines `region_dbfirst = region_pip.fillna(region_city).fillna(region_db)` with the comment "this is the PIP-first sensitivity variant"; `region_category.py:34`, `h1_raa.py:41` and `robustness.py:44` (`"S_R0_dbfirst_regions"`) use it for S_R0. The preregistration (§ samples) defines S_R0 as "stores with a Fabri-Ka region only (`region_source = db`)". Verified: `region_dbfirst == region_pip` for 100 % of stores with a PIP value. So the S_R0 rows of Table h1 (label "S\_R0\_dbfirst\_regions") are PIP-first regions on the 1 223 DB-labelled stores — not the preregistered sample, and the table label says the opposite.38- `stores.py:23-45` globs **every** `.gpkg/.shp/.geojson` under `data/external/` and accepts the first layer whose name column covers ≥ 15 regions; if the MERN file is missing it silently falls back to `qc_er_2021_statcan_cbf.gpkg` (pre-2021 Estrie/Montérégie boundaries, the very artefact A-001 removed), logging only a filename.39- Namespace separation itself is correct (verified: `data/processed_model/` holds only label-dependent files; label-independent `store_panel.parquet` is byte-identical in both; audit/matching artefacts live only in base dirs; `category_model.parquet` is written to `BASE_PROC`). But `ledger()` (`common.py:45`) never records `LABEL`: 139 ledger lines, 0 with a label field, and steps such as `models.h1_raa` appear 9 times and `reports.make_tables` 18 times with no way to tell which namespace they wrote. `tests/test_panel.py` (7 tests, all pass in 0.85 s) covers none of: audit/train disjointness, namespace separation, S_R0 definition, consensus-precision arithmetic, H4 inputs.4041## 2. Checks actually run4243| check | command / artefact | result |44|---|---|---|45| Test suite | `make test` | 7 passed |46| Price sample currency | `listings_clean[f_analysis_price]` | only `CAD`; min `store_cad_share` = 1.0 (≥ 0.95 rule holds) |47| Sibling rule | non-rep sibling stores in assortment; H5 groups spanning only siblings | 0 / 0 of 1 064 groups |48| Audit vs train | uid overlap | 0 (audit 500, train 6 481); store overlap 92.2 % |49| Audit ⊂ assortment | 499 / 500 (see T2) |50| Match-validation precision | recomputed (T1) | reported = yes/60, not yes/consensus |51| H4 inputs | `active` values, `last_seen < 2026-09-28` | {1}: 390 108; 0 rows |52| Manifest integrity | sha256 of 59 manifest entries vs disk | 0 mismatches |53| Ledger | 139 records, 23 step types; API usage lines: audit 2×500 + repairs, matching 2×240 + repairs, embed 370 055 listings / 24.09 M tokens, classifier 6 481 calls / 8.37 M in / 0.26 M out | no `label`, no cost |54| Timing vs preregistration | prereg commit 17:57Z; first `models.*` ledger entry 17:58:18Z; `category_model.train` 18:40:59Z after `decisions.md` (18:22Z) | consistent with D-007 and A-002 |55| Secrets | `git grep` for key patterns; `.env` ignored | none tracked |56| Hard-coded paths | grep `/Users|/Desktop` in `src`, `tests`, `Makefile` | none (`ROOT` from `__file__`) |57| Large files tracked | `git ls-files` sizes | `coverage_map.pdf` 2.2 MB ×2, `main.pdf` 2.0 MB; acceptable |58| Background annotation | grep `nohup` | not used; annotation ran in foreground, logs in `logs/*_annotate.log`; CSVs written only at end of each annotator loop (checkpoint every 500 for the classifier) — a crash mid-loop loses that annotator's calls but never corrupts the file |59| Paper numbers | abstract/data.tex vs `experiments_model/*.json`, `experiments/*.json` | H1 (0.111/0.086, p 0.21/0.45), H3, H5 (4.7 log pts, −0.116, p 0.005/0.001), classifier (6 481, 0.69, 0.88, 0.91, 474, 52.3 %), representativeness (0.91/0.89/0.93/0.84; 1.8–11.1; CV 0.43), H4 (22 920 cells, 104 entries, 82 stores), ECI ρ 0.67 — all match; matching precision (T1) and README (T4) do not |60| Namespaces | file sets of `processed{,_model}` and `experiments{,_model}` | correct separation (T5) |61| Idempotence of `make clean` | reads `Makefile:21` | `listings` runs twice by design (circular dependency with `siblings`); final state stable, ledger/manifest get duplicate entries; `tables figures:` shared recipe runs twice when both targets are requested (18 vs 15 ledger runs) |6263## 3. Unresolved limitations6465- Determinism was verified only indirectly (manifest hashes match disk; seeds set everywhere; thread-pool results consumed in order); no two-run comparison exists because a rerun was out of scope.66- LLM annotations are intrinsically non-reproducible (reasoning models, `thinking: adaptive`); only caching protects them, and the caches are unversioned and unhashed (T2/T4).67- `region_pip` rests on FSA centroids geocoded upstream (zippopotam/GeoNames); the 40 straddling FSAs flagged in `fsa_to_region_2021.csv` are never joined to `stores_master`.68- The sample-flow step "of an enabled store with ≥ 1 published listing" removes 0 rows; the table does not say that `f_live_store` is redundant in this snapshot.6970## 4. Remediation plan7172**Must-fix (before submission)**731. `match_validation.report`: compute `c = cons[df.tier == t].dropna()` and report `yes = (c == "yes").mean()`, `no`, `unsure`, `n = len(c)`; same for `comparable_only`. Regenerate `experiments/match_validation.json`, `paper/tables/match_validation.tex`, `phase2_data_audit.md`, `limitations.md` L-10, `data.tex:24`, abstract.742. Register the three LLM sample CSVs in `logs/manifest.json` (`register_output` in `sample()` and after each `annotate()`), add their sha256 to `reports/reproducibility.md`, and either commit them (they contain no personal data) or publish them as a release asset; drop the uid from the sibling store or document it as a frozen pre-sibling draw.753. Reconcile `reproducibility.tex` and `README.md` with git: track `logs/run_ledger.jsonl` and `logs/manifest.json` (or say they are published separately); fix the README numbers to the primary namespace (0.111 / 0.086; p = 0.45; ρ = 0.00 [−0.63; 0.60]).764. `.gitignore`: replace `data/processed/*` by `data/processed*/` (or deliberately track both with a privacy review of `store_panel.parquet` / `h5_groups.parquet`); state the policy in `data/raw/README.md`.775. H4: load `listings_raw.parquet` filtered to live, non-sibling stores and `listing_status == 'published'` regardless of `active`, so that baseline presence includes withdrawn listings; recompute entries/exits; or rewrite the docstring and the paper to say "entries among surviving listings; exits unobservable".786. Rename `region_dbfirst` → `region_pipfirst` and the robustness key to `S_R0_pipfirst_regions`; either add the preregistered "Fabri-Ka label only" variant or amend the preregistration explicitly (A-003).7980**Should-fix**817. Add `label=LABEL` to every `ledger()` record and a `cost_usd` field computed from the model price list; add a `make model-labels` target (`sample annotate embed train`) and a `make registry` target for `summary.py`; make `make validate` skip gracefully when CSVs are absent.828. `load_regions_layer`: take the layer path from `settings.yaml` (`data/external/geo/qc_regions_admin_mern_bdat100k.gpkg`) and raise if missing.839. Tests: audit∩train = ∅ and audit ⊂ assortment; `data/processed_model` contains no audit/matching files; `store_panel` identical across namespaces; H4 inputs include `active == 0`; consensus-precision denominators; `region_final` precedence city > db > pip.8410. Store-level holdout for the classifier (GroupKFold by `store_id`, and an audit-store-disjoint evaluation) reported next to the 0.88.8586**Nice-to-have**8711. Regenerate `reports/data_dictionary.md` with the A-001 definitions (pull `DESC` for `region_final`, `region_source`, `region_conflict` from one place shared with `stores.py`); fix `data/raw/README.md`, `00_source_audit.md` §0.1.1 and `data/external/README.md`.8812. Break the `listings ↔ siblings` cycle (compute siblings from `listings_raw` + `norm_title`) so `make clean` runs each step once; split the `tables figures` recipe.8913. Join the 40 straddling FSAs as a `geo_uncertain` flag and add it to the robustness list already promised in `data/external/README.md`.90
Files · 2789bf667fc532fd5c04f52650ccc62606fd212e
Menu

Theme