[ENH] Allow .bval and .bvec files for pepolar fmaps

[mattcieslak]

Closes #1724

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.93%. Comparing base (01025da) to head (7370a63).

❗ Current head 7370a63 differs from pull request most recent head 389ab29. Consider uploading reports for the commit 389ab29 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1754   +/-   ##
=======================================
  Coverage   87.92%   87.93%           
=======================================
  Files          16       16           
  Lines        1375     1351   -24     
=======================================
- Hits         1209     1188   -21     
+ Misses        166      163    -3     

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

[tsalo]

I think the other approach I mentioned (adding .bval and .bvec to pepolar and override the extensions in pepolar_m0scan) might be easier to integrate in the schema.

EDIT: Can you also update the derivative rules for field maps?

bids-specification/src/schema/rules/files/deriv/preprocessed_data.yaml

Lines 100 to 105 in bd08602

	fmap_pepolar_common:
	$ref: rules.files.raw.fmap.pepolar
	entities:
	$ref: rules.files.raw.fmap.pepolar.entities
	space: optional
	description: optional

[tsalo]

Thanks! I think you just need to address the issues in the remark job (misaligned bars in the contributors table and trailing space from my suggestion).

[tsalo]

The change looks good to me!

[effigies]

Started having a look yesterday. It's not clear why this isn't showing up in the rendered examples.

[tsalo]

I'm seeing .bval and .bvec in the filename templates for field map case 4: https://bids-specification--1754.org.readthedocs.build/en/1754/modality-specific-files/magnetic-resonance-imaging-data.html#case-4-multiple-phase-encoded-directions-pepolar

Should it be showing up somewhere else?

[effigies]

Ah, no. Sorry, it turned out I opened the wrong PR's render.

[effigies]

Overall this LGTM. It would be good to update/duplicate these checks:

bids-specification/src/schema/rules/checks/dwi.yaml

Lines 4 to 44 in bd08602

	DWIVolumeCount:
	issue:
	code: VOLUME_COUNT_MISMATCH
	message: |
	The number of volumes in this scan does not match the number of volumes in the
	associated '.bvec' and '.bval' files.
	level: error
	selectors:
	- suffix == "dwi"
	- '"bval" in associations'
	- '"bvec" in associations'
	- type(nifti_header) != "null"
	checks:
	- associations.bval.n_cols == nifti_header.dim[4]
	- associations.bvec.n_cols == nifti_header.dim[4]
	# 30
	DWIBvalRows:
	issue:
	code: BVAL_MULTIPLE_ROWS
	message: |
	'.bval' files should contain exactly one row of values.
	level: error
	selectors:
	- suffix == "dwi"
	- '"bval" in associations'
	checks:
	- associations.bval.n_rows == 1
	# 31
	DWIBvecRows:
	issue:
	code: BVEC_NUMBER_ROWS
	message: |
	'.bvec' files should contain exactly three rows of values.
	level: error
	selectors:
	- suffix == "dwi"
	- '"bvec" in associations'
	checks:
	- associations.bvec.n_rows == 3

For example, you can update the suffix check to match multiple suffixes with:

-    - suffix == "dwi"
+    - intersects([suffix], ["dwi", "epi"])

And do we want to add a check for epi.bval there must be at least one 0 value?

[tsalo]

And do we want to add a check for epi.bval there must be at least one 0 value?

I think datasets can have non-zero, but low, b-values in these images. In QSIPrep there's a b0-threshold parameter that determines which volumes to treat as b=0.

[effigies]

I think datasets can have non-zero, but low, b-values in these images. In QSIPrep there's a b0-threshold parameter that determines which volumes to treat as b=0.

What are typical thresholds? 1, 1e-2, 1e2?

[effigies]

Just looked, the default is 100. Maybe let's make the check that there is at least one volume with b<100. cc @oesteban @arokem for a sanity check.

[oesteban]

Just looked, the default is 100. Maybe let's make the check that there is at least one volume with b<100. cc @oesteban @arokem for a sanity check.

I believe in DIPY it is 50.

That said, it may be necessary to define it in terms of the number of orientations: to fit the simplest model (DTI), you need at least 6 DWIs and one low-b. So, in practical terms, an EPI sequence with 5 b=1000 and 1 b=0 is, in reality, not a DWI (this is to say, it would fit best in fmap)

Anyways, this is a bit of splitting hairs, so happy to +1 the overall effort.

[effigies]

Yeah, this is just EPI that may have b-values, not DWI, so that doesn't seem like a problem here.

@mattcieslak @tsalo Do you want to make a schema check? Or need guidance?

[arokem]

Overall LGTM! Regarding threshold, I am guessing the b=100 default implemented in qsiprep is based on some use-case that @mattcieslak encountered where the low b-value was larger than 50? This is how DIPY chose that number 😄

[effigies]

LGTM! Since the spec text has not changed in well over a week, and we've been fiddling around in the schema, I think this should be okay to merge without additional delay. I will allow someone else to do the honors, to ensure I am not alone in this opinion.

[tsalo]

I agree. The actual content hasn't changed since @arokem and I approved, so I think we can say that there are still three approvals.