Test dMRI and field map file grouping by tsalo · Pull Request #927 · PennLINC/qsiprep

tsalo · 2025-02-25T17:15:19Z

Closes none.

Changes proposed in this pull request

Write tests to cover field map collection and DWI scan grouping, using mocked up datasets.
Modify the utility function group_dwi_scans to accept the BIDSLayout as a parameter instead of relying on the config object. This makes it easier to test. Also the nipreps folks only use the config in workflow functions. They tend to pass fields from it into interfaces and utility functions.

tsalo · 2025-08-05T14:12:18Z

I can drop the BIDS-URI tests for now so we at least get these tests in.

I'll just modify everything so it passes and then we can dig into why the results are the way they are.

tsalo · 2025-08-05T18:27:36Z

-        fname: meta.get('PhaseEncodingDirection', 'None')
+        fname: meta.get('PhaseEncodingDirection', None)


This is potentially a meaningful change.

codecov-commenter · 2025-08-05T19:21:07Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 46.12%. Comparing base (317828f) to head (c874de4).
⚠️ Report is 28 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #927      +/-   ##
==========================================
+ Coverage   46.02%   46.12%   +0.10%     
==========================================
  Files          65       65              
  Lines        9764     9764              
  Branches     1074     1074              
==========================================
+ Hits         4494     4504      +10     
+ Misses       5044     5036       -8     
+ Partials      226      224       -2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tsalo · 2025-08-06T17:51:44Z

+    scan_groups, _ = grouping.group_dwi_scans(
+        layout=layout,
+        subject_data=subject_data,
+        using_fsl=True,


@mattcieslak said to remove using_fsl. It's always True(?), so use that.

Commented in #923.

tsalo · 2025-08-06T17:59:08Z

+        using_fsl=True,
+        combine_scans=True,
+        ignore_fieldmaps=False,
+        concatenate_distortion_groups=True,


Tied to combine_before_denoising. If True, will concatenate across distortion groups before MP-PCA.

This should be respecting MultipartID (concatenate AP run-01 and AP run-02, but not PA) but isn't.

RPE DWI scans are higher priority for distortion correction than EPI fmaps, so it makes sense that we have selected the rpe_series for SDC.

However, the epi field maps that are collected are not correct. The IntendedFor fields I set should only use the AP EPI for the PA DWI. The PA EPI does not have the PA DWI in its IntendedFor.

We need to enumerate the possible combinations of metadata fields and what the resulting processing should be. For example, should MultipartID be limited to scans with the same parameters, or can we use it to determine which runs should be concatenated, even across distortion groups?

And if MultipartID should only be applied within distortion groups, then how should users specify which runs should be concatenated?

Opened #963 and #964.

tsalo · 2025-08-06T18:15:20Z

+    generate_bids_skeleton(str(bids_dir), dset_yaml)
+    layout = BIDSLayout(str(bids_dir))
+    subject_data = {'dwi': layout.get(suffix='dwi', extension='nii.gz', return_type='file')}
+    scan_groups, _ = grouping.group_dwi_scans(


We need to add a grouping parameter reflecting if DRBUDDI is used. TOPUP allows various different phase encoding directions (e.g., dir-AP, dir-RL, and dir-SI are a valid combination), but DRBUDDI only allows two reverse-PEDs (dir-AP and dir-PA are solid, but don't throw dir-RL into the mix). This needs to be reflected in the groups.

Opened #962.

tsalo · 2025-08-06T18:21:24Z

+            'fieldmap_info': {
+                'epi': [
+                    'sub-01_dir-AP_epi.nii.gz',
+                    'sub-01_dir-PA_epi.nii.gz',


This shouldn't be here.

tsalo · 2025-08-06T18:22:27Z

+                    'sub-01_dir-AP_run-1_dwi.nii.gz',
+                    'sub-01_dir-AP_run-2_dwi.nii.gz',
+                ],
+                'suffix': 'rpe_series',


Conclusion: Build dictionary around the selected distortion correction suffix, so there's no need for a second dictionary for the AP scans linking to the PA fmap EPI. HOWEVER, we do need to prioritize supporting B0Field* fields in order to give users control over what is chosen.

tsalo · 2025-08-06T18:26:47Z

+        ignore_fieldmaps=False,
+        concatenate_distortion_groups=False,
+    )
+    expected = [


@mattcieslak: This one is clearly an error. Amelie probably used this option recently, and it worked at the time so this might just be weird.

tsalo · 2025-08-06T18:28:05Z

+    ]
+    check_expected(scan_groups, expected)
+
+    scan_groups, _ = grouping.group_dwi_scans(


This is probably what Amelie had.

Weirdly, using_fsl is hardcoded as True in QSIPrep so 🤷

tsalo · 2025-08-06T18:29:01Z

+        subject_data=subject_data,
+        using_fsl=True,
+        combine_scans=True,
+        ignore_fieldmaps=True,


This means ignore distortion correction, not just ignore field maps. Should rename, but logic makes sense.

--ignore fmaps in the CLI apparently ignores all distortion correction, including reverse PED DWI runs. Let's change that!

Opened #965.

Test field map collection.

c8cbac2

tsalo added the testing label Feb 25, 2025

tsalo added 6 commits February 25, 2025 12:26

Update test_utils_grouping.py

d538f36

Update test_utils_grouping.py

706e7f1

Update test_utils_grouping.py

1c91eab

Update test_utils_grouping.py

fd5b49a

Update test_utils_grouping.py

e8bcc12

Update test_utils_grouping.py

d390544

tsalo commented Feb 26, 2025

View reviewed changes

Comment thread qsiprep/tests/test_utils_grouping.py Outdated

tsalo commented Feb 26, 2025

View reviewed changes

Comment thread qsiprep/tests/test_utils_grouping.py Outdated

tsalo added 8 commits February 26, 2025 13:04

Work on complex grouping tests.

e8ac23c

Update test_utils_grouping.py

fec9ef0

Update test_utils_grouping.py

30437f6

Update test_utils_grouping.py

292fba1

Update grouping.py

7d95696

Merge branch 'master' into bids-uris

0a21d7e

Update grouping.py

81edc29

Update.

957f478

tsalo mentioned this pull request Jul 31, 2025

QSIPrep can not read IntendedFor fields using the bids URL. bids:: format! #957

Closed

tsalo added 3 commits August 5, 2025 10:13

Merge branch 'master' into bids-uris

beb1e83

Comment out failing portion of test.

6c7632a

try to clarify test expectations (still failing)

7dacd7c

tsalo commented Aug 5, 2025

View reviewed changes

Comment thread qsiprep/tests/test_utils_grouping.py

tsalo added 6 commits August 5, 2025 10:53

Skip failing test.

4ec58b7

Try printing more.

623a677

Default should be None, not 'None'.

17ce24f

Update grouping.py

1640445

Use return_type='file' in tests.

c49e9b7

Change expected results.

e440e85

I'll just modify everything so it passes and then we can dig into why the results are the way they are.

tsalo added 5 commits August 5, 2025 13:21

Update test_utils_grouping.py

101a874

Update test_utils_grouping.py

572322f

Update test_utils_grouping.py

56a5744

Update test_utils_grouping.py

74529be

Update test_utils_grouping.py

e9c238a

tsalo commented Aug 5, 2025

View reviewed changes

Drop unneeded messages.

c874de4

tsalo requested a review from mattcieslak August 5, 2025 18:40

tsalo marked this pull request as ready for review August 5, 2025 18:40

tsalo changed the title ~~Test field map collection~~ Test dMRI and field map file grouping Aug 5, 2025

tsalo commented Aug 6, 2025

View reviewed changes

tsalo mentioned this pull request Aug 6, 2025

EPI field map collection is too aggressive- may ignore IntendedFor field #964

Closed

mattcieslak approved these changes Aug 6, 2025

View reviewed changes

tsalo merged commit a826be8 into master Aug 6, 2025
24 checks passed

tsalo deleted the bids-uris branch August 6, 2025 18:59

tsalo mentioned this pull request Nov 14, 2025

Add information on BIDS curation to Usage page #1001

Merged

		fname: meta.get('PhaseEncodingDirection', 'None')
		fname: meta.get('PhaseEncodingDirection', None)

Uh oh!

Conversation

tsalo commented Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes proposed in this pull request

Uh oh!

Uh oh!

Uh oh!

tsalo commented Aug 5, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

tsalo Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tsalo Aug 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tsalo commented Feb 25, 2025 •

edited

Loading

codecov-commenter commented Aug 5, 2025 •

edited

Loading

tsalo Aug 6, 2025 •

edited

Loading

tsalo Aug 6, 2025 •

edited

Loading