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

Frequency Acquisition Matrix for GEHC Data in JSON Output #880

Closed
mr-jaemin opened this issue Oct 23, 2024 · 14 comments
Closed

Frequency Acquisition Matrix for GEHC Data in JSON Output #880

mr-jaemin opened this issue Oct 23, 2024 · 14 comments

Comments

@mr-jaemin
Copy link
Collaborator

mr-jaemin commented Oct 23, 2024

image

I noticed that the Acquisition Matrix values (Frequency: 128 and Phase: 116, as shown in the GEHC UI screenshot) are reported in the DICOM tag (0018,1310). However, in the JSON output, only the AcquisitionMatrixPE (Phase Encoding) is included, and the frequency information is missing. For Siemens data, a similar value is present in the JSON under BaseResolution.

For GEHC data, considering the default behavior of interpolating data to 128/256, having the Acquisition Matrix frequency information would be extremely helpful—especially when the ReconMatrixPE and AcquisitionMatrixPE (already present in the JSON) is not sufficient for accurately understanding the acquisition details.

While the Acquisition Matrix is often a square matrix (Frequency = Phase) (in this case, AcquisitionMatrixPE would be suffient), there are many cases where a non-square matrix is used, making the frequency information more crucial to capture.

Here’s an example of the current JSON output:

"AcquisitionMatrixPE": 116,
"ReconMatrixPE": 256,

And here is the DICOM tag:

(0018,1310) US 128\0\0\116                              #   8, 4 AcquisitionMatrix

@neurolabusc Would it be possible to include the frequency acquisition matrix value from (0018,1310) in the JSON as well, at least for GEHC data? Perhaps we could call it AcquisitionMatrixFE or something similar?

I’d be happy to submit a PR if it would help!

@neurolabusc
Copy link
Collaborator

@mr-jaemin the field ReconMatrixPE in the JSON sidecar is defined by dcm2niix, not BIDS. However, it is mentioned in passing in the BIDS specification as a variable that can be used to estimate TotalReadoutTime. On the other hand, the frequency encoding steps do not seem to be involved in aiding reproducible image processing.

In the early days of dcm2niix, I added a lot of fields to the JSON sidecars that I thought would be useful, even though they were not specified by the BIDS standard. However, this caused compatibility issues with BIDS Extension Proposals that specified different names for values than used by dcm2niix (in particular BEP005 and BEP023). In order to be compliant with BIDS, subsequent versions of dcm2niix introduced breaking changes for image conversion.

Therefore, my current recommendation would be that if you feel that AcquisitionMatrixPE or ReconMatrixPE should be included in the BIDS sidecar, you should introduce this as a change to the BIDS specification. The DeidentificationMethod pull request to dcm2niix illustrates the approach: first update the BIDS specification and then introduce the feature to dcm2niix.

I hope you do not feel discouraged by my response. I do appreciate contributions, but I also think it is appropriate to recognize that the BIDS specification has a process for vetting changes. Since our goal is to produce BIDS-compliant files, we should not unilaterally add fields that have not been vetted by the BIDS team (@effigies gets a shout out for efforts in aiding dcm2niix-BIDS compliance).

@mr-jaemin
Copy link
Collaborator Author

mr-jaemin commented Oct 24, 2024

@neurolabusc Thanks for your detailed explanation. I understand and agree that it is important to produce BIDS-compliant files. To clarify, dcm2niix currently reports AcquisitionMatrixPE and ReconMatrixPE in the JSON sidecars. I propose adding AcquisitionMatrixFE for the frequency information or AcquisitionMatrix for both frequency and phase information.

I was wondering, when AcquisitionMatrixPE was introduced, why was only the phase reported and not the frequency? Including both pieces of information would provide a more complete representation of the acquisition parameters, especially for non-square matrices where frequency and phase differ. This could be particularly useful for understanding acquisition details, consistency checks, and downstream processing.

I appreciate your perspective on maintaining compatibility with BIDS and ensuring that fields are properly vetted through the BIDS process. It makes sense that breaking changes could cause issues, and I understand the importance of aligning with the BIDS standard. If introducing AcquisitionMatrixFE or enhancing AcquisitionMatrix to cover both dimensions could be beneficial, I'd be happy to help initiate a change to the BIDS specification.

Please let me know if that approach makes sense, or if you have other suggestions on how best to proceed. I'm more than willing to contribute to the BIDS side as well, including drafting the necessary proposal.

@mr-jaemin
Copy link
Collaborator Author

When data is reconstructed to its native resolution—like Siemens data by default—the ReconMatrix generally matches the AcquisitionMatrix. However, if data is reconstructed with interpolation, the AcquisitionMatrix becomes particularly important for understanding the original acquisition parameters. This distinction helps maintain clarity about how the data was collected versus how it has been processed or interpolated, which can be vital for accurate analysis and reproducibility.

I frequently use the AcquisitionMatrixPE reported by dcm2niix for this purpose and wish to have frequency information as well.

@neurolabusc
Copy link
Collaborator

@mr-jaemin this sounds reasonable to me. Can I suggest you propose this field as a pull request to the BIDS specification? Again, the DeIdentificationMethod PR provides a nice template. Once this is in the specification, we can adopt it into dcm2niix.

@mr-jaemin
Copy link
Collaborator Author

Sounds good. I will do. Thanks!

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

Kudos to @mr-jaemin for his willingness to go through the BIDS extension process. But I want to push back a bit on the notion that going forward no tags should be added to the dcm2niix sidecar json without blessing by BIDS first.

It is incredibly valuable to have certain params in the dcm2niix sidecar even if they are not in the BIDS specification. I think a strong argument can be made that dcm2niix should feel free to add parameters that may be useful, and that future BIDS extensions that may subsequently incorporate that parameter should favor using the same tag name as implemented by dcm2niix. When that turns out to be ill-advised (for some reason), dcm2niix could simply add the revised parameter tag, while keeping the older name as well for backwards compatibility.

I think this keeps the development of dcm2niix sufficiently flexible and responsive, without requiring that all new parameters get approved by BIDS first, which is a steep ask.

@mharms mharms reopened this Oct 24, 2024
@neurolabusc
Copy link
Collaborator

Happy to have @effigies weigh in on this one. I am somewhat reluctant to add new tags directly into dcm2niix, as several BEPs have used different nomenclature. However, we can do this if @effigies thinks it makes sense. If we do add fields independent of BIDS, it might be worth getting a consensus from people involved with BIDS that the proposed name fits nicely into the schema.

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

I have no issue with soliciting quick feedback from BIDS folks as to whether the name for a proposed new field fits into the general existing BIDS schema, and if not, what they would suggest as an alternative. But I don't think that addition of new parameters should be contingent on someone first completing a formal BIDS extension.

@captainnova
Copy link
Collaborator

As someone who maintains software that (sort of*) breaks when dcm2niix changes its JSON output, I would still rather have it be responsive to new developments in DICOM usage, which regularly happen with little to no concern for the BIDS process. So I agree with @mharms that a quick check with BIDS experts is a good idea, but formal BIDS extensions might take too long and/or be too onerous. Starting a formal BIDS extension is good practice, but encouraging open source contributions is an important concern, and I'd hate for someone contributing a software change to have their enthusiasm sapped by excessively delayed gratification. Conversely, there have been some things proposed for BIDS that didn't seem to have any connection to a practical implementation.

  • We are used to it and have a coping mechanism. As long as the dcm2niix JSON output is somewhat more stable across manufacturers and software versions than DICOM is, we're elated. Breaking changes in dcm2niix JSON also often act as early warnings for things that would cause bigger headaches elsewhere in our software. It's great to have the dcm2niix issues pages where people often discuss these things, and work out practical responses, before they hit us. The best way to keep that up going forward seems to be for dcm2niix contributors and BIDS people to work together but recognize that they may not always agree or want to wait for each other.

@effigies
Copy link

From the BIDS perspective, what's not defined is generally permissible but risks conflicting with future changes.

Perhaps a lightweight process could be:

  1. Open an issue just to stake a claim to metadata space. For example: "dcm2niix generates AcquisitionMatrixPE and ReconMatrixPE, and is considering adding AcquisitionMatrixFE". This should help grab the attention of anybody who might use them for other purposes. If there's already a conflict or convergence, it's best to start the conversation sooner than later.

  2. We can then work through what the actual definitions and types of these terms should be and add them to BIDS. This could take as much or little time as needed.

In this case, adding 3-4 metadata terms that apply to most MR images would not require a BEP, just a PR that could take a little time to workshop. If dcm2niix is already generating 2 of these, then a preference for consistency and backwards compatibility would probably shorten the conversation.


There are two open issues on nonstandard metadata:

One safe thing to do would be to prefix any nonstandard metadata with x-dcm- or similar.

@effigies
Copy link

Just to be clear on that last point, using x-dcm- would be a prospective change, and doing it retroactively would be a cure worse than the disease.

If anything, it would be good to propose defining all of the current fields dcm2niix emits in BIDS. I would advocate for adopting them more-or-less wholesale. I can't readily imagine a reason that we would reject a term altogether, but if that did happen, we could at least carve it out as not definable by future extensions, so that there won't be conflicts with dcm2niix outputs.

@mharms
Copy link
Collaborator

mharms commented Oct 24, 2024

Not a fan of requiring a common prefix for any new fields not already formally defined in BIDS. I don't know what specific conflicts arose in the aforementioned BEPs and whether the BEP was aware of the already existing name/definition within dcm2niix (but explicitly decided that name/definition didn't work for some reason). But overall, I don't see much need for big changes, because things seem to be working pretty well "as is".

@neurolabusc
Copy link
Collaborator

@mharms PET BEP009 did cause key renaming as described in issues 669 and 802. The issues from ASL BEP005 are more problematic. This BEP both renamed and defined new keys, but only provided validation datasets as the output BIDS/NIfTI without the input DICOM that would allow concrete validation and regression testing. The software created by the BEP team uses an institutional license that prevents developers from understanding the logic for determining these (as it would compromise the open source licenses used by dcm2niix and dicm2nii). Therefore, no open source tools support ASL BIDS conversion. Hopefully that is an isolated issue, as recent changes expect complete source data and its corresponding target BIDS datasets.

@neurolabusc
Copy link
Collaborator

Closing this issue for the upcoming stable release. I do think we can revisit this once consensus is reached on issues 884 and 881

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

5 participants