Skip to content

Add fix_rbc_release_tr.py for the C-PAC TR bandpass bug#343

Merged
nx10 merged 3 commits into
mainfrom
scripts/fix-rbc-release-tr
May 28, 2026
Merged

Add fix_rbc_release_tr.py for the C-PAC TR bandpass bug#343
nx10 merged 3 commits into
mainfrom
scripts/fix-rbc-release-tr

Conversation

@nx10
Copy link
Copy Markdown
Contributor

@nx10 nx10 commented May 22, 2026

Fixes the TR/bandpass bug (divergence #4) on a downloaded RBC release without re-running upstream.

For each functional run: reads the correct TR from the native desc-preproc_bold.nii.gz, patches the template-space desc-head_bold.nii.gz header (released with pixdim[4]=0.0, which AFNI silently coerces to 1.0 and drives the bandpass off by 2x), and re-runs nuisance regression + bandpass.

The bad bandpass propagates to all downstream metrics, so the script also regenerates ALFF/fALFF/ReHo (desc-sm6, desc-smZstd, desc-zstd), atlas mean timeseries, and Pearson + Nilearn partial-correlation matrices under the release's filename conventions. --skip-metrics opts out.

Notes:

  • Atlas .1D files written (T, n_rois) to match the release.
  • Atlas resolution goes through ATLAS_REGISTRY short names.
  • Schaefer2018p300n17 is skipped: upstream atlas has 619 labels instead of 300.
  • _stage_mask lifts the BOLD's sform/qform/xyzt_units onto the mask copy so AFNI's grid check passes (release ships masks with sform_code=0).
  • RBC_WORK_DIR scoped to a tempdir so niwrap exec folders don't accumulate.
  • _restore_tr inlined so the script doesn't need a private rbc symbol exported.

Validated end-to-end on sub-10612 (BHRC, 16 atlases, Docker): atlas ROI counts match the release exactly, cleaned BOLD RMS-diffs from the release by 4.7× of the original signal.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 22, 2026

Coverage

Tests Skipped Failures Errors Time
808 8 💤 0 ❌ 0 🔥 11.758s ⏱️

@nx10 nx10 force-pushed the scripts/fix-rbc-release-tr branch 3 times, most recently from 2144e0e to 9ac150e Compare May 22, 2026 20:24
Copy link
Copy Markdown
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A quick skimmed review, largely looks good. I think mostly checking against known entities currently (sub, ses, run, task) and defensive against space. Is it possible (for the datasets these are intended for) to encounter an entity that isn't accounted for?

Comment thread scripts/fix_rbc_release_tr.py
@nx10 nx10 force-pushed the scripts/fix-rbc-release-tr branch 4 times, most recently from 158b55c to 7f4b3eb Compare May 22, 2026 22:53
@nx10 nx10 force-pushed the scripts/fix-rbc-release-tr branch 2 times, most recently from 140f360 to c2b2e65 Compare May 23, 2026 00:30
Lets users of an RBC data release re-derive a correctly-bandpassed
cleaned BOLD and all downstream metrics, by patching the template-space
`desc-head_bold.nii.gz` header (TR=0.0 on disk) with the TR read from
the native-space `desc-preproc_bold.nii.gz` before regression.

Outputs land under a parallel `--output-dir` tree using the published
release's filename conventions:

  * cleaned BOLD + bandpass-filtered regressors
  * ALFF / fALFF / ReHo at `desc-sm6`, `desc-smZstd`, `desc-zstd`
  * Atlas-based mean timeseries (`.1D`, `(n_timepoints, n_rois)`)
  * Pearson + nilearn partial-correlation matrices, per atlas

Metrics compose `rbc.workflows.metrics.single_session_metrics` and
`rbc.core.metrics.standardization.compute_zscore`. Atlas resolution
goes through `rbc_resources.ATLAS_REGISTRY` so the release-name -> file
mapping survives filename changes inside `rbc_resources`.

The script ships with PEP 723 inline metadata so it can be invoked
standalone via `uv run --script` without a clone. `--skip-metrics`
falls back to BOLD-only behavior. `--verify` scans every discoverable
run (header-only reads) and reports buggy / clean / inconclusive
counts. `RBC_WORK_DIR` is scoped to the script's tempdir for the
duration of the run so all niwrap exec folders get cleaned up.

The TR-restore helper is inlined in the script (mirrors
`rbc.core.functional.resampling._restore_tr`) so standalone PEP 723
runs against an unmodified `main` don't trip an ImportError on a
symbol that isn't yet public.
@nx10 nx10 force-pushed the scripts/fix-rbc-release-tr branch from c2b2e65 to f1785bc Compare May 23, 2026 00:33
Copy link
Copy Markdown
Contributor

@kaitj kaitj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm 🚀just the one comment about switching back to the import, but not blocking.

Comment thread scripts/fix_rbc_release_tr.py
Existing file permissions were copied alongside the file itself, causing issues with
overwrriting existing files even when in an temporary directory if user executing
script did not have the appropriate permissions.

This switches the `copy2` method used to `copyfile` which does not have this issue
@kaitj
Copy link
Copy Markdown
Contributor

kaitj commented May 28, 2026

@nx10 029ab37 switches all shutil.copy2 to shutil.copyfile to handle file permission issues when copied (instead of copying original file's permission, will copy with 0o644 permissions). Let me know if this is what you had in mind.

@nx10 nx10 merged commit aa927ba into main May 28, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants