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

Additional fields for the json #127

Closed
mharms opened this issue Aug 31, 2017 · 21 comments
Closed

Additional fields for the json #127

mharms opened this issue Aug 31, 2017 · 21 comments

Comments

@mharms
Copy link
Collaborator

mharms commented Aug 31, 2017

Hi,
After looking through the BIDS spec, and just considering what can be useful, I was wondering if you would consider adding the following to the BIDS sidecar:

0008 0080: InstitutionName
0008 1010: StationName
0018 0022: ScanOptions
0018 1000: DeviceSerialNumber
0018 1020: SoftwareVersion

On Siemens at least, 0018 1020 is not sufficient to fully distinguish software versions (e.g., doesn't distinguish between VE11B vs VE11C). So, it would be cool, if a SoftwareVersion tag got added, if it could be the concatenation of the 0018 1020 and the sProtConsistencyInfo.tMeasuredBaselineString entry on Siemens scanners.

cheers,
-MH

@neurolabusc
Copy link
Collaborator

Thanks, I will try to add 0008 1010 (StationName) and 0018 0022 (ScanOptions) to the next release. The others are available in the latest code. In my experience, the current code provides information on your software stepping, e.g. on our machine "SoftwareVersions": "syngo_MR_D13D"

neurolabusc added a commit to neurolabusc/dcm_qa that referenced this issue Aug 31, 2017
@neurolabusc
Copy link
Collaborator

Please check out and test the latest source code for both dcm2niix and dcm_qa. You should see new JSON elements, e.g.

	"StationName": "MRC35131",
	"ScanOptions": "FS",

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

Interesting re SoftwareVersions. Is the value of that tag based solely on the value of 0018,1020? Because on our VE11B and now VE11C scanners, 0018,1020 is just "syngo MR E11" -- i.e., no info on software stepping is included. I also took a look at data from the Connectom scanner, which was officially "VD11D", yet 0018,1020 is just "syno MR D11". So, it is puzzling to me how you got the software stepping detail in your example. (Did Siemens add that detail into 0018,1020 for VD13, but then leave it out of VE11?)

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

Sorry, I didn't think of these earlier, but here are a couple other pieces of information from the custom Siemens fields that would be very handy to have readily extracted as part of the json:

ReceiveCoilName: sCoilSelectMeas.aRxCoilSelectData[0].asList[*].sCoilElementID.tCoilID
ParallelReductionFactorInPlane: sPat.lAccelFactPE
ReceiveCoilActiveElements: sCoilSelectMeas.sCoilStringForConversion OR (0051,100f)
(This last one isn't in the current BIDS spec doc, but it is very useful to have available for QC. I'll propose it as an official tag for the BIDS spec).

@chrisgorgo
Copy link
Collaborator

Looking forward!

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

Ok, one more piece of information that we use within the HCP Pipelines is (0019,1018). We call that "ReadoutSampleSpacing" in our internal XML database, but Eddie Auerbach originally described it to me as:
RealDwellTime -- actual dwell time of the receiver per point (in ns), always including 2X oversampling.

I like "RealDwellTime" since that is more specific and closer to the MR physics.

Siemens returns this as nanoseconds in (0019,1018), so I guess it would need to be converted to seconds, since the json's appear to specify all times in seconds.

Thanks for your consideration and responsiveness on these additional tags!

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

As an additional plug for including "RealDwellTime" it provides a handle on the readout bandwidth, which isn't currently included. :)

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

I will peter out soon, but ImageComments (0020 4000) would be really handy to include as well, since custom sequences frequently embed some useful information there!

neurolabusc added a commit that referenced this issue Sep 1, 2017
@neurolabusc
Copy link
Collaborator

neurolabusc commented Sep 1, 2017

@mharms and @chrisfilo why don't you compile and check the latest version. Note that travis and AppVeyor will report errors as the new BIDS files do not match the reference BIDS files in dcm_qa. Once you confirm the new changes we can update dcm_qa.

The new version generates many new attributes

	"EchoTrainDuration": 0.0007,
	"EPIFactor": 128,
	"ParallelReductionFactorInPlane": 2,
	"CoilID": "Head_32",
	"ConsistencyInfo": "N4_VD11D_LATEST_20110129",
	"ImageComments": "Unaliased_MB4_PE3",
	 "DwellTime": 2.6e-06,

My only concern is that many of these new functions require peaking into the text portion of the CSA header, which I feel a bit uncomfortable about. If you comment out #define myReadAsciiCsa my software will limit itself to the DICOM header and the binary portion of the CSA header which feels safer but will omit several of these fields.

I refer to 0019,1018 as DwellTime, but if you and @chrisfilo agree we can make it RealDwellTime

@chrisgorgo
Copy link
Collaborator

@mharms - shouldn't this be RealDwellTime according to your latest proposal?

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

This is fabulous! Thanks for being so responsive! The only suggestion you didn't include was (0051,100f). Was there a reason, or did that one just get lost in the shuffle? (It is really handy and important to know which coil blocks were activated...)

As far as naming, I'm fine with "DwellTime".

"CoilID" is short and sweet, but there is already "ReceiveCoilName" in the BIDS spec, so would that be a more appropriate tag for the the coil ID?

[I'm not sure how naming issues are best adjudicated. I did notice that the pre-compiled "19-Aug-2017" release has an "AccelFactorPE" tag, so did you just rename that to "ParallelReductionFactorInPlane" with the latest changes? I actually would favor AccelFactorPE for conciseness, and then use AccelFactorSlice (for MB factor) for symmetry...].

If only extracting AccelFactorSlice (MultibandAccelerationFactor in the BIDS spec) wasn't such a pain... The location of that in the DICOMs from the CMRR SMS sequences has changed over the years (and I could relay that info to you if you wanted to add it). Not sure where it is in the Siemens product sequence, although I'm sure we could find it if you wanted to add that.

@neurolabusc
Copy link
Collaborator

The new code renames elements to ReceiveCoilName and AccelFactorPE and adds ReceiveCoilActiveElements and AccelFactorSlice. I compute the multi-band factor by counting repetitions in the CSA vector MosaicRefAcqTimes, in other words, how often are values in the BIDS SliceTiming array are repeated. This seems to be a robust solution, but if you have examples where it fails please suggest an alternative method.

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

Again, awesome! Creative solution to computing the multi-band factor.

I'll let Chris adjudicate between AccelFactorPE vs ParallelReductionFactorInPlane; and AccelFactorSlice vs. MultibandAccelerationFactor as the json tags. I like the AccelFactor{PE,Slice} variants for their conciseness and symmetry (and apparently you do as well), but acknowledge that based on the BIDS 1.0.2 spec there may be users that have "manually" already added that information under the ParallelReductionFactorInPlane and MultibandAccelerationFactor tags.

@mharms
Copy link
Collaborator Author

mharms commented Sep 1, 2017

I'll just mention the following and you can decide if you want to include this. The BIDS spec includes a generic PulseSequenceDetails tag. The value of "tSequenceFileName" in the ASCCONV section (not sure which private DICOM field that is contained in) seems a useful choice for that tag, and allows one to quickly distinguish between product and customer sequences.

We will additionally probably be including the value of "FmriExternalInfo" as well as part of that PulseSequenceDetails tag for the CMRR MB sequences, for which "FmriExternalInfo" includes the specific version of the CMRR sequences employed.

@neurolabusc
Copy link
Collaborator

@mharms please see new code

  1. PulseSequenceDetails supported
  2. FmriExternalInfo will be reported if it exists between the ### ASCCONV BEGIN and
    ### ASCCONV END portion of the series header. I do not have any examples that behave this way - I do have some that contain this after the ### ASCCONV END, but this area seems even less structured than the ASCCONV section. I worry about delving to deeply into these, as at one stage Siemens had some buffer overflow issues that makes parsing these danger prone.
    @chrisfilo - I always defer to you regarding BIDS spec. The BIDS 1.0.2 clearly defines MultibandAccelerationFactor and ParallelReductionFactorInPlane, whereas @mharms favors AccelFactorSlice and AccelFactorPE.

Once @mharms signs off on these changes and @chrisfilo verifies the BIDS description, we should be able to generate a new release.

@chrisgorgo
Copy link
Collaborator

To maintain backward compatibility I would vote for keeping MultibandAccelerationFactor and ParallelReductionFactorInPlane.

However, RealDwellTime/DwellTime is a new term so we should decide which of those two names should be picked. I don't have a strong opinion.

neurolabusc added a commit that referenced this issue Sep 3, 2017
@neurolabusc
Copy link
Collaborator

Thanks @chrisfilo for your clarification. Latest update uses MultibandAccelerationFactor and ParallelReductionFactorInPlane.

@mharms
Copy link
Collaborator Author

mharms commented Sep 5, 2017

DwellTime is fine with me (and requires no change).

@mharms
Copy link
Collaborator Author

mharms commented Sep 6, 2017

Couple of issues that came up in my review so far:

  1. FmriExternalInfo indeed seems to fall after the ### ASCCONV END section, so I guess we won’t have that, since you are leery about parsing that.

  2. PulseSequenceDetails isn’t capturing what I suggested. In every instance, it just seems to be a copy of ProtocolName, whereas I was suggesting that you use the tSequenceFileName field in the ASCCONV section. e.g.,:
    tSequenceFileName = ""%CustomerSeq%\hcp_mbep2d_bold""

  3. I noticed that 3D anatomicals, as well as a 2D TSE sequence, are missing a number of tags that I'd expect to be present. Is the code for writing out certain tags currently dependent on it being a certain type of sequence (e.g., EPI sequence)? For example, among other tags that I'd expect to be present:

	"ReceiveCoilName": 
	"ReceiveCoilActiveElements": 
	"PulseSequenceDetails": 
	"ConsistencyInfo": 
  1. What determines when AccelFactorPE is output rather than (or in addition to) ParallelReductionFactorInPlane? (3D anatomicals are using AccelFactorPE, and I have a custom 2D, MB-enabled ASL sequence that is outputting both, but the value for AccelFactorPE in the json is actually the value for "Accel. factor slice" in the protocol).

  2. What is the origin/derivation for the EchoTrainDuration and EPIFactor values?
    e.g., for our standard HCP BOLD, we have a 104x104 matrix (so PhaseEncodingLines = 104), but the Siemens protocol PDF (and on the console) indicates that the “EPI factor” is also 104, whereas the “EPIFactor” value in the .json is showing 128.
    For our dMRI, we have a 140x140 matrix, but EchoTrainDuration (=0.0007) and EPIFactor (=128) as reported in the json are identical to the BOLD, which makes me wonder how those params are derived, and whether they are meaningful for those particular scans.

neurolabusc added a commit that referenced this issue Sep 7, 2017
@neurolabusc
Copy link
Collaborator

  1. Correct, dcm2niix will not find FmriExternalInfo
  2. Fixed
  3. Fixed (in past these values were only extracted for EPI)
  4. AccelFactorPE no longer used. ParallelReductionFactorInPlane is from ASCII portion of CSA Series Header (sPat.lAccelFactPE)
  5. Please see issue 98.

@mharms
Copy link
Collaborator Author

mharms commented Sep 7, 2017

Just to clarify re (4), for anyone following this thread, based on my reading of the current code, the tag in the json called ParallelReductionFactorInPlane is based on the variable d.accelFactPE in nii_dicom_batch.cpp, which is derived from (0051, 1011) in nii_dicom.cpp (which is not from the ASCII portion of the CSA header).

FWIW, I don't know if the EchoTrainDuration and EPIFactor values have any utility or validity, since in a variety of scan types that I just checked under VE11C, EchoTrainDuration is 0.0007 for all of them, and EPIFactor is either 1 or 128 for all of them. I don't know what other users are using those particular tags/fields for, but I'd be skeptical about using them.

neurolabusc added a commit to neurolabusc/dcm_qa that referenced this issue Oct 17, 2017
yarikoptic added a commit to neurodebian/dcm2niix that referenced this issue Jan 24, 2018
* tag 'v1.0.20170923': (43 commits)
  Increment version for new release
  Update dcm_qa submodule.
  Update dcm_qa submodule.
  Kludge for GEIIS violations https://www.nitrc.org/forum/message.php?msg_id=22370
  Fix file permissions
  Use ReconMatrixPE variable and update some inline comments
  @xjqian & @mharms tags BaseResolution, PhaseResolution, VendorReportedEchoSpacing, DerivedVendorReportedEchoSpacing rordenlab#130
  mharms suggestions for  rordenlab#130
  Partial fix for rordenlab#132
  Fix for rordenlab#131 (comment)
  bandwidthPerPixelCorrected from Dr. Junqian (Gordon) Xu rordenlab#130
  Add PhaseOversampling to BIDS
  BIDS Interpolation2D, RectangularFOV, improve ReadoutTime rordenlab#130
  TotalReadoutTime iPAT vs partial Fourier: rordenlab#130
  Move AsciiCsa to end of BIDS
  Eliminate AppVeyor compiler warnings
  Remove EchoTrainDuration and EPIFactor: rordenlab#127
  BIDS cleanup
  Reorder items in BIDS as suggested by Michael Harms
  Report Partial Fourier in BIDS json
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants