RampNet 1.0 seam amendment, and the detector effect I had wrongly ruled out - #142
Conversation
…#132) Jon pushed back on "RampNet detects ramps on the seam normally", which I had drafted for the README: whether the model copes should depend on how much of the ramp falls on each side of the split. He was right, and the claim does not survive measurement. scripts/analysis/seam_response.py measures the thing the earlier retracted result could not: the RAW HEATMAP RESPONSE at the ramp, with no peak extraction anywhere in the path, so exclude_border cannot confound it. Paired and within-ramp -- the same ramp with the panorama as stored, and rolled 180 degrees so the seam sits half a world away. Ramps far from the seam in the SAME panoramas are the control. n stored -> rolled paired diff gained >0.05 seam-band ramps 25 0.785 0.818 +0.033 9 / 25 control 111 0.848 0.845 -0.003 0 / 111 Read the rate, not the mean. The effect is heterogeneous -- it depends on how the ramp straddles the split -- so averaging the badly-affected with the unaffected dilutes it to insignificance (t=1.20). The sharp statistic is that rolling moves no control ramp by more than 0.05 and 9 of 25 seam ramps, with individual gains reaching +0.32. It does not resurrect the retracted claim: recall at the seam is still 0.96, not 0.44, because most of these ramps have headroom above the 0.30 threshold. It does mean the model is not indifferent to the seam, and docs/seam.md section 1 no longer says it has no seam defect. New section 4a carries the measurement and its limits: n=25, and unstratified by straddle fraction, which is the variable the effect most plausibly depends on. README gains a dated dataset note as a sibling to the July 2026 erratum -- not an erratum itself, since no published number moves. It states the defect, that we are deliberately not correcting 1.0, and the detector effect above. It offers no remediation advice and makes no promise of a fix, at Jon's direction: the point is to make people aware of the problem. docs/seam.md gains an appendix holding the exact wording to be published on the Hugging Face dataset card, so the two cannot drift. Refs #132, #130. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jonfroehlich
left a comment
jonfroehlich
left a comment
There was a problem hiding this comment.
Deep review of this PR (head a0bb241 against base bf64451), run at high effort: ten finder passes, then every candidate verified against both commits and the committed artifacts before it was kept. 15 findings, ranked most-severe first; the inline comments are numbered in that order. One finding sits on a line outside the diff, so it goes here:
14. §6 Replication was not extended for §4a (docs/seam.md line 238). "Every command below runs from a clean clone" — but the new experiment's command exists only inline in §4a, with none of §6's requirement annotations (GPU, untracked benchmark/*/panos, committed op_cache, HF model download), and seam_response.py is absent from §6's inputs inventory. A replicator working from the doc's designated replication protocol reproduces everything except this PR's own new result; one working from §4a's one-liner is never told it needs the untracked panoramas and a GPU, unlike §6's command 4, which states exactly that.
Three smaller notes that did not make the ranked list (all verified):
- §1 (line 30) still says "the paper's numbers" were never affected, which now reads oddly against §2's and the README's admission that the gold-set recall figures are understated.
dedup_seam_onlykeeps the first member of each pair, so which physical mark gets measured depends on op_cache GT ordering.- The
MATERIALcomment says "this size or larger" while the code and the doc both use strict "more than".
What checked out clean: the paired design and the roll/np.roll alignment, the matcher-unit geometry, every §4a table number against the committed JSON (means, t-statistics, the 9/3/0/3 counts, the top gains +0.32/+0.29/+0.24), JSON LF purity and newline="", rounding margins at the 0.05 boundary (closest 0.0023), and the 0.98% / 3.7% / 17.2× / ~4° figures against the committed scan.
🤖 Generated with Claude Code (claude-fable-5)
| a = response(stored, g[0], g[1]) | ||
| b = response(rolled, g[0], g[1]) | ||
| d = dist_to_seam(g[0], PANO_SCALE_X) | ||
| (seam if d < SEAM_BAND else control).append( |
There was a problem hiding this comment.
1 (most severe). The control arm is contaminated — and the three control losses are the contamination. (seam if d < SEAM_BAND else control) bins as "control" two kinds of ramp the roll is not neutral for: (a) the centre-band ramp the 180° roll moves onto the seam — manual_gold:mLHZho9oLtF_S2JpEUai6g sits 4.48 px from the anti-seam (seam_px 507.52), loses 0.145 when rolled, and is the largest control mover, driving the control t=−1.69; the imported module's band_of() and its docstring ("centre ramps → move to the seam") exist precisely to exclude this case, and the committed seam_roll_diagnostic.json classifies this exact ramp as "centre" — and (b) near-seam ramps whose 22.53 px match disc overlaps the seam (seam_px 18.30 and 18.43, both < R). Those are all three control "lost > 0.05" rows in the committed JSON. The script's own docstring pre-registers the validity criterion — if the roll moves controls, "the roll itself is not neutral and no seam conclusion can be drawn" — yet §4a publishes control t=−1.69 (larger in magnitude than the seam arm's 1.20) without engaging it. A clean control (excluding d < R and the centre band) reads 0 lost / 0 gained, so the conclusion survives; the arm has to be re-binned. Separately, adjacent GT sharing one disc maximum produce duplicate rows — paterson:3dq9TqRXzopD0yvztHg8rg appears in both arms with identical 0.9357→0.9695 — so the 111 control rows are not independent for the quoted t-statistic.
|
|
||
| A panorama wraps: the left and right edges of an equirectangular image are the same place. Stage 1's label generator extracts curb ramp locations from a 4096×2048 equirectangular heatmap with `peak_local_max(min_distance=40)`, and that suppression does not carry across the wrap. A ramp sitting on the seam can therefore produce a peak on each edge and be labelled **twice**. | ||
|
|
||
| Measured across the whole published dataset: **8,361 seam-crossing duplicate label pairs among 849,904 labels (0.98%)**, affecting **7,987 of 214,385 panoramas (3.7%)** — 17.2× more than a uniform-azimuth null predicts. Independently of that null, panoramas containing such a pair carry exactly **+1.000** more labels than they have source government ramp records. |
There was a problem hiding this comment.
2. The "+1.000" sentence is false as stated. "Panoramas containing such a pair carry exactly +1.000 more labels than they have source government ramp records" — the measured raw excess for pair-panos is +0.591; +1.000 is the difference against the −0.408 baseline of panos without a pair. Base docs/seam.md §3 carries the defining parenthetical ("mean +0.591 against −0.408 without; z = +14.0"); this sentence and the HF-card appendix copy (docs/seam.md line 309) both dropped it. Anyone re-deriving mean(labels − records) over pair-panos from the published dataset gets +0.591 and concludes the README and the dataset card are wrong — on the two most public surfaces this PR creates.
|
|
||
| **We are not correcting the published 1.0 dataset.** It is the artifact the paper's numbers were computed on; replacing it would mean a downloader gets different data than the paper used, which we think is worse for reproducibility than a documented defect. | ||
|
|
||
| **The seam also costs the detector some response, though less than it costs the labels.** Rolling a panorama so the seam falls elsewhere raises the model's activation at the ramp by more than 0.05 for 9 of 25 gold-set ramps within ~4° of the seam, against 0 of 111 control ramps in the same panoramas. The effect varies with how much of a ramp falls on each side of the split; in this sample it rarely pushed a ramp below the detection threshold — 24 of the 25 were still detected — but it would where response is already marginal. The evaluation path itself is unaffected. |
There was a problem hiding this comment.
3. "25 gold-set ramps" misattributes the sample. Only 19 of the 25 seam-band ramps are from the gold set — recounted from the committed analysis_out/seam_response.json seam_rows: manual_gold 19, paterson 3, bend 2, richmond 1 (controls: 92/111 manual_gold). collect_targets iterates ALL_SPLITS, not manual_gold. The HF-card appendix repeats it (docs/seam.md line 322, "seam-adjacent gold-set ramps"). As written, the text implies the independently-labelled gold set while a quarter of the sample uses RampNet-anchored verdict GT.
| individual gains reaching +0.32, +0.29 and +0.24. | ||
|
|
||
| **What this does and does not mean.** In this sample the lost response rarely pushed a ramp below | ||
| the 0.30 detection threshold — 24 of 25 were still detected — because most had headroom. Where a |
There was a problem hiding this comment.
4. "24 of 25 were still detected" belongs to the other instrument, and this experiment's own artifact contradicts it. The minimum stored response in analysis_out/seam_response.json is 0.3083 — 25/25 are above the 0.30 threshold with the seam in place, so "the lost response pushed a ramp below the detection threshold" happens zero times in this sample. The only sub-0.30 value anywhere in the file is a rolled one (manual_gold:5JtOUPhPxP29dOkItrmQMw, 0.2848 — the seam-free condition). The 24/25 figure comes from §1/§4's earlier peak-based roll diagnostic, silently re-attributed to the response experiment; README line 87 repeats it. A reader recomputing detection status from the artifact this sentence cites cannot reproduce the sentence.
| > +1.000 more labels than they have source government ramp records. | ||
| > | ||
| > **If you are training on this dataset**, the practical effect is a small number of near-duplicate | ||
| > targets close to `x ≈ 0` / `x ≈ 1`. Duplicate pairs sit roughly 1–9° apart. On a |
There was a problem hiding this comment.
5. "Duplicate pairs sit roughly 1–9° apart" is wrong at both ends for the population it describes. The committed scan's 8,361 pair separations (analysis_out/stage1_seam_scan.json, seam_pair_separations_px; the zone fractions 65.5/20.0/14.5 match §3, confirming units) span 0.088°–7.914°, with 12.5% below 1° — and 9° is unreachable by construction, since pairs were counted within the 22.53 px ≈ 7.9° wrapped radius. 1–9° is the span of the 14-pair gold-set adjudication (1.04°–8.81°), not the dataset's. A consumer deduplicating with a "pairs are ≥ 1° apart" assumption misses one in eight of the actual pairs.
| seam, control = [], [] | ||
| for i, (city, pid) in enumerate(keys, 1): | ||
| path = find_pano_image(args.panos_root, city, pid) | ||
| if path is None: |
There was a problem hiding this comment.
10. Missing panoramas are silently dropped, and the written JSON carries no provenance. No skipped list, pano count, panos_root, threshold, model revision, or content hash — the sibling script records every one of these, and the repo rule is that a derived artifact carries a content hash so a regenerated copy can be proven identical. Run from a git worktree or a machine holding only the manual_gold panos (find_pano_image's own docstring warns about exactly this), the script completes normally reporting seam n=19 instead of 25, with no signal that 6 panos were skipped — and the doc's n=25 cannot be reconciled from either output. load_model() fetches whatever projectsidewalk/rampnet-model currently serves, so a re-run against a later revision produces a plausible replacement JSON nothing can prove same-or-different.
| s, c = summarize(seam), summarize(control) | ||
| for name, r in (("SEAM-BAND (within ~4 deg of the seam)", s), | ||
| ("CONTROL (same panoramas, away from the seam)", c)): | ||
| print(f"\n{name} n={r['n']}") |
There was a problem hiding this comment.
11. An empty run crashes with a bare KeyError — after the expensive model download and load. summarize() returns {} for an empty group and the summary loop unconditionally indexes r['n']. Wrong --panos-root, absent benchmark/*/panos (worktree or clean clone), or missing op_cache files (collect_targets skips absent city caches silently) all yield empty arms; the script pays for the HF download and inference, then dies at print(f"n={r['n']}") with no hint that the real problem is zero panoramas found. The parent script guards the analogous case per band and prints a skipped report.
|
|
||
| **Read the rate, not the mean.** The mean shift is small and not individually significant, because | ||
| the effect is heterogeneous — it depends on how much of a ramp falls on each side of the split, so | ||
| averaging the badly-affected together with the unaffected dilutes it. The sharp statistic is that |
There was a problem hiding this comment.
12. The "sharp statistic" is drawn one-sided. 9/25 gained vs 0/111 control gained — while the table shows 3/25 seam-band ramps lost response when the seam was rolled away, one by −0.329, larger than all but one gain, and the text never explains or tests that rate. Read two-sidedly by the doc's own methodology it is 12/25 moved vs 3/111 moved (or a 9-vs-3 sign test) — a different-strength result — and an unexplained asymmetric choice of statistic invites exactly the criticism this amendment exists to avoid. The −0.329 loss also drops that ramp below the 0.30 threshold when rolled: the roll is not neutral for seam ramps in the unpredicted direction either.
| ap.add_argument("--limit", type=int, default=0) | ||
| args = ap.parse_args() | ||
|
|
||
| targets = collect_targets("seam") |
There was a problem hiding this comment.
13. Input and output trees can silently diverge. collect_targets reads the op_cache through operating_point_curve's CACHE_DIR, which honours $RAMPNET_ANALYSIS_OUT; the --json-out default is hard-anchored to REPO/analysis_out. With the env var set (standard on the cluster), GT comes from one tree — possibly a stale or regenerated cache with different GT, and the GT set defines which 25 ramps are measured — while the result overwrites the committed artifact in another, with nothing recording which caches were read.
| MATERIAL = 0.05 # a response change this size or larger counts as "moved" | ||
|
|
||
|
|
||
| def roll_image(img): |
There was a problem hiding this comment.
15. roll_image() and HEAT_W/HEAT_H re-implement things this script already imports the module for. The half-width crop/paste roll exists as seam_roll_diagnostic.rolled_pass() — duplicating it means a change to one (odd-width handling, say) silently misses the other, and the two seam experiments stop measuring the same transformation. And 1024/512 are re-hardcoded where silent_activation.py derives them from PANO_SCALE_X/Y with a runtime assert precisely "so a future resolution change cannot silently desynchronize the two" — a PANO_HEATMAP_SIZE change in rampnet/model.py would leave this script rolling by a stale 512 and wrapping columns at a stale 1024, producing wrong-but-plausible responses instead of an error.
…l for The first committed run of this experiment binned every ramp outside the seam band as a control. Three of those were ramps the roll itself moves: the ramp 4.5 px from the centre column, which is exactly where the 180-degree roll lands the seam, and two ramps whose response window straddles the seam as stored. Those three were the control arm's entire "lost > 0.05" column, and the script's own docstring states the validity rule they break: if the roll moves controls, no seam conclusion can be drawn. Found in review of #142. The binning is now three-way. A ramp is excluded from both arms when its response window touches the seam as stored or the column the seam rolls to, when it overlaps a seam-band ramp's window (its response is partly the seam ramp's, re-measured), or when it overlaps an already-kept control's window (adjacent ramps sharing one response are one measurement, not two). The control arm goes 111 -> 77 and is now exactly neutral: paired diff -0.0001, t = -0.26, 0 gained / 0 lost. The seam arm is unchanged: 9/25 gained, 3/25 lost. The conclusion survives its own correction; the arm it rests against is now clean. Also from the same review: - Responses now go through silent_activation.radius_max instead of a local re-implementation: values clip to [0, 1] as peak extraction clips (8 rows in the previous artifact exceeded 1.0, up to 1.0418) and the window applies the exact in-disc check. Two of the nine gains survive clipping by under 0.005. - The roll is defined once: roll_half() in seam_roll_diagnostic, used by both seam experiments. HEAT_W comes from silent_activation's derived constant rather than a second hardcoded 1024. - --json-out defaults to None again and --limit refuses it, restoring the parent script's guard against overwriting the committed artifact with a partial run. - The artifact records its provenance: model revision, per-city op_cache sha256, panos_root, skipped panos (previously silently dropped), and every float is rounded at the writer. - A run that finds no targets or loses an arm exits with a message naming the likely cause instead of a bare KeyError after the model download. analysis_out/seam_response.json is regenerated with the corrected instrument (model revision 606a119). tests/test_seam_response.py pins the binning rules, the strict "moved" threshold, and the artifact's internal consistency: the summary block re-derives from the rows, every control row satisfies the binning rules, bytes are LF with floats at six decimals. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…fact Review of #142 found the amendment's two public surfaces (the README note and the HF-card appendix) and section 4a stating numbers their own committed artifacts contradict. All corrected against analysis_out/stage1_seam_scan.json and the regenerated analysis_out/seam_response.json: - "+1.000 more labels" was the gap between pair-panos (+0.591) and non-pair panos (-0.408); the defining figures were dropped when the sentence was copied out of section 3. Both copies now carry them. - "8,361 duplicate label pairs" overstated: section 3's own adjudication puts ~1 in 7 pairs as two genuine adjacent ramps that must not be merged. Both public texts now say "seam-crossing label pairs" and state the caveat, and the unevidenced 4.5-6.1 degree band is no longer silent. - "Pairs sit roughly 1-9 degrees apart" was the 14-pair adjudication's span, not the dataset's: the committed scan spans 0.09-7.9 degrees with one pair in eight closer than 1 degree (9 degrees is unreachable by construction). - "25 gold-set ramps" mixed samples: 19 of the 25 are gold-set (3 paterson, 2 bend, 1 richmond). The composition is now stated wherever the 25 appears. - "24 of 25 were still detected" belonged to section 4's peak-level diagnostic, not this experiment, whose artifact holds no stored response below the 0.30 threshold (minimum 0.31). Now attributed correctly in all three places. - "moves no control ramp by more than 0.05" contradicted the table above it (3 of 111 moved; they were the contaminated rows). Section 4a's table and sharp statistic now reflect the corrected binning (control 77, 0 moved in either direction) and are read two-sided: 12 of 25 seam ramps moved, 9 up and 3 down, and the losses are discussed rather than passed over. - Section 4a documents the control-arm correction itself, the [0,1] clipped scale, the sample composition, and the <0.005 margins on two of the nine gains; its reproduce command is now command 5 in section 6's replication protocol, with the GPU/panos/model-download requirements stated like the others. - Section 1's "what was never affected" now scopes itself to the two retractions, since section 2's double-marks do slightly understate recall. - dedup_seam_only documents that which pair member survives depends on op_cache row order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All 15 review findings (and the three smaller notes in the review body) are now addressed:
The PR body's table and statistic are updated to the corrected run. The conclusion survives its own correction: the effect is real, and the arm it rests against is now clean. 🤖 Generated with Claude Code (claude-fable-5) |
1 participant
The RampNet 1.0 seam amendment, plus a measurement that corrects something I had drafted for it.
The correction that prompted this
The draft README note said "RampNet detects ramps on the seam normally." Jon pushed back: whether the model copes should depend on how much of the ramp falls on each side of the split. That is right, and the claim does not survive measurement.
scripts/analysis/seam_response.pymeasures the thing the earlier retracted result could not — the raw heatmap response at the ramp, with no peak extraction anywhere in the path, soexclude_bordercannot confound it. Paired and within-ramp: the same ramp with the panorama as stored, and rolled 180° so the seam sits half a world away. Ramps far from the seam in the same panoramas are the control — after excluding every ramp the roll is not neutral for (window touching either seam position, or overlapping another measured window).Read the rate, not the mean. The effect is heterogeneous — it depends on how the ramp straddles the split — so averaging the badly-affected together with the unaffected dilutes it to insignificance. The sharp statistic is two-sided: rolling the seam away moves 12 of 25 seam ramps by more than 0.05 (9 up, gains to +0.32; 3 down, the largest −0.33) and 0 of 77 controls in either direction.
This does not resurrect the retracted "the model is blind at the seam" claim — recall at the seam is still 0.96, not 0.44, because these ramps have headroom above the 0.30 threshold. It does mean the model is not indifferent to the seam, and
docs/seam.md§1 no longer says it has no seam defect.What is in the PR
README.md— a dated dataset note, a sibling to the July 2026 erratum rather than a second erratum, since no published number moves. It states the defect, that we are deliberately not correcting 1.0 and why, and the detector effect above. Per Jon's direction it offers no remediation advice and makes no promise of a fix — the point is to make people aware of the problem.scripts/analysis/seam_response.py+analysis_out/seam_response.json— the measurement, committed and re-runnable (§6 command 5), with the model revision and op_cache hashes recorded in the artifact — plustests/test_seam_response.pypinning the binning rules and the artifact's internal consistency.docs/seam.md— §1 corrected, new §4a for the response finding with its limits stated, and an appendix holding the exact wording to be published on the Hugging Face dataset card, so the published text and this report cannot drift.Amended after review (2026-08-19)
The deep review on this PR found the first committed version of the experiment invalid in exactly the way its own docstring warns about: three ramps the roll is not neutral for — the ramp sitting where the roll lands the seam, and two whose response window straddles it — were binned as controls, and they were the control arm's entire "lost" column. Fixed in 255dc29 (three-way binning, responses through the unit-tested
radius_max, provenance in the artifact, the partial-write guard restored) and c7b143c (every published number reconciled to its committed artifact — the +1.000 parenthetical, the pair-separation range, the gold-set attribution, the 24-of-25 misattribution, and the one-sided sharp statistic). The control arm went 111 → 77 and is now exactly neutral; the seam arm's result is unchanged. The table and statistic above are the corrected ones; the review comments carry the full list.Limits, stated in the doc rather than smoothed over
Not in this PR
The 10-pair GT merge, regenerating the
op_caches, the Stage 1 generator fix, and the three viewers that still clamp. The README note's line about corrected gold-set figures is deliberately a forward reference — those numbers need both the merge and the regeneration, so they are quoted nowhere yet.The Hugging Face card itself still has to be edited by hand; this PR only version-controls the wording.
🤖 Generated with Claude Code (claude-opus-5[1m])