[Bug fix] fix get_dicom_info.pl so that it skips the scanner reports #371

cmadjar · 2019-01-17T20:31:35Z

Description

The dcm2mnc conversion is currently failing on datasets that contains Siemens reports. This PR adds a check in get_dicom_info.pl so that the script skips the scanner report.

Also added a check so that only DICOM medical imaging data files are passed to get_dicom_info.pl when converting data to MINC files.

…marizing the files so that dcm2mnc does not crash

…or dcm2mnc so that it only provides to the script DICOM medical imaging data

nicolasbrossard · 2019-01-21T14:35:29Z

dicom-archive/get_dicom_info.pl

@@ -132,6 +132,8 @@ =head2 Methods
    my $dicom = DICOM->new();
    $dicom->fill($filename);

+    next if ($dicom->value('0008','103E') eq 'PhoenixZIPReport');


Wouldn't checking for the presence of header (7fe0,0010) # Pixel data be more robust? Couldn't there be other types of reports (like AtlantaZIPReport, if that even exists) that could be falsely reported as being DICOM files?

Also, I think the code that checks if a file is DICOM should be centralized in subroutine isDicom of ImagingUpload.pm. I suggest modifying the existing code in that library and adding your additional check in there. Also, subroutine isDicom should be allowed to receive one or more file name arguments and return a hash array (key: file name, value: 0 or 1) that would tell whether a file is DICOM or not. The reason behind this is because isDicom issues a Unix file command to do its job and it's better to issue one big file command with a lot of arguments, possibly using xargs, instead of a truckload of file commands with a single argument.

uploadNeuroDB/NeuroDB/MRIProcessingUtility.pm

cmadjar · 2019-01-21T22:53:10Z

@nicolasbrossard Thank you for the review. I have integrated your suggestion. Please, re-review when you get a chance.

nicolasbrossard · 2019-01-22T00:38:51Z

dicom-archive/get_dicom_info.pl

    my $dicom = DICOM->new();
    $dicom->fill($filename);

+    next unless ($dicom->value('7fe0','0010'));


Sorry if I wasn't clear in my original review but I wanted to move this check inside the isDicom method that you put in MRI.pm. Maybe also rename isDicom into isDicomImage since DICOM files can be images, reports or something else. Makes sense?

ooops. I did not get that. Makes sense. Will move this to the isDicomImage function :)

nicolasbrossard · 2019-01-22T00:40:01Z

uploadNeuroDB/NeuroDB/MRI.pm

+  - @non_dicom_files: array with the list of non-DICOM files
+
+=cut
+


As noted in my previous comment, I think this method should be renamed and the check for the pixel data header added in the algorithm.

nicolasbrossard · 2019-01-22T01:05:29Z

uploadNeuroDB/NeuroDB/MRI.pm

+    my @non_dicom_files;
+    foreach my $line (@file_types) {
+        my ($file, $type) = split(':', $line);
+        push @dicom_files, $file     if ($type =~ /DICOM medical imaging data$/);


Since the method is of the form is<something> I'd have a tendency to return true or false for each file, depending on whether the file is DICOM or not:

$isDicom{$file} = $type =~ /DICOM medical imaging data$/ ? 1 : 0;

You'll have to declare my %isDicom before the foreach loop for this to work though.

nicolasbrossard · 2019-01-22T01:06:35Z

uploadNeuroDB/NeuroDB/MRI.pm

+        push @non_dicom_files, $file if (!$type =~ /DICOM medical imaging data$/);
+    }
+
+    return \@dicom_files, \@non_dicom_files;


Replaced by

return \%isDicom;

The code that calls this method will have to be replaced also.

nicolasbrossard · 2019-01-22T01:07:53Z

uploadNeuroDB/NeuroDB/ImagingUpload.pm

-            }
-        }
-    }
+    my ($dicom_files, $non_dicom_files) = NeuroDB::MRI::isDicom(@file_list);


This will have to change if you agree with my comments on isDicom (see below).

cmadjar

@nicolasbrossard I integrated your suggestions to the code. Note that in order to be able to run the imaging pipeline on datasets with scanner reports I had to modify part of the code in ValidateCandidateInfo so that it gives a warning that there are non-image files instead of returning an error and 0 from that function.

cmadjar · 2019-01-23T14:19:45Z

dicom-archive/get_dicom_info.pl

    my $dicom = DICOM->new();
    $dicom->fill($filename);

+    next unless ($dicom->value('7fe0','0010'));


ooops. I did not get that. Makes sense. Will move this to the isDicomImage function :)

cmadjar · 2019-01-23T14:30:09Z

uploadNeuroDB/NeuroDB/ImagingUpload.pm

-            }
-        }
-    }
+    my ($dicom_files, $non_dicom_files) = NeuroDB::MRI::isDicom(@file_list);


cmadjar · 2019-01-23T14:30:26Z

uploadNeuroDB/NeuroDB/MRI.pm

+  - @non_dicom_files: array with the list of non-DICOM files
+
+=cut
+


cmadjar · 2019-01-23T15:13:39Z

uploadNeuroDB/NeuroDB/MRI.pm

+    my @non_dicom_files;
+    foreach my $line (@file_types) {
+        my ($file, $type) = split(':', $line);
+        push @dicom_files, $file     if ($type =~ /DICOM medical imaging data$/);


cmadjar · 2019-01-23T15:13:45Z

uploadNeuroDB/NeuroDB/MRI.pm

+        push @non_dicom_files, $file if (!$type =~ /DICOM medical imaging data$/);
+    }
+
+    return \@dicom_files, \@non_dicom_files;


nicolasbrossard · 2019-01-23T17:34:38Z

uploadNeuroDB/NeuroDB/MRI.pm


-This method checks whether the list of files given as an argument is of type DICOM.
+This method checks whether the files given as an argument are of type DICOM. All


Mmmm. Isn't this doc outdated?

nicolasbrossard · 2019-01-23T17:37:17Z

uploadNeuroDB/NeuroDB/MRI.pm


 =cut

-sub isDicom {
+sub isDicomImage {
    my (@files_list) = @_;

    my $cmd = "file " . join(' ', @files_list);


I think this command could fail for large file lists (we could exceed the maximum length a Unix command can have). Maybe we could change this to:

my $cmd = "ls @files_list | xargs file";

just to be safe. What do you think?

good point!

cmadjar · 2019-01-23T18:21:58Z

@nicolasbrossard Ready for another review. Thanks!

nicolasbrossard

👍

Fixed get_dicom_info.pl so that it skips the Siemens reports when sum…

aefea7f

…marizing the files so that dcm2mnc does not crash

cmadjar added Bug [branch] minor Priority: High labels Jan 17, 2019

cmadjar added this to the 20.2 milestone Jan 17, 2019

cmadjar requested a review from nicolasbrossard January 17, 2019 20:31

cmadjar added the Discussion Required label Jan 17, 2019

modified the find command that gives the files to get_dicom_info.pl f…

882b3ed

…or dcm2mnc so that it only provides to the script DICOM medical imaging data

cmadjar assigned nicolasbrossard Jan 18, 2019

nicolasbrossard requested changes Jan 21, 2019

View reviewed changes

Nicolas' feedback

4975759

nicolasbrossard requested changes Jan 22, 2019

View reviewed changes

Nicolas' feedback

e932954

cmadjar commented Jan 23, 2019

View reviewed changes

nicolasbrossard requested changes Jan 23, 2019

View reviewed changes

Nicolas' feedback

3a46743

nicolasbrossard approved these changes Jan 23, 2019

View reviewed changes

nicolasbrossard merged commit 7dad428 into aces:20.2-dev Jan 23, 2019

nicolasbrossard mentioned this pull request Jan 23, 2019

Non DICOM files are now ignored when running the pipeline. #361

Closed

cmadjar deleted the fix_get_dicom_info_to_skip_Siemens_report branch July 22, 2020 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug fix] fix get_dicom_info.pl so that it skips the scanner reports #371

[Bug fix] fix get_dicom_info.pl so that it skips the scanner reports #371

cmadjar commented Jan 17, 2019 •

edited

Loading

nicolasbrossard Jan 21, 2019

cmadjar Jan 21, 2019

cmadjar commented Jan 21, 2019

nicolasbrossard Jan 22, 2019

cmadjar Jan 23, 2019

nicolasbrossard Jan 22, 2019

cmadjar Jan 23, 2019

nicolasbrossard Jan 22, 2019

cmadjar Jan 23, 2019

nicolasbrossard Jan 22, 2019

cmadjar Jan 23, 2019

nicolasbrossard Jan 22, 2019

cmadjar Jan 23, 2019

cmadjar left a comment

cmadjar Jan 23, 2019

cmadjar Jan 23, 2019

cmadjar Jan 23, 2019

cmadjar Jan 23, 2019

cmadjar Jan 23, 2019

nicolasbrossard Jan 23, 2019

cmadjar Jan 23, 2019

nicolasbrossard Jan 23, 2019

cmadjar Jan 23, 2019

cmadjar commented Jan 23, 2019

nicolasbrossard left a comment

		- @non_dicom_files: array with the list of non-DICOM files

		=cut


		This method checks whether the list of files given as an argument is of type DICOM.
		This method checks whether the files given as an argument are of type DICOM. All

[Bug fix] fix get_dicom_info.pl so that it skips the scanner reports #371

[Bug fix] fix get_dicom_info.pl so that it skips the scanner reports #371

Conversation

cmadjar commented Jan 17, 2019 • edited Loading

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar commented Jan 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmadjar commented Jan 23, 2019

nicolasbrossard left a comment

Choose a reason for hiding this comment

cmadjar commented Jan 17, 2019 •

edited

Loading