Run the topology fix single-threaded for reproducibility, and stop reporting success when a step failed

[m-reuter]

Three independent fixes, found while investigating two near-identical runs of one subject that
produced different surfaces. Also unifies how the two surface scripts read --threads, and adds a
tool to compare runs by content.

• Topology fix pinned to one thread. mris_fix_topology repairs defects in an order-dependent
  way. At --threads 4 the first differing file is lh.orig.premesh, which it writes (133836 vs
  133966 vertices, Left-Hippocampus off by 0.5%), while every input to the stage, all transforms
  and the whole rh side are bit-identical. Costs ~266 s of a 48.6 min pipeline on linux, nothing
  on macOS.
• One thread rule for both surface scripts. --threads is a total budget for surfaces: from 2
  up, the hemispheres run at the same time and split it. Surface default moves 1 → 2. --parallel
  no longer changes the thread count, forces parallel hemispheres at --threads 1, and is no longer
  obsolete. recon-surfreg.sh previously ran hemispheres sequentially at --threads 2.
• Two exit-path bugs in run_fastsurfer.sh. [[ ]] overwrites PIPESTATUS, so the license
  check and a failed segstats subshell both exited 0 on failure. Same defect as c747812. Plus
  --long "$basedir", a variable this script never sets.
• Spherical projection fallback tidied. No observed failure: a space-padded " -hemi ", a
  -threads that undid the pinning above it, None reaching Popen, and ITK not pinned alongside
  OMP and BLAS.
• recon-surfreg.sh quoting and arrays. 67 expansions quoted, CMDFS becomes a real array, a
  stale bash 4 guard removed. Reproduces the full run bit-identically; 47 → 34 min on macOS.
• Version in the logs. VERSION='$Id$' was never expanded by git, so every recon-surf.log and
  .done file carried that literal text. Both scripts now read the project version, falling back to
  unknown.
• recon_surf/make_upright deleted as dead code. long_prepare_template.sh calls it unqualified
  and nothing puts recon_surf on PATH, so FreeSurfer's copy has been running all along
  (command -v confirms /opt/freesurfer/bin/make_upright). Behaviour-neutral, and neuroreg retires
  the step. Note the deleted copy had silently diverged: it dropped --halfmov for a
  mri_vol2vol --no-resample step, avoiding an interpolation pass, and capped iterations with
  --highit 1 --maxit 3. Those unused improvements are worth carrying into the neuroreg
  replacement rather than losing.
• tools/compare_subjects.py. Compares voxels, vertices, transforms, stats and labels rather
  than bytes, since headers record timestamps and the command line.

Release note needed (no changelog file in the repo): the surface default changes from 1 to 2
threads, --threads is a total budget so N-per-hemisphere callers should pass 2N, --parallel
is undeprecated, and --threads > 2 now yields slightly different surfaces.

Verified: two independent runs at --threads 4 with the pin in place are identical, which is
the direct demonstration that the pin works. Two runs at the new default of --threads 2 are
identical in all 169 files compared, while 76 of those differ byte for byte. bash -n clean under
3.2 and 5.3, ruff clean, 84 shell tests pass.

[Copilot]

🟡 Changes recommended

The unresolved ITK threading and geometry-comparison issues must be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Pins topology repair for reproducibility, improves failure propagation, and adds semantic output comparison.

Changes:

• Pins topology correction and projection threading.
• Fixes shell exit-status and longitudinal path handling.
• Adds reproducibility documentation and comparison tooling.

File summaries

File	Review
tools/compare_subjects.py	Adds semantic comparison. Moderate (2 votes): one-sided missing geometry metadata can be incorrectly ignored.
run_fastsurfer.sh	Updates defaults and error handling. Nit (2 votes): update the outdated example and add the stated release note.
recon_surf/spherically_project_wrapper.py	Pins fallback threading. Moderate (1 vote): also override inherited ITK thread settings.
recon_surf/recon-surfreg.sh	Aligns threading and quoting behavior.
recon_surf/recon-surf.sh	Pins topology correction to one thread.
doc/scripts/RUN_FASTSURFER.md	Documents threading and reproducibility expectations.

Review details

Suppressed comments (3)

tools/compare_subjects.py:230

• When a stats row has a different number of numeric columns, zip(..., strict=False) silently drops the unmatched values. The comparator can therefore report identical stats—and exit 0—even though one file contains an added or missing measurement. Compare the list lengths before comparing their shared values.

        for x, y in zip(va[key], vb[key], strict=False):

tools/compare_subjects.py:360

• This second pass ignores each group's patterns and router, so an unsupported file or subdirectory that exists only in B is reported as a difference, while the same unsupported entry only in A is skipped above. Filter B-side candidates through the same patterns and route(name) logic to keep the comparison symmetric.

    for group, _, _ in GROUPS:
        if not (b_dir / group).is_dir():
            continue
        extra = {p.name for p in (b_dir / group).glob("*")} - {p.name for p in (a_dir / group).glob("*")}
        only_one_side.extend(f"{group}/{name} (only in B)" for name in sorted(extra))

tools/compare_subjects.py:293

• With --subject, this returns an unchecked path. If the requested subject is absent from both roots, all group loops are skipped and the tool prints 0 files compared before exiting successfully, which can falsely certify invalid inputs as identical. Validate that the selected subject has an mri directory before returning it.

    if subject:
        return root / subject

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Balanced

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Three moderate issues remain in default threading behavior and subject-comparison correctness.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

tools/compare_subjects.py:114

• If two surfaces have the same vertex count but different face counts, fa != fb cannot broadcast and raises ValueError. The outer handler then reports only “could not compare” instead of the face-count difference (and loses any coordinate/geometry details). Check the face-array shape before elementwise comparison.

run_fastsurfer.sh:92

• This also changes the no-flag surface default from 1 to 2, so the PR description's claim that only callers using --threads > 2 see a behavior change is incomplete. Default runs now execute hemispheres concurrently and give the pre/post-hemisphere stages two threads (as the new reproducibility docs acknowledge), changing CPU usage and potentially outputs. Please either retain the previous default or explicitly include this default-path change in the release notes/validation scope.

# 2, so the surface pipeline runs the two hemispheres at the same time with one thread each by
# default. recon-surf.sh is always called with --threads "$threads_surf", so its own default of 2
# would never be reached otherwise.
threads_surf="2"

• Files reviewed: 7/7 changed files
• Comments generated: 1
• Review effort level: Balanced