Paper benchmark re-run: timing + distortion figures (post-merge code)#71
Conversation
Branch off dev (current merged perf/fidelity code) overlaid with the benchmark/ harness. New scripts for the paper benchmark: - time_cores.py: flatten-only core-scaling (1/8/16 physical cores via CPU affinity), k-ring cache disabled to measure prep scaling, true-geodesic + k-ring metrics, resumable, timestamped, ledger-logged. - time_e2e.py: end-to-end (FS-free projection + prep + flatten) @16 cores, per-subject both hemispheres, timestamped. - fig_paper.py: Nature-style aggregate figures (runtime breakdown, core scaling vs FS6, true-geodesic local/global distortion). - fig_flatmaps.py: per-hemisphere 3-method Fischl-1999 flatmap+histogram, fair fixed-neighborhood per-vertex distortion at optimal scale. Distortion reported via true-geodesic local/global at optimal scale (Ju 2005), not the n_neighbors-biased k-ring metric. mris_flatten results reused, never re-run. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
compute_kring_distortion returns (vertex_distortion, mean_distortion); the whole tuple was passed to np.asarray (inhomogeneous-shape error). Unpack [0], and cast big-endian surface faces to native int64 for the Numba k-ring. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the 3 separate per-method histograms with a single panel overlaying all three distributions, color-coded by method, normalized to % of vertices (fair across differing patch sizes), with mean/p90 in the legend. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Restrict the FreeSurfer6 distortion distribution to the exact (subject, hemi) set AutoFlatten was scored on, so all three methods are compared on the same 10 hemispheres (paired), not FS6's full 40-hemi set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces significant performance and metric-fidelity improvements to the flattening pipeline, including a FreeSurfer-free orientation alignment, a distance-optimal output scale calculation, and parallelized Numba kernels for k-ring neighbor and distance computations. A performance issue was identified in _angular_kring_kernel where calling _limited_dijkstra_numba inside a parallel prange loop introduces thread-allocator contention due to internal O(n_vertices) array allocations, which should be refactored to use pre-allocated per-chunk scratch buffers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
benchmark/ is research/paper tooling, not part of the installable package. - MANIFEST.in: prune benchmark (keeps it out of the sdist, which setuptools-scm otherwise fills from all VCS-tracked files). - pyproject.toml: explicit exclude=["benchmark*"] in packages.find (the include whitelist already excluded it from the wheel; this documents intent). Verified: 0 benchmark files in both the built wheel and sdist; autoflatten intact. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…here The continuity-only projection default (cli.py + test_cli.py) is split into the standalone package PR #72. The benchmark passes refine_geodesic=False explicitly, so it is unaffected. Keeps this PR benchmark-only (no package changes). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The angular k-ring kernel ran prange over every vertex and called _limited_dijkstra_numba inside the loop, allocating five O(n_vertices) scratch arrays per vertex -- the exact "melts the allocator across threads" pattern that _kring_distances_kernel was written to avoid (flagged on PR #71). Refactor to mirror _kring_distances_kernel: parallelize over chunks, pre-allocate per-chunk scratch (dist/visited/is_target/touched/heap) once, and inline the limited Dijkstra with the same correction and lazy-deletion heap discipline, resetting only touched entries between vertices. Output is bit-identical to the serial reference (verified single- and multi-chunk). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
fig_distortion now has a third panel showing flipped-triangle counts per method (robust_fast / tutte_default / FreeSurfer 6) alongside local and global metric distortion. FS6 flip counts come from the existing fs6_compare/comparison.csv (n_flipped), paired to the same hemispheres as AutoFlatten; AutoFlatten counts come from the cores CSV (n_flipped). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The flipped-triangle panel now plots the fraction of patch faces flipped rather than the raw count, so it is comparable across hemispheres/subjects with different patch sizes. AutoFlatten uses frac_flipped from the cores CSV; FS6's n_flipped is divided by the same hemisphere's patch face count (FS6 flattened the same AutoFlatten-projected patch -- verified identical induced face count, so denominators match). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Group quality-at-scale deliverable (distinct from timing): flattens a fixed 50-subject / 100-hemisphere Narratives cohort with both shipped variants, end-to-end FS-free (continuity-only projection + k-ring prep + flatten), and records true-geodesic distortion per hemi. Driver runs 4 hemispheres concurrently at 8 logical CPUs each (all 32 logical CPUs; HT is free throughput here since distortion is core-count-invariant). Resumable per-config CSV; run-local per-patch k-ring cache; reuses existing config-independent truegeo references as a head start. Cohort is deterministic (prior scaleup-20 + 30 from the materialized-content pool) and written to COHORT.md at launch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
The true-geodesic distortion reference ran heat geodesics on flattener.vertices, which load_data sets to the patch's stored coords = the FreeSurfer inflated surface (~1.7x larger, non-rigid, stretched triangles). This scored distortion against the wrong geometry (the energy preserves fiducial distances) and ill-conditioned the heat solver, emitting negative/near-zero geodesics that blew the relative metric up to 190-350% on a few hemispheres. compute_truegeo now runs heat on fiducial_vertices (the anatomical surface the flattener preserves and that metric distortion is defined on), clamps numerical negatives, and the opt-scale search is widened to [0.80, 1.25]. On the fiducial surface the blow-ups vanish with no denominator floor (sub-272 rh 192% -> 14.3%, 0 bad sources; normal sub-022 18.1% -> 15.9%; opt_scale -> ~1.0), so the interim 5mm floor is reverted. Tools to regenerate existing runs without re-flattening: - rebuild_truegeo.py: re-run heat on fiducial per hemi (4-lane, resumable) - recompute_truedist.py: rewrite metric columns from saved flat + truegeo - fig_group_raincloud.py: paired LH/RH + collapsed distortion rainclouds Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Head-start truegeo slots are symlinks to other runs' references; np.savez through a symlink would clobber the shared target. Unlink first so each rebuilt fiducial ref is a fresh local file. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Even on the fiducial surface the heat solver occasionally emits near-zero (or negative) geodesics for far vertices, surviving the >1e-6 mask and blowing the relative metric up to ~350% on ~10/100 hemispheres (median stays clean). A geodesic can never be below the straight-line 3D chord, so zero out any estimate under 0.5*chord -- a loose physical bound that flags only gross failures (~0% of pairs on clean hemis, normalizing the blown ones: sub-268 rh 367% -> 16.2%). Baked into compute_truegeo; rebuild_truegeo gains a --sanitize mode to mask already-built refs without re-solving heat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Re-scores both the FreeSurfer mris_flatten output (never re-run) and a fresh pyflatten robust_fast flat on the same patch against a fiducial + chord- sanitized truegeo, each at its own optimal scale. Replaces the inflated- surface true_comparison_20subj.csv numbers. 4-lane, resumable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
--mode config-paired renders robust_fast vs tutte_default as a vertical collapsed raincloud, paired per hemisphere (connector lines), points colored by hemisphere (LH/RH). Same far-outlier zoom + off-scale handling. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
On the corrected fiducial metric the extremes are mild (~22% vs ~14% bulk), so show the full range with no zoom/off-scale carets. _far_fence now returns None (kept as a hook). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Point _fs6_distortion / _fs6_local_perhemi at true_comparison_fiducial.csv (fiducial + chord-sanitized, true_local_at_optscale) instead of the old inflated-surface true_comparison_20subj.csv, so fig_distortion and fig_speed_accuracy overlay FreeSurfer on the same corrected metric as pyflatten. Flip counts (comparison.csv) are metric-independent, unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
fig_distortion_paired connects each hemisphere's two values (pyflatten vs FreeSurfer 6) for local and global distortion, from true_comparison_fiducial .csv (both methods on the SAME patch, fiducial metric, 40 hemis). Shows the per-hemi consistency of relative error across methods; points colored by hemisphere. (Avoids the patch mismatch of connecting cores-pyflatten to comparison-FS in fig_distortion.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Orientation for release: run directories, file layouts, CSV/truegeo/flat schemas, the metric definition, a verified load+score recipe, and the script inventory. Notes the fiducial-surface correction and the pre-fix backups (truegeo_inflated/, *.unfloored.csv) to avoid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Fixes from a high-effort code review of the paper-rerun benchmark scripts: - truedist: raise a clear ValueError instead of an opaque concatenate/ broadcast failure when no source/target pairs survive masking or when the flat patch and truegeo reference have mismatched vertex counts. - truedist.compute_truegeo now stamps surface/sanitized provenance; parameterize true_distortion_full's opt-scale bracket (default unchanged). - time_cores.run_truegeo reuses a cached truegeo.npz only if it is a sanitized fiducial reference (pre-fix inflated refs are recomputed), and unlinks stale seeded symlinks before savez so a shared head-start ref can't be clobbered. - fs6_recompare._score delegates to the shared truedist metric (wide [0.5,2.0] bracket as a parameter) instead of a divergent reimplementation -- verified byte-identical output, so headline numbers are unchanged. Collapse _done's redundant double CSV scan. - recompute_truedist skips a misaligned/empty row instead of aborting the rewrite. - fig_group_raincloud: guard gaussian_kde against degenerate (single/identical) input that triggered a LinAlgError crash. - build_truegeo preserves provenance keys on save. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request parallelizes the angular k-ring kernel in autoflatten/flatten/distance.py over chunks rather than individual vertices, significantly reducing memory allocation overhead across threads. It also introduces a comprehensive benchmark and autoresearch harness under the benchmark/ directory. Feedback on the changes points out that the inlined Dijkstra algorithm in the parallel loop currently uses an inefficient linear scan to find the minimum-distance vertex, and recommends implementing a proper binary heap to achieve O(log N) complexity.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
The parallel _angular_kring_kernel and _kring_distances_kernel described their
priority queue as a "lazy-deletion binary heap", but both reproduce the serial
_limited_dijkstra_numba, whose extract-min is a linear scan ("fast enough for
local neighborhoods"). Reword the comments to describe the actual unsorted-array
linear-scan extract-min. No behavior change.
The linear scan is intentional: this is a *limited* Dijkstra with early
termination over a local k-ring neighborhood (small frontier), and the kernel's
contract is bit-for-bit reproduction of the serial loop -- so a real heap (which
would change extract-min tie-break order) is deliberately not used. Addresses a
Gemini review comment on #71.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01ByqwVTgqZa7tFURYmuxQiL
Why the diff is large (~9.5k lines)
devhas never contained thebenchmark/directory (it lived onbenchmark-autoresearch). This PR introduces the whole benchmark suite todevfor the first time — the pre-existing autoresearch tooling plus the new paper scripts below.What this PR introduces
Timing benchmark
time_cores.py— flatten-only core-scaling (1/8/16 physical cores via CPU affinity; run-local truegeo + k-ring caches).--reprojectre-projects continuity-only into<run>/patches.time_e2e.py— end-to-end (FS-free continuity-only projection + prep + flatten) @16 cores, both hemispheres per subject.Distortion benchmark (true-geodesic, Ju 2005)
truedist.py— the quality yardstick: libigl heat-method geodesics from sampled sources, scored as relative error|d2d − d_geo|/d_geoat each map's distance-optimal scale, reported local (≤30 mm) and global (all pairs). The k-ring energy metric is kept only as a diagnostic.group_flatten.py— distortion-at-scale runner: 50 subjects / 100 hemispheres from Narratives, 4 hemispheres concurrent × 8 CPUs each, resumable; both configs.fs6_recompare.py— re-verifies the FreeSurfer-6-vs-pyflatten comparison by scoring FS's{hemi}.flatand a fresh pyflatten flat on the same patch against the same reference.rebuild_truegeo.py/recompute_truedist.py— rebuild references / rewrite metric columns from saved flats without re-flattening.Figures
fig_paper.py— runtime, core-scaling, distortion, speed/accuracy, and per-hemisphere paired FS6-vs-pyflatten panels.fig_group_raincloud.py— paired LH/RH and config-paired group-distortion rainclouds (median-based).fig_flatmaps.py— per-hemisphere 3-method Fischl-1999 flatmap + histogram.fig_pipeline.py/fig_pipeline_detail.py— pipeline schematics.Critical metric correction (this branch)
The true-geodesic reference was originally computed on the inflated surface (what
patch.3dstores), where the heat solver ill-conditions and emits spurious near-zero/negative geodesics — blowing the relative metric to 200–350% on a handful of hemispheres. Fixed to compute on the fiducial surface (the surface the energy preserves), with ageodesic ≥ ½·chordsanitizer for residual solver failures. All prior true-geodesic numbers were regenerated on the corrected reference. The FS-vs-pyflatten verdict is unchanged by the fix (same reference scores both methods).Robustness hardening (code review)
truegeo provenance stamping + fiducial-only cache reuse (a pre-fix inflated
.npzcan no longer be silently reused); clear errors on empty/misaligned references;fs6_recomparenow delegates to the sharedtruedistmetric instead of a divergent reimplementation (verified byte-identical); degenerate-input guard on the raincloud KDE; per-row resilience in the CSV recompute.Method
robust_fast(default; k7/n6, line-search 7, ~3× faster) andtutte_default(k7/n12, quality-first).Headline results (corrected fiducial metric)
robust_fastlocal median 14.3% / global 12.2%;tutte_default14.2% / 12.1% — configs ~tied, LH/RH symmetric, max local ~22% (genuine).robust_fast) / ~14.8 min (tutte_default) per brain @16c vs FreeSurfer 6 ~181 min (≈12–19×); sublinear core scaling (latency-bound optimizer).robust_fastPareto-dominates (parity on distortion, ~3× faster thantutte_default).Artifacts (not in repo)
/data2/projects/autoflatten/paper_bench_2026/— core-scaling20260617-123120(+e2e_…), groupgroup_20260624-060144, geodesic-refined/presentation20260614-105856;fs6_compare/true_comparison_fiducial.csv. Each run dir holds timestamped CSVs,run_meta.json, figures (PDF + 300-dpi PNG), per-hemi flatmaps, and truegeo references. Seebenchmark/OUTPUTS.mdfor the full layout, schemas, and a verified reload-and-score recipe.TODO before un-drafting
devvsbenchmark-autoresearchas the home forbenchmark/.npz, k-ring caches) to publish vs regenerate for release