[MRG+1] Fix handling of bit-packed PixelData #627

fedorov · 2018-04-20T22:26:00Z

Reference Issue

Aims to resolve the problem identified in #293 (review)

What does this implement/fix? Explain your changes.

bits were unpacked from the left, which was incorrect, see discussion in
[MRG + 1] pixel_array for images with self.BitsAllocate=1 #293 (review).
Correct unpack order implemented now, whereas first pixel from the left in
the image frame corresponds to the first bit from the right in the packed
PixelData
fixed consistency checks verifying the size of the PixelData
reordered the code to perform unpacking after consistency checks
removed the dependency on numpy.unpackbits and the associated exception

Any other comments?

The functionality of the code was verified using the DICOM Segmentation image
dataset being developed in this PR (3x23x38, which illustrates a case where
bits of the individual frame are not aligned at the byte boundary, and the total
length of pixel bits is less than the number of bits required to encode the bytes
in PixelData): QIICR/dcmqi#334,
and this issue: QIICR/dcmqi#341. In turn, consistency of
the rendering of that dataset with the implementation was confirmed for independent
implementations (Brainlab and DCMTK dcm2pnm).

We might consider existing tests, since they are not sufficient to identify the bug in the original implementation of the feature (wrong bit pack order) and incorrect calculation of the expected pixel length for datasets where frame is not byte-aligned.

pep8speaks · 2018-04-20T22:26:02Z

Hello @fedorov! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on April 24, 2018 at 16:50 Hours UTC

This commit aims to fix the functionality originally introduced in pydicom#293: * bits were unpacked from the left, which was incorrect, see discussion in pydicom#293 (review). Correct unpack order implemented now, whereas first pixel from the left in the image frame corresponds to the first bit from the right in the first byte of the packed PixelData * fixed consistency checks verifying the size of the PixelData * reordered the code to perform unpacking after consistency checks * removed the dependency on numpy.unpackbits and the associated exception The functionality of the code was verified using the DICOM Segmentation image dataset being developed in this PR (3x23x38, which illustrates a case where bits of the individual frame are not aligned at the byte boundary, and the total length of pixel bits is less than the number of bits required to encode the bytes in PixelData): QIICR/dcmqi#334, and this issue: QIICR/dcmqi#341. In turn, consistency of the rendering of that dataset with the implementation was confirmed for independent implementations (Brainlab and DCMTK dcm2pnm).

mrbean-bremen · 2018-04-21T13:37:08Z

pydicom/pixel_data_handlers/numpy_handler.py

+            for bit in range(bit, bit+8):
+                pixel_array[bit] = byte & 1
+                byte >>= 1
+            bit += 1


This fails in Python 2 because byte is a str. You may need to convert it to int using ord or something.

mrbean-bremen · 2018-04-21T13:38:43Z

About the test - the current test is quite sloppy as it only checks the dimensions and a pixel in the middle and a pixel at the edge to be correct. It would make sense to check the whole pixel data, especially if we can replace the test image with a smaller one.

mrbean-bremen · 2018-04-21T13:39:30Z

And thanks for the fix!

fedorov · 2018-04-21T21:32:00Z

@mrbean-bremen thanks for the review! Let me know if you want the updated tests for this PR, or make an issue and address that separately.

mrbean-bremen · 2018-04-22T11:07:05Z

Let me know if you want the updated tests for this PR, or make an issue and address that separately.

If would be nice if you could adapt the test, as you are the one who knows the code best. If you don't have the time - a separate issue would also be fine with me, if @darcymason can go with it.

mrbean-bremen · 2018-04-22T11:13:24Z

pydicom/pixel_data_handlers/numpy_handler.py

+        #  * DICOM Annex D (examples of encoding)
+        for byte in pixel_bytearray:
+	    byte = ord(byte)
+            for bit in range(bit, bit+8):


Now it fails with Python 3. You may use if in_py2: for that line (or make separate loops should that impact the performance). Also, in the same line a tab character has crept in...

Sorry about the tab sloppyness, I used a different text editor.

About the error, it's not the ord() issue, it appears that elements of pixel_bytearray have different type between python 2 and 3! This should be fixed now.

Yes, this is because a bytearray in Python 3 is really an array of bytes (where each element has int type), whereas in Python 2 a str is used as bytearray (there is no other), and an element of a str is a str.

Type of pixel_bytearray elements changes between python 2 and 3!

fedorov · 2018-04-22T18:19:42Z

If would be nice if you could adapt the test, as you are the one who knows the code best. If you don't have the time - a separate issue would also be fine with me, if @darcymason can go with it.

I can definitely work on this, but I would prefer to create an issue and address separately, since I don't want to delay fix of the issue. But if you prefer to address in this PR, I will do it, just will take a bit more time.

mrbean-bremen · 2018-04-22T18:22:45Z

That's ok for me - thanks!

scaramallion · 2018-04-24T07:41:46Z

pydicom/pixel_data_handlers/numpy_handler.py

+        #  See the following for details:
+        #  * DICOM 3.5 Sect 8.1.1 (explanation of bit ordering)
+        #  * DICOM Annex D (examples of encoding)
+        print("Type: "+str(type(pixel_bytearray[0])))


Remove the print statement

mrbean-bremen · 2018-04-24T08:36:34Z

pydicom/pixel_data_handlers/numpy_handler.py

+                byte = ord(byte)
+            for bit in range(bit, bit+8):
+                pixel_array[bit] = byte & 1
+                byte >>= 1


On a second thought - it might not be a good idea to use isinstance() in the loop for performance reasons. Better use if in_py2.

Or you could do pixel_bytearray[n:n+1] instead. It should return a length 1 str/bytes in python 2/3, then you can just do the ord(byte) conversion without needing a type check. I suppose it depends on how expensive the in_py2 check is.

How about thinking along the lines of:

numpy.flip( numpy.unpackbits(pixel_bytearray).reshape((length_of_pixel_array, 8)), axis=1).reshape(length_of_pixel_array * 8)

I am pretty sure something like this should work....

@rhaxton can you make a PR with your proposed change to my branch https://github.com/qiicr/pydicom/tree/fix-seg-pixeldata? To me personally, the code I wrote is more readable, and I am not sure if the suggested change leads to any performance improvements (which also I am not sure are critical in this situation). I am not an expert in numpy, so I would defer to someone else doing the numpy-specific optimizations.

Actually, I take it back - I oppose this suggestion of changing to use unpackbits, since it means we would only be able to unpack in python 3. Not clear at all what is the advantage of restricting this operation to python 3 only.

Better use if in_py2

Done

scaramallion · 2018-04-24T12:31:32Z

pydicom/pixel_data_handlers/numpy_handler.py

-                numpy.frombuffer(pixel_bytearray, dtype='uint8'))
-        except NotImplementedError:
-            # PyPy2 does not implement numpy.unpackbits
-            raise NotImplementedError(


Could you fix up the failing unit test, too?

It can just be removed, likewise the skip condition on OneBitAllocatedTests.

Done (if I understood correctly how things were expected to work ...)

Hmm, interesting - don't understand why CI of the commit prior to removing those tests succeeded.

fedorov force-pushed the fix-seg-pixeldata branch 3 times, most recently from 21e5933 to 49edbf7 Compare April 20, 2018 22:34

fedorov changed the title ~~[MRG] BUG: fix handling of bit-packed PixelData~~ [MRG] Fix handling of bit-packed PixelData Apr 20, 2018

fedorov mentioned this pull request Apr 20, 2018

[WIP] Voxel Segmentation Object Support #417

Closed

fedorov force-pushed the fix-seg-pixeldata branch from 49edbf7 to 8db3f36 Compare April 20, 2018 22:41

fedorov mentioned this pull request Apr 20, 2018

pep8speaks integration AIM-Harvard/pyradiomics#376

Closed

mrbean-bremen reviewed Apr 21, 2018

View reviewed changes

mrbean-bremen reviewed Apr 22, 2018

View reviewed changes

Cast str to int to fix Python 2 runtime error

a49e826

fedorov force-pushed the fix-seg-pixeldata branch from 1c2f511 to a49e826 Compare April 22, 2018 18:07

Convert to int only if pixel_bytearray contains str

3f903aa

Type of pixel_bytearray elements changes between python 2 and 3!

mrbean-bremen changed the title ~~[MRG] Fix handling of bit-packed PixelData~~ [MRG+1] Fix handling of bit-packed PixelData Apr 22, 2018

mrbean-bremen approved these changes Apr 22, 2018

View reviewed changes

scaramallion reviewed Apr 24, 2018

View reviewed changes

mrbean-bremen reviewed Apr 24, 2018

View reviewed changes

scaramallion reviewed Apr 24, 2018

View reviewed changes

fedorov added 3 commits April 24, 2018 12:38

Remove print statement left by accident

91b8f88

Remove Py2 specific test and check that assumed the use of unpackbits()

527dc15

Use in_py2 in place of isinstance() for optimization

263e61d

mrbean-bremen approved these changes Apr 24, 2018

View reviewed changes

scaramallion merged commit a1dca34 into pydicom:master Apr 24, 2018

mrbean-bremen mentioned this pull request May 2, 2018

pixel_array doesn't work when self.BitsAllocated = 1 #292

Closed

fedorov mentioned this pull request May 2, 2018

Improve testing of unpacking of DICOM SEG bit-packed PixelData #637

Closed

scaramallion mentioned this pull request May 26, 2018

Help: Handling Overlay Data #643

Closed

fedorov mentioned this pull request Jul 30, 2018

How to use python code to read the pixel data in the segmentation file ? QIICR/QuantitativeReporting#232

Open

TseSteven mentioned this pull request Aug 2, 2019

Example of dicom overlay handling #912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG+1] Fix handling of bit-packed PixelData #627

[MRG+1] Fix handling of bit-packed PixelData #627

fedorov commented Apr 20, 2018

pep8speaks commented Apr 20, 2018 •

edited

Loading

mrbean-bremen Apr 21, 2018

fedorov Apr 21, 2018

mrbean-bremen commented Apr 21, 2018

mrbean-bremen commented Apr 21, 2018

fedorov commented Apr 21, 2018

mrbean-bremen commented Apr 22, 2018

mrbean-bremen Apr 22, 2018

fedorov Apr 22, 2018

mrbean-bremen Apr 22, 2018

fedorov commented Apr 22, 2018

mrbean-bremen commented Apr 22, 2018

scaramallion Apr 24, 2018

fedorov Apr 24, 2018

mrbean-bremen Apr 24, 2018 •

edited by scaramallion

Loading

scaramallion Apr 24, 2018 •

edited

Loading

rhaxton Apr 24, 2018

fedorov Apr 24, 2018

fedorov Apr 24, 2018

fedorov Apr 24, 2018 •

edited

Loading

scaramallion Apr 24, 2018

mrbean-bremen Apr 24, 2018

fedorov Apr 24, 2018

fedorov Apr 24, 2018

[MRG+1] Fix handling of bit-packed PixelData #627

[MRG+1] Fix handling of bit-packed PixelData #627

Conversation

fedorov commented Apr 20, 2018

Reference Issue

What does this implement/fix? Explain your changes.

Any other comments?

pep8speaks commented Apr 20, 2018 • edited Loading

Comment last updated on April 24, 2018 at 16:50 Hours UTC

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbean-bremen commented Apr 21, 2018

mrbean-bremen commented Apr 21, 2018

fedorov commented Apr 21, 2018

mrbean-bremen commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fedorov commented Apr 22, 2018

mrbean-bremen commented Apr 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrbean-bremen Apr 24, 2018 • edited by scaramallion Loading

Choose a reason for hiding this comment

scaramallion Apr 24, 2018 • 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

fedorov Apr 24, 2018 • 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

pep8speaks commented Apr 20, 2018 •

edited

Loading

mrbean-bremen Apr 24, 2018 •

edited by scaramallion

Loading

scaramallion Apr 24, 2018 •

edited

Loading

fedorov Apr 24, 2018 •

edited

Loading