bids-standard / bids-specification

Brain Imaging Data Structure (BIDS) Specification
https://bids-specification.readthedocs.io/
Creative Commons Attribution 4.0 International
265 stars 154 forks source link

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

Closed mattcieslak closed 2 months ago

mattcieslak commented 3 months ago

Closes #1724

codecov[bot] commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

:exclamation: 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 ```diff @@ Coverage Diff @@ ## master #1754 +/- ## ======================================= Coverage 87.92% 87.93% ======================================= Files 16 16 Lines 1375 1351 -24 ======================================= - Hits 1209 1188 -21 + Misses 166 163 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tsalo commented 3 months ago

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?

https://github.com/bids-standard/bids-specification/blob/bd08602760d2fa0fd61e8f579a2677ea9bda8588/src/schema/rules/files/deriv/preprocessed_data.yaml#L100-L105

tsalo commented 3 months ago

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).

effigies commented 3 months ago

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

tsalo commented 3 months ago

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 commented 3 months ago

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

tsalo commented 3 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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?

tsalo commented 2 months ago

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.