Refactor dicom importer #758

Raedlr · 2024-08-28T13:04:37Z

object oriented structure, import handled by matRad_DicomImporter
Adapted GUI to work with new Importer
scan is part of the importer and output adapted for better compatibility

wahln · 2024-08-28T13:11:22Z

Thanks for the Pull Request!
@amitantony could you do the review?
Also, we should think about a way to run tests on the DICOM import / export. First step could be to take a very small dummy ct / cst / dose, export it with the dicom exporter, and then try to reimport those by script again.

amitantony

Overall the PR looks very good and clean 👍 I have given few format and minimal comments, will run some testing before final approval

amitantony · 2024-08-29T14:53:43Z

matRad/dicom/@matRad_DicomImporter/matRad_importDicom.m

+%
+% input
+%   importFiles:          list of files to be imported (will contain cts and rt
+%                         structure set)


Same as above :
you can move the input description to the description of the funtion, descipbe the require properties of input and output object

I've changed the description here c5b44ad

amitantony · 2024-08-29T15:22:04Z

matRad/dicom/@matRad_DicomImporter/matRad_createCst.m

+% call
+%   obj = matRad_createCst(obj)
+%
+% input


you can move the input description to the description of the funtion, descipbe the require properties of input and output object

amitantony · 2024-08-30T08:24:10Z

matRad/gui/widgets/matRad_importDicomWidget.m

@@ -993,14 +895,15 @@
    end    

    methods (Access = private)
-        % SCAN FUNKTION
+    % SCAN FUNKTION


can we rename this to a more descriptive name for the funtion and not just "scan" ?
like "scanFolder" or something

renamed here 5dcd737

amitantony · 2024-08-30T08:27:17Z

matRad/util/matRad_comparePlnStf.m

@@ -64,7 +64,13 @@
 end

 %% compare Bixel width in stf and pln
-if  ~isfield(pln.propStf,'bixelWidth') || stf(1).bixelWidth ~= pln.propStf.bixelWidth
+if  isfield(pln.propStf,'bixelWidth') && ischar(stf(1).bixelWidth)
+    LogVal = strcmp(stf(1).radiationMode, pln.radiationMode);


change variable name to match matRad rules and could chnage it to a descriptive name "matchRadiationMode"

amitantony · 2024-08-30T08:34:01Z

matRad/dicom/@matRad_DicomImporter/matRad_DicomImporter.m

+                end
+            end 
+
+            %We need to set one more variable I forgot to mention above 


clean up the comments 😋

amitantony · 2024-08-30T08:39:58Z

matRad/dicom/matRad_interpDicomCtCube.m

 %
 % input
 %   origCt:         original CT as matlab 3D array
 %   origCtInfo:     meta information about the geometry of the orgiCt cube
 %   resolution:     target resolution [mm] in x, y, an z direction for the
 %                   new cube
-%   grid:           optional: externally specified grid vector
+%   doseGrid:       optional: externally specified grid vector


maybe change from 'doseGrid' to 'importGrid'

Was changed in this update c5b44ad

amitantony · 2024-08-30T08:41:56Z

matRad/dicom/@matRad_DicomImporter/matRad_interpDicomDoseCube.m

-dose.cube = double(dosedata);
-% dose.cube = single(dosedata);
+obj.importRTDose.dose.cube = double(dosedata);
+% importRTDose.dose.cube = single(dosedata);


can remove unused lines

amitantony · 2024-08-30T08:42:31Z

matRad/dicom/@matRad_DicomImporter/matRad_interpDicomDoseCube.m


 % give it an internal name
-dose.internalName = currDose{12};
+ % obj.dose.internalName = obj.currDose{12};%?????


whats going on here ?

I remember that we discussed this, and I still have no idea, what is this for

id suggest removing it

…inge patient

… be used to find the structure file related to the CT (but not yet)

…re files

Raedlr · 2024-09-06T15:53:04Z

matRad_importDicomSteeringParticles* in the last one

…ith different thicknesses

amitantony · 2024-10-02T08:04:48Z

matRad/dicom/@matRad_DicomImporter/matRad_importDicomSteeringParticles.m

+    obj.stf(i).gantryAngle   = obj.pln.propStf.gantryAngles(i);
+    obj.stf(i).couchAngle    = obj.pln.propStf.couchAngles(i);
+    obj.stf(i).bixelWidth    = obj.pln.propStf.bixelWidth;
+    obj.stf(i).matchRadiationMode = obj.pln.radiationMode;


check all files for this name and correct it ( matchRadiationMode)

amitantony · 2024-10-02T10:50:54Z

matRad/gui/widgets/matRad_importDicomWidget.m

-                if numel(res_z) > 1
+                set(handles.resx_edit,'String',this.importer.importFiles.resx);
+                set(handles.resy_edit,'String',this.importer.importFiles.resy);
+                if numel(this.importer.importFiles.resz) > 1


In testing, numel(this.importer.importFiles.resz) is >1 if the number of characters in resz (String) is >1,
Will need to implement some other check to see if you have multiple resz entries.

amitantony · 2024-10-02T10:52:10Z

matRad/gui/widgets/matRad_importDicomWidget.m

-                set(handles.resx_edit,'String',res_x);
-                set(handles.resy_edit,'String',res_y);
+                set(handles.resx_edit,'String',this.importer.importFiles.resx);
+                set(handles.resy_edit,'String',this.importer.importFiles.resy);
                if numel(res_z) > 1


see comment above

matRad/util/matRad_comparePlnStf.m

…dth'

…icomImporter

amitantony · 2024-10-02T15:42:27Z

final testing required for importing particle therapy plan, then we are ready to go

* temorary directory used for dicom export and reimport * removed GUI elements from folder scanning

wahln · 2024-10-02T22:33:14Z

I did a few changes:

added helper function to use temporary directory in tests
extended the tests to do an export and an actual reimport
temorary directory used for dicom export and reimport tests
removed GUI elements from folder scanning (I don't want to many boxes opened when we are using the exporter from script!)

In matRad_interpDicomCtCube, there still seems to be an issue in case of SliceThickness being a String in this line:

matRad/matRad/dicom/matRad_interpDicomCtCube.m

Line 90 in 2a3c895

    
           if(all(abs([origCtInfo(1).PixelSpacing(1),origCtInfo(1).PixelSpacing(2),origCtInfo(1).SliceThickness] - [resolution.x,resolution.y,resolution.z]) < 1e-6))

If this only happens with the data exported from us, then it might either be an exporter error or an import error. The standard suggests a "Decimal String" for the attribute, so its unclear.

wahln

Not entirely sure if you can always be sure that the imported resz is coming in as a string - str2double will produce NaN if it is already a double.

wahln · 2024-10-08T15:21:07Z

matRad/dicom/@matRad_DicomImporter/matRad_importDicomCt.m

@@ -65,7 +65,7 @@
    % might not been defined for individual files
    obj.importCT.ctInfo(i).PixelSpacing            = tmpDicomInfo.PixelSpacing;
    obj.importCT.ctInfo(i).ImagePositionPatient    = tmpDicomInfo.ImagePositionPatient;
-    obj.importCT.ctInfo(i).SliceThickness          = obj.importFiles.resz;
+    obj.importCT.ctInfo(i).SliceThickness          = str2double(obj.importFiles.resz);


Are you certain that this is always a string / char array? Because if it is a number, it will become NaN here.

wahln · 2024-10-08T15:54:29Z

matRad/dicom/@matRad_DicomImporter/matRad_scanDicomImportFolder.m

+if ~warnDlgDICOMtagShown && strcmp(allfiles{row,column},defaultPlaceHolder) && (column == 3 || column == 4)
+
+   dlgTitle    = 'Dicom Tag import';
+   dlgQuestion = ['matRad_scanDicomImportFolder: Could not parse dicom tag: ' tag '. Using placeholder ' defaultPlaceHolder ' instead. Please check imported data carefully! Do you want to continue?'];
+   answer      = questdlg(dlgQuestion,dlgTitle,'Yes','No', 'Yes');
+
+   warnDlgDICOMtagShown = true;
+
+   switch answer
+      case 'No'
+         matRad_cfg.dispError('Inconsistency in DICOM tags')  
+   end
+end


This is opening a dialog waiting for an answer. Dialogs should not be opened in the functions (which might also beused from scripts) but in the corresponding GUI piece.

questdlg error for octave

replacing rmmissing (not supported on octave)

wahln · 2024-10-09T09:27:00Z

Pushed some fixes to pass octave tests

Raedlr added 3 commits August 28, 2024 14:37

Refactor of the dicom import to object oriented structure

0f2c94b

Small bug fixes to Plan Widget and stf checker

417d967

Added name to authors list

be6a1fa

wahln requested review from wahln and amitantony and removed request for wahln August 28, 2024 13:08

wahln assigned wahln and amitantony and unassigned wahln Aug 28, 2024

wahln added the refactor label Aug 28, 2024

fixed small bag

2cbd7b1

amitantony reviewed Aug 30, 2024

View reviewed changes

Raedlr added 7 commits August 30, 2024 15:59

Fixed bug with resolution

c0043a6

Added the possibility of selecting patients and structures within a s…

d545170

…inge patient

A small upgrade for the scan function, adding new parameters that can…

aee6991

… be used to find the structure file related to the CT (but not yet)

some changes in the description of matRad_DicomImporter funktions

c5b44ad

scan renamed to scanFolder

5dcd737

renamed field in stf 'radiationMode' to 'matchRadiationMode'

182194b

matRad_importDicomParticles was adapted, but I would check it with mo…

2d5bdfc

…re files

Raedlr added 4 commits September 12, 2024 12:50

Scan funktion was adapted for the case whenthe file contains slices w…

5bbd607

…ith different thicknesses

fixed bug

99bff69

some changes in comparePlnStf

0256b01

correction

142b82a

amitantony reviewed Oct 2, 2024

View reviewed changes

Raedlr added 3 commits October 2, 2024 15:00

radiationMode renamed back

c948a73

added test funktion for matRad_DicomImporter constructor

4ae1815

removed unnecessary lines and added check for field presence 'bixelWi…

806a94a

…dth'

Merge branch 'dev' of https://github.com/e0404/matRad into refactor/d…

fe02bb8

…icomImporter

wahln added 3 commits October 3, 2024 00:16

* added helper function to use temporary directory

66825be

* temorary directory used for dicom export and reimport * removed GUI elements from folder scanning

add actual export test

f616715

use temporary folder helper for example tests

2a3c895

enforcing dicom standard of storing slice thickness as string

1dfb220

wahln reviewed Oct 8, 2024

View reviewed changes

amitantony and others added 6 commits October 8, 2024 18:05

Update matRad_scanDicomImportFolder.m

c425155

questdlg error for octave

Update matRad_scanDicomImportFolder.m

1705db5

Update matRad_scanDicomImportFolder.m

d56ac92

replacing rmmissing (not supported on octave)

respect waitbar settings and loglevels in dicom import

4ecde64

fix in DICOM exporter to pass tests

f89d141

disable octave asking for deletion of temporary test directory

0d9f177

Merge branch 'dev' into refactor/dicomImporter

97fdc68

wahln approved these changes Oct 9, 2024

View reviewed changes

wahln merged commit 5d6be04 into e0404:dev Oct 9, 2024
5 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dicom importer #758

Refactor dicom importer #758

Raedlr commented Aug 28, 2024

wahln commented Aug 28, 2024

amitantony left a comment

amitantony Aug 29, 2024

Raedlr Sep 4, 2024

amitantony Aug 29, 2024

amitantony Aug 30, 2024

Raedlr Sep 4, 2024

amitantony Aug 30, 2024

amitantony Aug 30, 2024

amitantony Aug 30, 2024

Raedlr Sep 4, 2024

amitantony Aug 30, 2024

amitantony Aug 30, 2024

Raedlr Aug 30, 2024

amitantony Oct 2, 2024

Raedlr commented Sep 6, 2024

amitantony Oct 2, 2024

amitantony Oct 2, 2024

amitantony Oct 2, 2024

amitantony commented Oct 2, 2024

wahln commented Oct 2, 2024

wahln left a comment

wahln Oct 8, 2024

wahln Oct 8, 2024

wahln commented Oct 9, 2024

Refactor dicom importer #758

Refactor dicom importer #758

Conversation

Raedlr commented Aug 28, 2024

wahln commented Aug 28, 2024

amitantony 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

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

Raedlr commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amitantony commented Oct 2, 2024

wahln commented Oct 2, 2024

wahln left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wahln commented Oct 9, 2024