Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistency of development and stable branch behavior #881

Open
neurolabusc opened this issue Oct 24, 2024 · 23 comments
Open

Consistency of development and stable branch behavior #881

neurolabusc opened this issue Oct 24, 2024 · 23 comments

Comments

@neurolabusc
Copy link
Collaborator

@captainnova noted a change between the development branch and the stable release that can be seen with the dcm_qa_fmap validation dataset.

The stable release will append the PHASE type for phase images

"ImageType": ["ORIGINAL", "PRIMARY", "P", "ND", "PHASE"]

The development release omits this if the alias P is already listed as the image type:

"ImageType": ["ORIGINAL", "PRIMARY", "P", "ND"]

This is due to this line of code and reflects my general style of avoiding redundancy.

Note that the same behavior is seen for Imaginary, Real and Magnitude.

Note that the field ImageType is unique to dcm2niix, and is not required by the BIDS specification. Also, note that the DICOM tag ImageType (0008,0008) does not define PHASE or other values, though by convention some manufacturers report it here. For modern DICOM, this information is typically reported in Complex Image Component (0008,9208).

Since this behavior is specific to dcm2niix, the upcoming commit reverts to the previous style, and will insert PHASE even if the alias P is already present.

@neurolabusc
Copy link
Collaborator Author

Associated with this issue, Philips enhanced DICOM fieldmaps like this one pack both a magnitude image and a fieldmaphz image into a single file. This causes dcm2niix to erroneously append FIELDMAPHZ to both the magnitude and fieldmap image. This flag should only be appended to the fieldmap and not magnitude image.

neurolabusc added a commit that referenced this issue Oct 24, 2024
@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

Just to refresh memories, the reason the PHASE/MAGNITUDE aren't added as tags if P/M is already present is because of #851. So, this potential change is a reversion of that.

Personally, I don't think there should be two tags representing the exact same thing, which is potentially confusing. IMO, other tools/code that was previously looking for PHASE/MAGNITUDE should be adapted/updated to look for P/M instead.

@neurolabusc
Copy link
Collaborator Author

@mharms I think the challenge is that one persons feature is another person's regression. There is no clear standard. I am happy for @effigies to weigh in here. There seems to be four options:

Assuming that a DICOM phase image has ImageType (0008,0008) ORIGINAL\PRIMARY\P\NONE (e.g. classic Siemens scanner; most Philips; UIH) there seem to be three options:

  1. Direct copy ORIGINAL\PRIMARY\P\NONE
  2. Append Phase to expand alias ORIGINAL\PRIMARY\P\NONE\PHASE
  3. Append Phase and remove alias ORIGINAL\PRIMARY\NONE\PHASE
  4. not applicable

Assuming that a DICOM phase image has ImageType (0008,0008) ORIGINAL\PRIMARY\NONE (e.g. GE scanner, modern Siemens) there seem to be three options:

  1. Direct copy ORIGINAL\PRIMARY\NONE
  2. Append Phase ORIGINAL\PRIMARY\NONE\PHASE
  3. not applicable
  4. Append alias ORIGINAL\PRIMARY\NONE\P

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

Are you saying that an XA Siemens system doesn't already have P for a Phase recon? That would be surprising to me.

@captainnova
Copy link
Collaborator

Assuming that a DICOM phase image has ImageType (0008,0008) ORIGINAL\PRIMARY\P\NONE (e.g. classic Siemens scanner; most Philips; UIH) there seem to be three options:

1. Direct copy  `ORIGINAL\PRIMARY\P\NONE`

2. Append Phase to expand alias  `ORIGINAL\PRIMARY\P\NONE\PHASE`

3. Append Phase and remove alias `ORIGINAL\PRIMARY\NONE\PHASE`

4. _not applicable_

+1 (or +2!) for 2, please.

Assuming that a DICOM phase image has ImageType (0008,0008) ORIGINAL\PRIMARY\NONE (e.g. GE scanner, modern Siemens) there seem to be three options:

1. Direct copy  `ORIGINAL\PRIMARY\NONE`

2. Append Phase  `ORIGINAL\PRIMARY\NONE\PHASE`

3. _not applicable_

4. Append alias `ORIGINAL\PRIMARY\NONE\P`

Out of these choices 4 is the best, but personally I would like ORIGINAL\PRIMARY\P\NONE\PHASE to be more like the more common behavior, and be more descriptive.

The single letter P, I, R + M codes are needed for existing software that depends on them. "PHASE" is just cosmetic, but it's an important addition since the single letter codes are treated as suspiciously ambiguous by humans until they build up a fair bit of experience with them. If the long form moved to ImageTypeText that would be fine.

@mharms : Siemens XA moved it, but only if using enhanced DICOM. I forget where the new address is, but it was a case of moving from an unofficial but well known location to an official but obscure one. I appreciate dcm2niix restoring it to where we'd gotten used to looking for it.

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

@captainnova : Why do you so favor having P and PHASE both simultaneously present? Personally, I think that having both present is going to be confusing, because a user would not expect the DICOMs themselves to contain redundant information (which they don't), so then said user starts wondering what P represents since they will presume that it must represent something other than PHASE.

Siemens has used M and P in (0008,0008) going back at least a decade. (And based on the comment above, perhaps Philips as well?) I don't see why the behavior and output should be changed for Siemens and Philips data just to accommodate GE's choice to apparently use either MAGNITUDE/PHASE instead (if I'm understanding correctly).

Or, if GE DICOMs don't have the MAGNITUDE/PHASE distinction in (0008,0008) itself (but it can be inferred from elsewhere), then it seems like the solution should be to add M and P, consistent with long-standing Siemens (and perhaps also Philips) behavior/convention.

@effigies
Copy link

I am happy for @effigies to weigh in here.

I have no opinions here. @yarikoptic @marcelzwiers @dlevitas might, representing tools that wrap dcm2niix.

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

One other thought, which is contradictory to my suggestion above to consolidate on using M and P, would be to use MAGNITUDE/PHASE/REAL/IMAGINARY/FIELDMAPHZ to represent an addition by dcm2niix to the native contents of (0008,0008), but only if M/P/R/I isn't already present (to avoid duplication). Thus, heuristically, one knows that the presence of MAGNITUDE/PHASE/REAL/IMAGINARY/FIELDMAPHZrepresents an addition by dcm2niix. [This incidentally is exactly where we ended up with #851 :) ].

@captainnova
Copy link
Collaborator

@captainnova : Why do you so favor having P and PHASE both simultaneously present?

P because downstream software needs it. PHASE because most humans appreciate, or even need, it. But simultaneously present doesn't necessarily mean colocated. I have no strong opinion on whether PHASE is in ImageType or in ImageTypeText. There is a case where writing something to BIDS ImageType that is different than the DICOM (0008, 0008) ImageType is clearly the right thing to do (add P for downstream software), so adding PHASE only breaks a BIDS-DICOM correspondence that is already broken in some cases.

@neurolabusc helpfully supplied the tag Siemens XA enhanced is using at the top: Complex Image Component (0008,9208). On reviewing it I noticed it uses words (which I like, obviously), and 'MIXED', which is illuminating. (Philips) Enhanced DICOM needs a code for different complex components in different volumes, so the M,P,I,R convention wouldn't have worked. dcm2niix/BIDS, and oddly enough Siemens enhanced (I think), put the different components in different niis/volumes, but it's interesting that (0008, 9208) is motivated by more than just wanting to formalize things.

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

There are some different, somewhat distinct issues at play here:

  1. Whether redundant information should be present. I'm not a fan of that personally.
  2. Whether, and under what conditions the native entry in (0008,0008) should be modified/extended. And somewhat relatedly, for Siemens at least, which changed how it populates ImageType (0008,0008) for Enhanced XA DICOMs, why can't the magnitude/phase distinction be pulled from the ImageTypeText (0021,1175) field in the json instead?) (Converting Siemens Vida data #236 (comment))
  3. Readability -- i.e., PHASE rather than P, or MAGNITUDE rather than M. Sure, the former may be more intuitive, but the (0008,0008) field has long contained various cryptic entries, so I'm not sure why phase and magnitude merit special status in that regard. [e.g., Magnitude and Phase images are visually easily distinguishable, but the ND vs. DIS3D distinction is not, yet those entries are (understandably) not translated into a more "user-friendly" form, despite being quite important to knowing what type of recon was done on the acquisition].
  4. Community expectations based on vendors' earlier and existing conventions, and which other existing tools/software may be built against in terms of identifying magnitude and phase images.

Re (4): I feel that I'm missing some potential context here. @bpinsard: Why did #851 choose to add MAGNITUDE and PHASE, rather than M and P? Did you just want something that was intuitive, without regard to existing tools/code? Is there a discussion of this prior to #851?

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

@captainnova : I also feel like I'm missing some context on the origin of this Issue, and why you object to the change made in #851. Is it solely that P and M may be a little bit obscure (although quickly induced if one has a pair of magnitude and phase images available for viewing)?

@bpinsard
Copy link

So IIRC the history is:

  • PHASE/REAL/IMAGINARY was/is inserted in ImageType for stable release for a long time, but maybe there wasn't any GE dicoms in qa that would cause the duplication. So the duplication was already there from the start, just for phase/real/imaginary.
  • due to the syntax used above, the change I contributed added MAGNITUDE in add MAGNITUDE to ImageType for GE data  #826, but of course this impacts most QA files, causing the duplication (M+MAGNITUDE).
  • this duplication was corrected in Addition of MAGNITUDE to ImageType of Siemens data #851

If we want to revert to the old long-standing dcm2niix behavior for tools that use it, I am fine with that.
But, as previously stated, we would need a dedicated ComplexImageComponent BIDS json tag that store that info in a reliable, human-readable (not M/P/R/I) way to allow reliable BIDS conversion.
The main source being the dicom tag ComplexImageComponent, and some custom extraction for other old format and manufacturer specific oddities.

@neurolabusc
Copy link
Collaborator Author

@mharms

Are you saying that an XA Siemens system doesn't already have P for a Phase recon? That would be surprising to me.

You can download an example from Siemens TerraX XA60 here. Siemens rationale for this change is that ImageType (0008,0008) does not define either M or MAGNITUDE, but enhanced DICOM does define Complex Image Component (0008,9208). This is also the reason they removed DIS2D from ImageType.

(0020,0011) IS [2]                           # SeriesNumber
(0018,1030) LO [PRODUCT__ep2d_bold__p3_sms1] # ProtocolName
(0008,0008) CS [ORIGINAL\PRIMARY\FMRI\NONE]  # ImageType
(0021,1175) CS [ORIGINAL\PRIMARY\M\DIS2D]    # ImageTypeText
(0008,9208) CS [MAGNITUDE]                   # ComplexImageComponent

I appreciate the comments that each of you have made so far - open discussion and dissent is the first stage of consensus building. This is very timely with the discussion of issue 880 - in general I think the BIDS specification is the appropriate venue to resolve these issues. The BIDS team has clear mechanisms for resolving these issues and a democratically elected steering committee to reconcile conflicts.

@mharms
Copy link
Collaborator

mharms commented Oct 25, 2024

So IIRC the history is:

* PHASE/REAL/IMAGINARY was/is inserted in ImageType for stable release for a long time,

@bpinsard : Are you referring specifically to DICOMs from GE in that comment?

So, if the insertion of PHASE/REAL/IMAGINARY into the ImageType entry for GE DICOMs goes back farther than #826, I guess that explains why that PR went with MAGNITUDE rather than M (i.e., to match the nomenclature already in place for the addition of PHASE/REAL/IMAGINARY).

That said, I don't understand the origin of the objection to M/P/R/I, given that (as I noted above) there are already plenty of ImageType fields that are not intuitive. (M/P/R/I is actually pretty clear IMO for anyone that knows a bit about MR physics). Code/tools can be set up to parse M just as well as MAGNITUDE, and in fact code/tools presumably already needs to handle M given that is the standard ImageType tag for a magnitude image on a VE11 Siemens system.

@mharms
Copy link
Collaborator

mharms commented Oct 25, 2024

Are you saying that an XA Siemens system doesn't already have P for a Phase recon? That would be surprising to me.

You can download an example from Siemens TerraX XA60 here. Siemens rationale for this change is that ImageType (0008,0008) does not define either M or MAGNITUDE, but enhanced DICOM does define Complex Image Component (0008,9208). This is also the reason they removed DIS2D from ImageType.

Yes, I had temporally forgotten that in Enhanced XA DICOMs Siemens changed what they report in (0008,0008), presumably to conform to how they interpret the DICOM standard for that field, with the E11 version of (0008,0008) thankfully retained in (0021,1175), as you noted above.

That raises the separate question of whether M should be added "back" to ImageType in the json for Siemens Enhanced XA, or if users/tools simply need to adapt to checking ImageTypeText as a back-up field for that information.

@yarikoptic
Copy link
Contributor

... representing tools that wrap dcm2niix.

ping @tsalo and @bpinsard who might have even better grasp of the situation of handling those in heudiconv

@bpinsard
Copy link

bpinsard commented Oct 25, 2024

@yarikoptic
Heudiconv requires #826 to properly disambiguate complex component stored in the same series (eg. GE hyperband, or Siemens SBRef) without blindly assuming that no info meant magnitude (see nipy/heudiconv#761 for more context). But if that info reliably ends-up in another json tag, that easily adaptable and all for the better.

@bpinsard : Are you referring specifically to DICOMs from GE in that comment?

Yes GE only, thanks for the precision.

@neurolabusc
Copy link
Collaborator Author

I am closing this issue here, as it is described in the BIDS issue I opened. The upcoming stable release of dcm2niix will include the suggestions @bpinsard to support heudiconv. However, I do think @mharms has appoint that in general we try to avoid duplication, and the BIDS tag ImageType is no longer a direct translation of the DICOM header.

I do think the best solution might be to introduce the BIDS key ComplexImageComponent that would correspond with the tag Complex Image Component (0008,9208) introduced in enhanced DICOM. For classic DICOMs, we could still populate the BIDS tag from manufacturer specific details.

@effigies
Copy link

Just in case it needs clarification, ImageType is not a BIDS term at all.

Personally, I would probably expect direct translations of DICOM, but as this is not about BIDS and no software I write uses these fields, I don't have any stake here.

@bpinsard
Copy link

bpinsard commented Oct 30, 2024

More thoughts about adding a ComplexImageComponent into BIDS: this will be redundant in some cases with the part- entity, but there is a chicken-n-egg problem, where dcm2niix is best suited to extract that value consistently across the whole zoo of dicoms and some BIDS converters rely on the json sidecars (eg. dcm2bids, and heudiconv for multiple nifti for the same-series). @effigies I am not sure if that redundancy is problematic for BIDS, as part- is also optional for most BIDS patterns.

@effigies
Copy link

Redundancy between metadata and entities is not a problem. In many cases there is a link between entities and a metadata field, (e.g., dir and PhaseEncodingDirection, flip and FlipAngle...). In cases where the metadata is not needed to distinguish at the filename level, the entity is not used.

@mharms
Copy link
Collaborator

mharms commented Oct 30, 2024

So, just to summarize where things currently stand, as a result of 2ffaba7 if the type of the image can be identified (not sure exactly how that is all accomplished), then MAGNITUDE/PHASE/REAL/IMAGINARY will get appended to the ImageType json field (if that string isn't already present), regardless of manufacturer, software version, and whether M/P/R/I is already present.

@mharms
Copy link
Collaborator

mharms commented Oct 30, 2024

Personally, I would have favored that we resolved to append M/P/R/I if not already present, since those are the long-standing tags already in use by Siemens (apparently Philips as well), and thus other tools should presumably be checking against those tags, not some new tags that were initially only introduced in the context of GE data. @bpinsard : How was heudiconv previously distinguishing image type for Siemens data?

@mharms mharms reopened this Oct 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants