[ENH] Add "_phase" suffix to func datatype for functional phase data

[tsalo]

When users choose to reconstruct both magnitude and phase images from their EPI sequences, they end up with full 4D phase time series. The "part" keyword is used in BEP001 for situations where there is both a magnitude and a phase image.

An alternative to this might be to place the phase time series in fmap/ and to label them the same way one would label single-volume phase images (i.e., for Case 2 for field maps in the specification).

UPDATE: Ultimately it was decided to add a new suffix under the func datatype (_phase) for phase data, because doing otherwise would cause problems for pybids and other tools/workflows.

[chrisgorgo]

I thought about this a little bit more and I am worried that this change will break a lot of existing software. The reason is that previously all _bold images were assumed to be the magnitude and before this change there was no need to perform any additional checks. With this change, existing software will have to be modified to check for the part filename keyword (even if the software in question does not do anything with phase images).

To avoid it I would suggest exploring a route that uses a different suffix or subfolder.

[tsalo]

Since this will involve some discussion, should I close this PR and maybe raise an issue instead?

[chrisgorgo]

I think we can discuss here and modify the PR as we go. It will keep everything in one place (so we would not circle back to old ideas). Best, Chris

…

On Sun, Jan 13, 2019 at 12:39 PM Taylor Salo ***@***.***> wrote: Since this will involve some discussion, should I close this PR and maybe raise an issue instead? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#128 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOkp35PMB40A7TKlN12db6873zagWKvks5vC29kgaJpZM4Z81wl> .

[tsalo]

Here are some options that we've discussed:

• Label the 4D phase time series the same way 3D phase maps are labeled in fmap:
  • Magnitude: func/sub-01_task-rest_echo-1_run-1_bold.nii.gz
  • Phase: fmap/sub-01_task-rest_echo-1_run-1_phase.nii.gz
  • Pros:
    • Most users will use these phase maps for distortion correction.
    • It shouldn't break existing pipelines.
  • Cons:
    • I don't think the specification supports _phase in fmap. I think it only supports _phase1 and _phase2, which may also require _magnitude1 and optionally _magnitude2.
    • If we do this, then we might need to duplicate the magnitude images in func in fmap and label them as magnitude1, magnitude2, etc. instead of echo-1, echo-2, etc.
    • This may be a minor issue, but this specification will enforce a certain use for the phase maps (as field maps), even though others might have other uses for them.
• Add a new datatype to the specification for phase maps from fMRI sequences:
  • Magnitude: func/sub-01_task-rest_echo-1_run-1_bold.nii.gz
  • Phase: phase/sub-01_task-rest_echo-1_run-1_phase.nii.gz
  • Pros:
    • Should work well with existing pipelines
  • Cons:
    • This means making a major addition to the specification for what is going to be a rare use case.

Are there other good options worth discussing as well?

[chrisgorgo]

I personally prefer phase/sub-01_task-rest_echo-1_run-1_phase.nii.gz or even func/sub-01_task-rest_echo-1_run-1_phase.nii.gz because it avoids duplication and makes the link between magnitude (bold) and phase (phase) easy and akin to _sbref.

[tsalo]

I really like func/sub-01_task-rest_echo-1_run-1_phase.nii.gz! I think it's the most compatible with current code with the smallest impact, and it doesn't assume a very specific use for the images.

Should I update the PR or should we pull in others to weigh in first?

[chrisgorgo]

I think it's worth updating the PR now. We will wait a few days for others to chime in.

[KirstieJane]

Hey @tsalo - the BEP001 team have a call on Thursday 24. It would be really great if you could hang tight for that call so I double check that the folks working on that extension don't have any big objections to _phase. Is that ok?

[tsalo]

That's no problem at all. Thanks for making sure there is integration.

[tiborauer]

The BEP001 team has just discussed the issues and we do not have a strong opinion on this.
I think supporting phase in fmap (and func/sub-01_task-rest_echo-1_run-1_phase.nii.gz) would be the best, but I can also understand the pragmatic aspect of func/sub-01_task-rest_echo-1_run-1_phase.nii.gz.

[KirstieJane]

Yep - as @tiborauer said - we (the BEP001 crew) don't think this needs to conform to the same formats etc, and I think @chrisfilo's point about having to check the previous datasets is a really good one.

My gut instinct would be to keep the _phase image in the same folder as the _bold image so it was clear that they came together. I think the "downside" of adding a new modality label is pretty small. This is a super lightly held opinion, so really happy to go with whatever 😸

[tsalo]

Thank you for looking into this. I'll go with the phase suffix within the func folder for now. I totally agree that the downsides are minimized in that case.

[tsalo]

@sappelhoff Would it be possible to change the name of this PR (and the change log) to reflect the actual change that was made? I failed to change the name when we switched from adding the "part" entity to adding a new suffix for functional phase data, so the change log is misleading.

[sappelhoff]

I am not sure whether it's possible to manually change the changelog, or whether that would lead to problems. Perhaps @franklin-feingold knows more?

[franklin-feingold]

To change an entry in the changelog one needs to change the name of the original PR. This will be reflected in the changelog once a new PR is merged in (this will regenerate the changelog)

[tsalo]

That's good to hear. Would it then be alright for me to change the name of this PR so that it reflect the change that was actually merged?

[effigies]

@tsalo Yes, please.

[pvelasco]

Related to this PR: is there any provision to name the phase images for the _sbref images? Maybe for that case they should be named: func/sub-01_task-rest_run-1_sbref.nii.gz and func/sub-01_task-rest_rec-phase_run-1_sbref.nii.gz?

[tsalo]

@pvelasco I think that #367 might be a better place to discuss this, though my understanding is that rec-<magnitude|phase> is automatically added for SBRefs by dcm2niix, but I don't think that matches the actual description for that entity in the specification. I think that part-<magnitude|phase> may solve it, if that proposal is accepted.