Utils function for read/write value from unprocessed data #306
Conversation
There was a problem hiding this comment.
Thanks for this! Had a couple questions in the comments below. Is the main way this saves memory by only loading one frame at a time into the larger ints?
In general, having utilities for folks to work with the unprocessed pixel data after parsing does seem reasonable to me. That said, I wonder if we can potentially share some of the existing code to read native pixel data... will take a closer look next week or the following weekend to see if I can give a more direct suggestion. Thanks!
pkg/frame/inplace/read_test.go
Outdated
| for _, tc := range cases { | ||
| t.Run(tc.Name, func(t *testing.T) { | ||
| var filesOut bytes.Buffer | ||
| require.NoError(t, dicom.Write(io.Writer(&filesOut), tc.existingData)) |
There was a problem hiding this comment.
nit: let's not use require/assert, and instead just use Go code to do comparisons, see https://google.github.io/styleguide/go/decisions#assert
There was a problem hiding this comment.
Removed at 16701fa
| // ReadUnprocessedValueData read the value of Dicom image directly | ||
| // from the byte array of PixelData.UnprocessedValueData with given frame ID. | ||
| // This ease the memory usage of reading DICOM image. | ||
| func ReadUnprocessedValueData(info *PixelDataMetadata, unprocessedValueData []byte, frameIndex int) ([][]int, error) { |
There was a problem hiding this comment.
Since this returns [][]int, this will still have the same issue of expanding the memory from the smaller int to the 64-bit int right? Is the main place this saves memory in the fact that it only does one frame at a time?
There was a problem hiding this comment.
Is the main place this saves memory in the fact that it only does one frame at a time
Yes, most of the DICOM with the issues contains multiple frames.
But, I think I would change to a new structure of storing data.
B/c users need to adapt to new way of read/write pixel data anyway.
There was a problem hiding this comment.
Changed at 16701fa
|
@ducquangkstn Do you feel that #315 addresses this sufficiently? This makes a bit more of an API change, including using smaller system ints where needed. Thanks again for the support! |
Ok, I would check out the new API |
|
Hi @ducquangkstn did you have a chance to peek at the new API? |
Hi @suyashkumar, sorry for late response, I am recently busy with my company's internal development. I took a glimpse at your MR and raised few questions. |
|
@ducquangkstn no worries at all, thanks for the comments! Sent some first pass responses as well. Thanks! |
Rationale:
[[[sample for sample in pixel] for pixel in frame] for frame in image]. B/c the number of pixel per frame is usually 1M (1000 * 1000 image) and sample per pixel can be 1 => this structure takes 4x of memory.Solution: