Support channeled dicom slices #16

jond01 · 2020-05-11T16:54:45Z

Refactored _merge_slice_pixel_arrays to support channeled slices, e.g. RGB.
Discussion in PR #14.
Currently, there is no special test for RGB slices case, I will probably add it a bit later.

…g. RGB

johndgiese · 2020-05-11T17:07:03Z

dicom_numpy/combine_slices.py

-    if rescale:
-        voxels = np.empty((num_columns, num_rows, num_slices), dtype=np.float32, order='F')
-        for k, dataset in enumerate(sorted_slice_datasets):
+    first_dataset = sorted_slice_datasets[0]


I won't have a chance to read through this in detail until later, but two comments:

We should add a test

What happens if some slices are RGB and others aren't? Would the error message make the problem clear to a user?

BTW thanks for making a PR for this---it will definitely make dicom-numpy more useful and generic!

My pleasure :)

Yes, will add a bit later

I do not think it is a legitimate situation (?). My guess is that along with Rows, Columns, BitsStored - the attribute SamplesPerPixel is necessary for calculating pixel_array (and its shape) from PixelData. If there are some slices with different shape, they cannot be combined.
The current error will be something related to numpy's incompatible shapes.
In order to avoid it, maybe we should add SamplesPerPixel to the invariant_properties list? (in _validate_slices_form_uniform_grid)

jond01 · 2020-05-12T19:49:13Z

After inspecting in detail the combine_slices code, I found some optimization possibilities:

_sort_by_slice_position is called twice:
First in the voxels (_merge_slice_pixel_arrays),
Second in the affine (_ijk_to_patient_xyz_transform_matrix).
Obviously, it can be done once in the outer combine_slices function.
_slice_positions is called 4 times:
_validate_slices_form_uniform_grid - 1 time
_slice_spacing - 1 time
_sort_by_slice_position - 1 time, but _sort_by_slice_position is called twice --> 2 times
_extract_cosines is called 6 times:
_validate_image_orientation - 1 time
_ijk_to_patient_xyz_transform_matrix - 1 time
_slice_positions - 1 time, but _slice_positions is called 4 times --> 4 times

All these calls better be done once (and can be).
However,

this may incorporate substantial redesign of the code,
it will change the functionality of _merge_slice_pixel_arrays and _ijk_to_patient_xyz_transform_matrix, which should not be used alone probably (?), but I saw some special tests for _merge_slice_pixel_arrays.

@johndgiese - what do you think?

johndgiese · 2020-05-12T22:34:01Z

@jond01 it would be great if we could remove some or all of these duplicate calls. When this library was written we were optimizing for clarity of the code over performance. I think performance matters, so removing redundant calls would be welcome. The only situation I can see where we would not want to do this is if the refactor made the code significantly more complicated.

Regarding:

it will change the functionality of _merge_slice_pixel_arrays and _ijk_to_patient_xyz_transform_matrix, which should not be used alone probably (?), but I saw some special tests for _merge_slice_pixel_arrays.

We probably have some tests for private functions as a convenience, but if the python functions are private they are not meant to be used directly.

jond01 · 2020-05-13T08:46:06Z

Ok,

RGB slices test added
SamplesPerPixel added to invariant_properties (and MockSlice for tests), in order to avoid different values between slices

Regarding the optimizations - I do not know when I'll have time for that, so let's keep it as an optional improvement issue.

The shape of pixel_array is: pixel_array.T.shape == (rows, cols) Or (RGB): pixel_array.T.shape == (channels, rows, cols)

johndgiese

Thanks for adding the test---this looks great! I left a few suggestions for naming improvements, including for the existing names we had used (na and nb, which weren't very helpful).

johndgiese · 2020-05-13T17:20:01Z

tests/conftest.py

+        shape = pixel_array.shape
+        if len(shape) == 2:
+            na, nb = shape
+            SamplesPerPixel = 1


Suggested change

SamplesPerPixel = 1

samples_per_pixel = 1

johndgiese · 2020-05-13T17:20:12Z

tests/conftest.py

+            na, nb = shape
+            SamplesPerPixel = 1
+        else:
+            na, nb, SamplesPerPixel = shape


Suggested change

na, nb, SamplesPerPixel = shape

num_rows, num_columns, samples_per_pixel = shape

👍 except of the order:
num_columns, num_rows, samples_per_pixel = shape

johndgiese · 2020-05-13T17:20:20Z

tests/conftest.py

-        self.Columns = nb
+        self.Columns = na
+        self.Rows = nb
+        self.SamplesPerPixel = SamplesPerPixel


Suggested change

self.SamplesPerPixel = SamplesPerPixel

self.SamplesPerPixel = samples_per_pixel

johndgiese · 2020-05-13T17:20:38Z

tests/conftest.py

-        na, nb = pixel_array.shape
+        shape = pixel_array.shape
+        if len(shape) == 2:
+            na, nb = shape


Suggested change

na, nb = shape

num_rows, num_columns = shape

num_columns, num_rows = shape
(because the pixel_array is not transposed:
pixel_array[j, i] == pixel_array.T[i, j])

johndgiese · 2020-05-13T22:39:12Z

tests/conftest.py


        self.pixel_array = pixel_array

        self.SeriesInstanceUID = 'arbitrary uid'
        self.SOPClassUID = 'arbitrary sopclass uid'
        self.PixelSpacing = [1.0, 1.0]
-        self.Rows = na
-        self.Columns = nb
+        self.Columns = na


Note that your code appears to swap na and nb. My suggestions here swap them back---I am assuming your swap was unintentional.

Suggested change

self.Columns = na

self.Columns = num_columns

👍 for naming, but the swap is intentional:
#16 (comment)

johndgiese · 2020-05-13T22:39:21Z

tests/conftest.py

-        self.Rows = na
-        self.Columns = nb
+        self.Columns = na
+        self.Rows = nb


Suggested change

self.Rows = nb

self.Rows = num_rows

johndgiese · 2020-05-13T22:39:29Z

tests/conftest.py

@@ -63,5 +70,16 @@ def axial_slices():
    ]


+@pytest.fixture


johndgiese · 2020-05-13T22:39:51Z

tests/conftest.py

@@ -63,5 +70,16 @@ def axial_slices():
    ]


+@pytest.fixture
+def axial_rgb_slices():
+    # SamplesPerPixel = 3


The purpose and meaning of this comment wasn't entirely clear to me

Yep, unnecessary - removed

johndgiese · 2020-05-13T22:40:47Z

tests/test_combine_slices.py

+    def test_rgb_axial_set(self, axial_rgb_slices):
+        combined, _ = combine_slices(axial_rgb_slices)
+
+        manually_combined = np.stack([ds.pixel_array for ds in axial_rgb_slices], axis=0).T


Is the transpose expected? Perhaps it was necessary because of the na and nb swap?

It is expected because we return the transposed pixel array:

dicom-numpy/dicom_numpy/combine_slices.py

Line 98 in f0cc77d

voxels[:, :, k] = dataset.pixel_array.T

With more convenient shape:
voxels.shape == (num_rows, num_columns, num_slices)
Or:
voxels.shape == (num_channels, num_rows, num_columns, num_slices)

johndgiese · 2020-05-14T15:58:16Z

Nice work @jond01 ! Thank you :)

jond01 · 2020-05-14T16:02:39Z

You're welcome, but it seems like the last commit is missing - can you reopen this PR for a moment?

johndgiese · 2020-05-14T16:04:28Z

@jond01 sorry about that; we have not had too many outside contributors to our open source projects, so I am still getting used to the typical process.

It doesn't appear that I can re-open the PR. Would you mind making a new one with just your commit? Sorry again about that. I should have waited until you said you were ready to merge.

jond01 · 2020-05-14T16:07:11Z

Sure, no problem at all. I'm also getting used to it

jond01 added 2 commits May 11, 2020 19:37

Refactored _merge_slice_pixel_arrays to support channelled slices, e.…

9b29a87

…g. RGB

Merge remote-tracking branch 'innolitics/master' into issues11+12_fix

cb0d926

johndgiese reviewed May 11, 2020

View reviewed changes

jond01 added 2 commits May 13, 2020 11:33

Added SamplesPerPixel to invariant_properties and MockSlice

1ab1954

Added test for combining RGB dicom slices

7cc1961

jond01 mentioned this pull request May 13, 2020

combine_slices optimization #18

Open

jond01 requested a review from johndgiese May 13, 2020 08:47

Fixed Rows <-> Columns from pixel_array shape (tests)

899d277

The shape of pixel_array is: pixel_array.T.shape == (rows, cols) Or (RGB): pixel_array.T.shape == (channels, rows, cols)

johndgiese reviewed May 13, 2020

View reviewed changes

Merge branch 'master' of https://github.com/innolitics/dicom-numpy

6a8b0b4

johndgiese merged commit ee570d7 into innolitics:master May 14, 2020

jond01 mentioned this pull request May 14, 2020

Changed names in conftest.py #19

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support channeled dicom slices #16

Support channeled dicom slices #16

jond01 commented May 11, 2020 •

edited

Loading

johndgiese May 11, 2020

johndgiese May 11, 2020 •

edited

Loading

jond01 May 12, 2020

jond01 commented May 12, 2020

johndgiese commented May 12, 2020

jond01 commented May 13, 2020 •

edited

Loading

johndgiese left a comment

johndgiese May 13, 2020

jond01 May 14, 2020

johndgiese May 13, 2020

jond01 May 14, 2020

johndgiese May 13, 2020

jond01 May 14, 2020

johndgiese May 13, 2020

jond01 May 14, 2020

johndgiese May 13, 2020

jond01 May 14, 2020 •

edited

Loading

johndgiese May 13, 2020

jond01 May 14, 2020

johndgiese May 13, 2020

johndgiese May 13, 2020

jond01 May 14, 2020

johndgiese May 13, 2020

jond01 May 14, 2020 •

edited

Loading

johndgiese commented May 14, 2020

jond01 commented May 14, 2020 •

edited

Loading

johndgiese commented May 14, 2020

jond01 commented May 14, 2020

	na, nb, SamplesPerPixel = shape
	num_rows, num_columns, samples_per_pixel = shape

	self.SamplesPerPixel = SamplesPerPixel
	self.SamplesPerPixel = samples_per_pixel

Support channeled dicom slices #16

Support channeled dicom slices #16

Conversation

jond01 commented May 11, 2020 • edited Loading

Choose a reason for hiding this comment

johndgiese May 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jond01 commented May 12, 2020

johndgiese commented May 12, 2020

jond01 commented May 13, 2020 • edited Loading

johndgiese 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

jond01 May 14, 2020 • edited Loading

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

jond01 May 14, 2020 • edited Loading

Choose a reason for hiding this comment

johndgiese commented May 14, 2020

jond01 commented May 14, 2020 • edited Loading

johndgiese commented May 14, 2020

jond01 commented May 14, 2020

jond01 commented May 11, 2020 •

edited

Loading

johndgiese May 11, 2020 •

edited

Loading

jond01 commented May 13, 2020 •

edited

Loading

jond01 May 14, 2020 •

edited

Loading

jond01 May 14, 2020 •

edited

Loading

jond01 commented May 14, 2020 •

edited

Loading