Functional initialization & motion correction by jpillai00 · Pull Request #39 · childmindresearch/rbc

jpillai00 · 2026-02-05T21:11:52Z

Starting functional workflow implementation addressing #16 & #17 -- sets up core initialization and motion reference + correction. Still need to add testing for these.

Currently, the workflow saves all intermediate outputs but I'm assuming we don't need all of that saved?

kaitj

This will need a rebase from main to pull in all the changes + linting / formatting.

kaitj · 2026-02-06T14:58:51Z

+    )
+
+def motion_correction(
+        in_file: Path, ref_file: Path, output_prefix: str


I think there might be an extra tab in this line.

kaitj · 2026-02-06T15:08:51Z

+
+    func_out_dir = output_dir / bids(datatype="func", directory=True)
+
+    save_directory(


So would this save the directory as <directory>/sub-<subject>[/ses-<session>]/func/sub-<subject>_[ses-<session>]_desc-motion_mat/?

While nothing inherently wrong with this AFAIK with respect to bids derivatives, we would still want the files within the folder to be BIDS-esque for the end user (e.g. file names should be somewhat bids compliant). Alternatively, could just save the files directly into the output folder. Can chat more about this offline if this comment is confusing!

Currently, inside /func, the directory is named sub-<subject>_ses-<session>_motionMatrices. The matrices are saved inside as MAT_0000, MAT_0001, etc., with a matrix for each volume (this is just the default naming from mcflirt I think). I thought saving them to a directory would clutter the /func folder a bit less, but if saving them directly into the output is better (with something like sub-<subject>_[ses-<session>]_desc-motion_mat0000), that works too!

No I think this makes sense then. If its 1 matrix per volume having the folder is better than saving N number of matrices in the parent folder.

kaitj · 2026-02-06T15:44:34Z

Also realized I missed your question:

Currently, the workflow saves all intermediate outputs but I'm assuming we don't need all of that saved?

We won't need to save everything, only what is important in the final output directory (@nx10 do we still have that list or I can find it later).

The way I've been going about it is treating runner.data_dir as a temporary working directory, with files essentially being "removed" once the workflow is completed, hence the need to save the files to a final output directory with bids naming. This just requires less overall disk space. I think we'll want an option to keep the intermediate directory for debugging purposes though.

jpillai00 · 2026-02-08T23:34:09Z

Added unit + integration tests for this

Also do we even want the transformation matrices saved after motion correction? I added it because it was in the reimplementation guide, but it's not in the output spreadsheet.

@nx10 let me know if we don't need it and I'll remove from the workflow. otherwise, this should be ready for review

nx10 · 2026-02-09T00:49:41Z

Amazing! I'll have a closer look at this tomorrow, but re transform matrices:

As far as I remember cpac/fmriprep save these as massive voxel-wise xfm nifti files which we want to avoid (as the affine transform matrices are much smaller). So I believe as you have them right now is exactly what we what we want.

kaitj · 2026-02-09T14:51:13Z

+    motion_mat_dir = [
+        d for d in Path(mc_result.root).iterdir() if d.is_dir() and d.suffix == ".mat"
+    ]
+
+    return SimpleNamespace(
+        bold=Path(mc_result.out_file),
+        par=Path(mc_result.par_file),
+        rms_rel=Path(mc_result.rmsrel_files),
+        rms_abs=Path(mc_result.rmsabs_files),
+        mat_dir=motion_mat_dir[0],
+    )


Suggested change

motion_mat_dir = [

d for d in Path(mc_result.root).iterdir() if d.is_dir() and d.suffix == ".mat"

]

return SimpleNamespace(

bold=Path(mc_result.out_file),

par=Path(mc_result.par_file),

rms_rel=Path(mc_result.rmsrel_files),

rms_abs=Path(mc_result.rmsabs_files),

mat_dir=motion_mat_dir[0],

)

motion_mat_dir = None

for d in Path(mc_result.root).iterdir():

if d.is_dir() and d.suffix == ".mat":

motion_mat_dir = d

break

return SimpleNamespace(

bold=Path(mc_result.out_file),

par=Path(mc_result.par_file),

rms_rel=Path(mc_result.rmsrel_files),

rms_abs=Path(mc_result.rmsabs_files),

mat_dir=motion_mat_dir,

)

Two minor suggestions here:

Break out of the loop once the first matrix is found; this step is probably quite quick anyways even without the break, but 🤷‍♂️

This change also ensures there is not an index error if the motion_mat_dir list is empty. That being said, we probably want to raise some sort of error if there is no corresponding directory?

kaitj · 2026-02-09T15:10:45Z

As far as I remember cpac/fmriprep save these as massive voxel-wise xfm nifti files which we want to avoid (as the affine transform matrices are much smaller). So I believe as you have them right now is exactly what we what we want.

This is only true if the transforms are only linear (saving the affines only; which I think they were, but we should double check)

Co-authored-by: Jason Kai <21226986+kaitj@users.noreply.github.com>

nx10 · 2026-02-09T16:37:56Z

Once you've rebased/merged main and addressed all previous issues, mark the branch as ready for review and I'll take a look.

github-actions · 2026-02-09T21:24:44Z

Coverage Report

File	Stmts	Miss	Cover	Missing
src/rbc
__init__.py	0	0	100%
src/rbc/cli
__init__.py	0	0	100%
anatomical.py	0	0	100%
derivatives.py	0	0	100%
functional.py	0	0	100%
longitudinal.py	0	0	100%
qc.py	0	0	100%
src/rbc/core
__init__.py	1	0	100%
bids.py	362	0	100%
common.py	7	0	100%
utils.py	33	7	78%	53, 67–68, 70–71, 73–74
src/rbc/core/anatomical
__init__.py	3	0	100%
registration.py	10	4	60%	25, 117, 132, 147
segmentation.py	10	4	60%	23, 45, 52, 63
src/rbc/core/functional
__init__.py	3	0	100%
initialization.py	6	1	83%	40
motion.py	17	9	47%	43, 53–57, 59–60, 62
src/rbc/core/longitudinal
__init__.py	0	0	100%
src/rbc/core/qc
__init__.py	0	0	100%
src/rbc/core/resources
__init__.py	8	0	100%
src/rbc/workflows
anatomical.py	15	8	46%	27–28, 30, 34, 37, 40, 45, 55
TOTAL	475	33	93%

Tests	Skipped	Failures	Errors	Time
47	0 💤	0 ❌	0 🔥	1m 13s ⏱️

nx10 · 2026-02-09T20:53:05Z

These tests dont actually test any functions in this codebase, please remove.

nx10 · 2026-02-09T21:35:15Z

+    new_info = afni.v_3dinfo(dataset=[truncated_bold.output_file], nv=True)
+    assert int(new_info.info[0]) == original_count - start_tr


Just use nibabel here to read and inspect the header

Same goes for all instances of afni.v_3dinfo below

nx10 · 2026-02-09T21:41:30Z

+    return SimpleNamespace(
+        bold=Path(mc_result.out_file),
+        par=Path(mc_result.par_file),
+        rms_rel=Path(mc_result.rmsrel_files),
+        rms_abs=Path(mc_result.rmsabs_files),
+        mat_dir=motion_mat_dir,
+    )


Use a NamedTuple - I know there are more instances of SimpleNamespace used like this in the codebase right now, its on my todo list to remove them. SimpleNamespace is really more intended for dynamic output patterns and is quite bad for static type checking

nx10 · 2026-02-09T21:44:14Z

+def save_directory(in_dir: str | Path, out_dir: str | Path, name: str) -> Path:
+    """Save a directory by copying it to a new location.
+
+    Args:
+        in_dir: Path to directory to copy.
+        out_dir: Path to destination directory.
+        name: BIDS compliant name for the directory.
+    """
+    in_dir = Path(in_dir)
+    target = Path(out_dir) / name
+
+    if not in_dir.is_dir():
+        raise ValueError(f"Input path {in_dir} is not a directory.")
+
+    shutil.copytree(in_dir, target, dirs_exist_ok=True)
+    return target


Why do I need this when I could just use shutil.copytree ? for the input directory check?

nx10 · 2026-02-09T21:47:03Z

 testpaths = ["tests"]
 markers = [
-  "unit: Fast (<1s) unit tests (e.g. utilities, BIDS parsing, file operations",
+  "unit: Fast (<1s) unit tests (e.g. utilities, BIDS parsing, file operations)",


Suggested change

"unit: Fast (<1s) unit tests (e.g. utilities, BIDS parsing, file operations)",

"unit: Fast (<1s) unit tests (e.g. utilities, BIDS parsing, file operations",

Correct, but unrelated change - separate PR

nx10 · 2026-02-09T21:48:01Z

+    Returns:
+        AFNI 3dcalc output object.
+    """
+    total_vols = afni.v_3dinfo(dataset=[in_file], nv=True)


Use nibabel to read the header

nx10 · 2026-02-09T21:49:44Z

+    motion_mat_dir = None
+    for d in Path(mc_result.root).iterdir():
+        if d.is_dir() and d.suffix == ".mat":
+            motion_mat_dir = d
+            break


I dont think we need to scan here, better match the output file directly with something like Path(mc_result.root) / f"{output_prefix}.mat"

(if need be hardcode the prefix it produces)

nx10 · 2026-02-09T21:53:17Z

+from rbc.core import functional
+from rbc.core.common import reorient


prefer absolute imports here

nx10 · 2026-02-09T21:54:46Z

+    )
+
+
+def scale(in_file: Path, scale_factor: float = 0.1) -> afni.V3drefitOutputs:


Rename to something more descriptive, e.g. scale_bold

* add nibabel and fix syntax in pyproject.toml * parantheses in pyproject.toml * Functional initialization & motion correction (#39)

jpillai00 requested review from kaitj and nx10 February 5, 2026 21:11

kaitj reviewed Feb 6, 2026

View reviewed changes

jpillai00 force-pushed the enh/functional branch from ba09bfa to 9f37771 Compare February 6, 2026 18:35

childmindresearch deleted a comment from codecov Bot Feb 7, 2026

kaitj reviewed Feb 9, 2026

View reviewed changes

jpillai00 and others added 18 commits February 9, 2026 11:34

Added functional initialization, workflow, test script

165466a

Add truncate_trs and updated func workflow

4c86557

fixed truncated_trs and func workflow with niwrap v0.8.5

a4cee6f

added generate_motion_reference and updated func workflow

9469ce2

remove commented out motion_correction

84a7333

add save_directory to utils

85b959b

added motion_correction

7d7a5e1

updated func workflow with motion_correction and renaming

e604285

update workflow outputs and motion_correction

66f3b4f

fix motion mats directory naming

e1565c0

Delete test_functional.py

a19670f

Update src/rbc/core/functional/motion.py

d963997

Co-authored-by: Jason Kai <21226986+kaitj@users.noreply.github.com>

update docstrings for completeness

ec759b4

include task in get_base_entities

cb614fd

rebase from main and format for func files

cf931f7

fix docstring motion_correction

e99e6af

added init and motion correction tests

a0b7554

added motionParams txt to output

6aa15dc

update core utils

0365e83

jpillai00 force-pushed the enh/functional branch from d361182 to e51a1ea Compare February 9, 2026 17:08

jpillai00 added 8 commits February 9, 2026 12:20

update rename params

af9f027

update rename params again

384d781

update motion_mat_dir searching

365c0a6

ruff for motion

16354b8

fix utils and func unit tests

82440c6

ruff tests

1d966b0

func tests again

2c13e91

mark motion_correction tests slow

d461c85

jpillai00 marked this pull request as ready for review February 9, 2026 20:42

kaitj force-pushed the enh/functional branch from 15e65e5 to 4aa30a9 Compare February 9, 2026 21:32

nx10 requested changes Feb 9, 2026

View reviewed changes

jpillai00 added 8 commits February 9, 2026 19:22

rm func unit tests

3458feb

rename to scale_bold in initialization

a35c0b5

use nibabel for header & fix imports

59c0b2f

use namedtuple in motion_correction & match pyproject.toml

bae12e1

remove save_directory from utils

13aa5c8

refactor workflow with bids.py naming & tests to use .shape

7bad426

match pyproject.toml to main

4daa1d0

fix nib type hinting for mypy

92866a8

jpillai00 force-pushed the enh/functional branch from 4aa30a9 to 92866a8 Compare February 10, 2026 13:51

reset pyproject to match main without format

4ff0e13

jpillai00 force-pushed the enh/functional branch from 702e376 to 4ff0e13 Compare February 10, 2026 16:23

nx10 changed the base branch from main to add-nib-and-fix-syntax February 10, 2026 18:17

nx10 merged commit 5e9083e into add-nib-and-fix-syntax Feb 10, 2026
2 of 6 checks passed

nx10 deleted the enh/functional branch February 10, 2026 18:18

nx10 pushed a commit that referenced this pull request Feb 10, 2026

Functional initialization & motion correction (#51)

a277c68

* add nibabel and fix syntax in pyproject.toml * parantheses in pyproject.toml * Functional initialization & motion correction (#39)

This was referenced Feb 10, 2026

Implement Functional Initialization #16

Closed

Implement Motion Correction #17

Closed


		func_out_dir = output_dir / bids(datatype="func", directory=True)

		save_directory(

		new_info = afni.v_3dinfo(dataset=[truncated_bold.output_file], nv=True)
		assert int(new_info.info[0]) == original_count - start_tr

	"unit: Fast (<1s) unit tests (e.g. utilities, BIDS parsing, file operations)",
	"unit: Fast (<1s) unit tests (e.g. utilities, BIDS parsing, file operations",

		from rbc.core import functional
		from rbc.core.common import reorient

		)


		def scale(in_file: Path, scale_factor: float = 0.1) -> afni.V3drefitOutputs:

Conversation

jpillai00 commented Feb 5, 2026

Uh oh!

kaitj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kaitj commented Feb 6, 2026

Uh oh!

jpillai00 commented Feb 8, 2026

Uh oh!

nx10 commented Feb 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kaitj commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nx10 commented Feb 9, 2026

Uh oh!

github-actions Bot commented Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kaitj commented Feb 9, 2026 •

edited

Loading

github-actions Bot commented Feb 9, 2026 •

edited

Loading