code changes to handle single-file dicom and a new TR/TE header field - #180

Is there a reason why you set the original field EchoTime (aas_convertseries_fromstream, lines 153 and 176)?

Why do you multiply TR with 1000 (aas_convertseries_fromstream, lines 144)?

Is there a reason why you set the original field EchoTime (aas_convertseries_fromstream, lines 153 and 176)?

infoD gets appended to DICOMHEADERS (line 245). Even if aa only ever uses TE, it seemed like good practice to make the two values consistent.

Why do you multiply TR with 1000 (aas_convertseries_fromstream, lines 144)?

I must have had some reason to think TR was expressed in seconds in this field so I converted to milliseconds. However, I've looked at a few more of these new Philiips dicoms today and all of them had TR in the field expressed in milliseconds. Is it possible for the value to be either seconds or milliseconds depending on a scanner setting? I can't find anything in the Philips documentation to confirm this (but TBH, the Philips documentation isn't very clear).

We must also consider the real explanation is: I screwed up. :-(

infoD gets appended to DICOMHEADERS (line 245). Even if aa only ever uses TE, it seemed like good practice to make the two values consistent.

The idea is to keep the original 'raw' information untouched. That is why fields volumeTR and volumeTE have been created. Some modules/functions uses these raw field as 'backup'-s, and I do no think we should change them. If you do not mind, I would omit them.

I must have had some reason to think TR was expressed in seconds in this field so I converted to milliseconds. However, I've looked at a few more of these new Philiips dicoms today and all of them had TR in the field expressed in milliseconds. Is it possible for the value to be either seconds or milliseconds depending on a scanner setting? I can't find anything in the Philips documentation to confirm this (but TBH, the Philips documentation isn't very clear).

AFAIK, all values are usually in ms; even across vendors. However, our 'derived' fields (volumeTR, volumeTE, slicetime, echospacing) are in seconds, because that is what SPM (and BIDS) use. Again, I would omit them.

Both of these issues causes problem if you use fieldmaps and/or slicetiming relying on DICOM header.

Ok, I will edit the code and update the PR.

Thanks!

I have done already. Still testing. Then I submit a PR to your branch. If you merge it, then your PR will be automatically updated.

I am afraid I have accidentally pushed into your branch rather than creating a PR. I am so sorry. I hope you can agree in my changes...

If yes, then I am happy to merge your (already updated) PR.

I am afraid I have accidentally pushed into your branch rather than creating a PR. I am so sorry. I hope you can agree in my changes...

No problem -- your changes look good.

Did everything look ok in aamod_convert_epis.m (the other file in this PR)?

Thanks for highlighting it!

There was no issue at all, however, I would revert lines 423-425. Was there any reason why you removed reference to 'sess'? It allows to provide session-specific setting 'ignoreafter'.

Was there any reason why you removed reference to 'sess'? It allows to provide session-specific setting 'ignoreafter'.

Is '"ignoreafter" really session-specific? The default value listed in the xml header is a scalar. As such, passing sess > 1 to aas_getsetting causes aa to print the following (in big red letters):

WARNING: Setting has fewer then 2 elements.
WARNING: First value will be applied!

The first time I encountered this message, I spent a week trying to figure out what was wrong with my analysis. It turns out nothing was wrong with my analysis. The warning is simply unhelpful. So I turned it off by removing sess from the call to aas_getsetting.

If ignoreafter can be specified as a vector then, sure, sess should be passed to aas_getsetting. But can we at least reword the warning message to be more helpful? Something like:

Warning: A single value of the setting was found and will be used for this index (2).

Yes, 'ignoreafter' can be a vector with session-specific values, so please, revert the changes!

I am sorry for the uninformative warning message. I am the one to be blamed. I agree that a more informative one would be preferable. Please, go ahead implementing it!

Update:

1. I added sess back to the call in aas_getsetting in aamod_convert_epi.m as requested
2. I modified the error message in aas_getsetting (using feedback from our aa users here to try to make the message as helpful as possible).
3. I added a comment in aamod_convert_epis.xml that ignoreafter can be defined as a vector, and that leaving it as a scalar will generate an error message.

I pushed these changes to my fork -- I think they will automatically show up here in the PR.

Thanks for all your help!
M

Looks great! I am going to test again.

Hi Tibor,

I saw your modification of aas_getsetting in my fork. Apparently, pulling the caller off the stack using dbstack is bad?

Anyway, I merged your change just now.

Thanks again.

Thanks!
dbstack failed for me. I think it works only when you are actually debugging a script but not in run-time. For that there is the exception stack I implemented.

Is there a reason why you set the original field EchoTime (aas_convertseries_fromstream, lines 153 and 176)?

I'm afraid we need to revisit this issue.

I've just discovered that if you don't set the original field EchoTime (as I had done), then aamod_convert_epis crashes immediately on these Philiips DICOMS.

Your reluctance to alter the original data because it provides a "backup" for other modules may well be true, but not setting the field makes it impossible for us to use this data.

Please advise.

Can you send me the data?

What's the best way to send you the data?

Whichever works for you(r institutional policy): tarred and zipped in Dropbox, GDrive, etc.

I have identified the issue: Your DICOM has no EchoTime at all!
In that special case, it does make sense to set it. Go ahead and then I (also) test it.

I suggest to set EchoTime only if it does not exist, perhaps, at line 214-218. And do not forget to use ms rather than second.

Testing

Ok, I added a check for EchoTime( and RepetitionTime as well) around line 214 and create the fields in the header if they don't exist. I've tested this on our strange philips dicoms as well as our siemens dicoms and it looks like it runs correctly.

Pushed to my fork and updated the PR.

Final PR to you PR has been sent. I think it is ready to be merged with this modification - if you also agree.

Merged.