-
Notifications
You must be signed in to change notification settings - Fork 228
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
Comments
@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 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). |
@neurolabusc Thanks for your detailed explanation. I understand and agree that it is important to produce BIDS-compliant files. To clarify, dcm2niix currently reports I was wondering, when 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 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. |
When data is reconstructed to its native resolution—like Siemens data by default—the I frequently use the |
@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. |
Sounds good. I will do. Thanks! |
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 It is incredibly valuable to have certain params in the I think this keeps the development of |
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. |
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. |
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.
|
From the BIDS perspective, what's not defined is generally permissible but risks conflicting with future changes. Perhaps a lightweight process could be:
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 |
Just to be clear on that last point, using 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. |
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 |
@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. |
Closing this issue for the upcoming stable release. I do think we can revisit this once consensus is reached on issues 884 and 881 |
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 theAcquisitionMatrixPE
(Phase Encoding) is included, and the frequency information is missing. For Siemens data, a similar value is present in the JSON underBaseResolution
.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
andAcquisitionMatrixPE
(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:
And here is the DICOM tag:
@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 itAcquisitionMatrixFE
or something similar?I’d be happy to submit a PR if it would help!
The text was updated successfully, but these errors were encountered: