Add exception handling instead of return value checks on DCMTK GetElement* calls

[fedorov]

It turns out, DCMTK GetElement* do not return non-success value on failure. Instead they throw exceptions: eg see Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx:939.

As a result, failure to find a certain element in the input DICOM series leads to failure of the DWIConvert tool.

This can be fixed by adding try {} catch (...) {} around the calls. I did this for my use case, and it fixed the problem.

[Chaircrusher]

This isn't the case -- DCMTKFileReader will (by default) throw an exception, but DCMTK::DataSet::GetElement does not -- it returns an error code.

[fedorov]

Sorry, I confused the two. Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx:939 is where the exception is thrown, yes it's DCMTKFileReader. Still, the issue remains that it has to be caught.

[fedorov]

Kent, do you acknowledge this is a bug?

[Chaircrusher]

The situation is this: DWIConvert is meant to be primarily, even
exclusively, a program to read DICOM series and generate DWI files. If
there are no gradients present, that primary purpose will fail. So in
that context, missing gradient information is a fatal error.

If you want to read non-DWI gradient series there are other tools
available. I can make DWIConvert read non-DWI DICOM series, but the
question is whether I should.

Do you have a patch for the problem as you percieve it?

Kent Williams norman-k-williams@uiowa.edu

On 6/27/13 10:52 AM, "Andrey Fedorov" notifications@github.com wrote:

Kent, do you acknowledge this is a bug?
‹
Reply to this email directly or
view it on GitHub
#46 (comment).

---

Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged. If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited. Please reply to the sender that you have received the message in error, then delete it. Thank you.

---

[fedorov]

I am confused.

1. the series I am trying to convert IS a DWI series, as produced by GE SignaHdx 3T scanner
2. DicomToNrrdConverter in Slicer3 CAN convert this very series into a NRRD
3. the way it is now, this code https://github.com/BRAINSia/BRAINSTools/blob/master/DWIConvert/GEDWIConverter.h#L96-L99 will never be executed, because GetElementOB() either returns EXIT_SUCCESS, or throws an exception (see https://github.com/Kitware/ITK/blob/master/Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx#L941-L943). There are a lot of other similar places in DWIConvert.

Because of the above, I don't see how this is not a bug. And I do not see why it should be expected that my DWI series should not be converted.

Sure, I can contribute a patch that will fix the specific issue I have - wrap that particular call in try/catch.

However, I don't think this is the right way to do it. Either all similar calls should be wrapped in try/catch, or the GetElement*() calls in DCMTK ITK reader should be fixed to return error, instead of throwing exception.

Kent, you are the main contributor to both DWIConvert and DCMTK reader, so I thought you are in the best position to make the choice between the two above.

What do you think?

[hjmjohnson]

Andrey,

We are also confused. All the previous e-mails you sent seemed to indicate that this was a time series, not a DWI series (hence my comment to try –fMRI flag).

Can you provide an anonymized version of this file format so that we can actually work on data rathter than guessing what is wrong.

For every data set we had access to (a large body of scanner/dicom types), we confirmed that DWIConvert produced equivalent results for both Dicom2Nrrd and DWIConvert. If we have inadvertently introduced a regression on a data set that is not available to us, we are sorry, but we won't be able to effectively resolve this issue when we are blind to what the actual problem is, and for the over 3000 DWI scan sessions from more than 30 sites we have not run into the problem that you express.

Please provide us something that we can actually evaluate, a patch is greatly appreciated because it is the most clear way to communicate what you expect to happen, and it has the side effect of allowing you to test your hypothesis that this will fix your problem without breaking the current test suite. If that is the case, then we will be happy to propagate your proven result throughout the rest of the tool.

Hans

From: Andrey Fedorov <notifications@github.commailto:notifications@github.com>
Reply-To: BRAINSia/BRAINSTools <reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, June 27, 2013 8:21 PM
To: BRAINSia/BRAINSTools <BRAINSTools@noreply.github.commailto:BRAINSTools@noreply.github.com>
Subject: Re: [BRAINSTools] Add exception handling instead of return value checks on DCMTK GetElement* calls (#46)

I am confused.

1. the series I am trying to convert IS a DWI series, as produced by GE SignaHdx 3T scanner
2. DicomToNrrdConverter in Slicer3 CAN convert this very series into a NRRD
3. the way it is now, this code https://github.com/BRAINSia/BRAINSTools/blob/master/DWIConvert/GEDWIConverter.h#L96-L99 will never be executed, because GetElementOB() either returns EXIT_SUCCESS, or throws an exception. There are a lot of other similar places in DWIConvert.

Because of the above, I don't see how this is not a bug. And I do not see why it should be expected that my DWI series should not be converted.

Sure, I can contribute a patch that will fix the specific issue I have - wrap that particular call in try/catch.

However, I don't think this is the right way to do it. Either all similar calls should be wrapped in try/catch, or the GetElement*() calls in DCMTK ITK reader should be fixed to return error, instead of throwing exception.

Kent, you are the main contributor to both DWIConvert and DCMTK reader, so I thought you are in the best position to make the choice between the two above.

What do you think?

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/46#issuecomment-20165417.

---

Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged. If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited. Please reply to the sender that you have received the message in error, then delete it. Thank you.

---

[fedorov]

Hans, I sent the dataset to Kent by email first time I reported the issue. I will resend that email (I don't want to make the data public).

Here's the description of the problem - what is confusing here:

the way it is now, this code https://github.com/BRAINSia/BRAINSTools/blob/master/DWIConvert/GEDWIConverter.h#L96-L99 will never be executed, because GetElementOB() either returns EXIT_SUCCESS, or throws an exception (see https://github.com/Kitware/ITK/blob/master/Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx#L941-L943). There are a lot of other similar places in DWIConvert.

There are three different ways to fix the problem:

1. add try/catch for this single call that causes the failure for my data (very easy, I can send you a patch in a minute, and it will fix my problem).
2. add try/catch calls for all similar calls.
3. return EXIT_ERROR in DCMTK Reader, instead of throwing exception

IMO, 3 is the optimal approach, but you are the architects of both, so please tell me what you think is right!

[hjmjohnson]

Kent will be on vacation, if you provide a patch that can be tested, someone else can test. I'll ask Kent to take a close look at it when he returns from vacation.

Hans

From: Andrey Fedorov <notifications@github.commailto:notifications@github.com>
Reply-To: BRAINSia/BRAINSTools <reply@reply.github.commailto:reply@reply.github.com>
Date: Thursday, June 27, 2013 9:23 PM
To: BRAINSia/BRAINSTools <BRAINSTools@noreply.github.commailto:BRAINSTools@noreply.github.com>
Cc: Hans Johnson <hans.j.johnson@gmail.commailto:hans.j.johnson@gmail.com>
Subject: Re: [BRAINSTools] Add exception handling instead of return value checks on DCMTK GetElement* calls (#46)

Hans, I sent the dataset to Kent by email first time I reported the issue. I will resend that email (I don't want to make the data public).

Here's the description of the problem - what is confusing here:

the way it is now, this code https://github.com/BRAINSia/BRAINSTools/blob/master/DWIConvert/GEDWIConverter.h#L96-L99 will never be executed, because GetElementOB() either returns EXIT_SUCCESS, or throws an exception (see https://github.com/Kitware/ITK/blob/master/Modules/IO/DCMTK/src/itkDCMTKFileReader.cxx#L941-L943). There are a lot of other similar places in DWIConvert.

There are three different ways to fix the problem:

1. add try/catch for this single call that causes the failure for my data (very easy, I can send you a patch in a minute, and it will fix my problem).
2. add try/catch calls for all similar calls.
3. return EXIT_ERROR in DCMTK Reader, instead of throwing exception

IMO, 3 is the optimal approach, but you are the architects of both, so please tell me what you think is right!

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/46#issuecomment-20167033.

---

Notice: This UI Health Care e-mail (including attachments) is covered by the Electronic Communications Privacy Act, 18 U.S.C. 2510-2521, is confidential and may be legally privileged. If you are not the intended recipient, you are hereby notified that any retention, dissemination, distribution, or copying of this communication is strictly prohibited. Please reply to the sender that you have received the message in error, then delete it. Thank you.

---

[Chaircrusher]

Andriy (Andrey? You're listed both ways in E-mail headers).

I am back, and looking at your problem. I've downloaded the test dataset.

The problem with that dataset is that it appears to be completely lacking
in gradient vectors. For GE files, this is stored in 3 dicom entries:

[0019,10bb],[0019,10bc],[0019,10bd]

These are missing from the file, and using DCMDump (which builds as part
of DCMTK, and is great for debugging DICOM datasets), I see no other
representation of gradients. Furthermore, the 0008,0104 tag indicates
that the scan is a prostate scan. Does Diffusion Weighted Imaging of the
human male pelvis have any clinical significance?

The patch you submitted simply suppresses throwing an exception when the
gradient vectors are missing. The net effect will be a gradient of zero.

Furthermore there are only 40 slices present in that dataset, which would
indicate a single-volume MRI scan, not a DWI dataset.

So I'm a little confused about what you think is proper operation of
DWIConvert in this context. The 'regression' involved between DicomToNrrd
and DWIConvert is that formerly, DicomToNrrd ignored missing gradient
information, and DWIConvert requires it is present.

As I've said before, there are other tools for converting DICOM data sets
to image volumes. As a design consideration, does it make sense for
DWIConvert to try and convert every arbitary DICOM image volume? It could
certainly do that but it seems to me to blur the purpose of the program.