suyashkumar / dicom

⚡High Performance DICOM Medical Image Parser in Go.
MIT License
933 stars 136 forks source link

Add `SkipPixelData` option to `Parser` #243

Closed tcheever closed 1 year ago

tcheever commented 2 years ago

Testing

$ go test -v -run TestNewParser ./...
=== RUN   TestNewParserSkipMetadataReadOnNewParserInit
--- PASS: TestNewParserSkipMetadataReadOnNewParserInit (0.00s)
=== RUN   TestNewParserSkipPixelData
--- PASS: TestNewParserSkipPixelData (0.01s)
=== RUN   TestNewParserPixelData
--- PASS: TestNewParserPixelData (0.62s)
PASS
ok      github.com/suyashkumar/dicom    0.677s
?       github.com/suyashkumar/dicom/cmd/dicomutil      [no test files]
?       github.com/suyashkumar/dicom/mocks/pkg/dicomio  [no test files]
?       github.com/suyashkumar/dicom/pkg/charset        [no test files]
testing: warning: no tests to run
PASS
ok      github.com/suyashkumar/dicom/pkg/dcmtime        0.011s [no tests to run]
?       github.com/suyashkumar/dicom/pkg/debug  [no test files]
?       github.com/suyashkumar/dicom/pkg/dicomio        [no test files]
testing: warning: no tests to run
PASS
ok      github.com/suyashkumar/dicom/pkg/frame  0.003s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/suyashkumar/dicom/pkg/personname     0.017s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/suyashkumar/dicom/pkg/tag    0.007s [no tests to run]
?       github.com/suyashkumar/dicom/pkg/uid    [no test files]
?       github.com/suyashkumar/dicom/pkg/vrraw  [no test files]
tcheever commented 2 years ago

Formal commit message aside, thank you for creating this library 😃. It has been very useful!

suyashkumar commented 2 years ago

Hi @tcheever just wanted to check in and see if you had a chance to consider the review comments? Thanks for the contribution! There may be some interplay with #245 as well as a heads up based on how these PRs progress.

tcheever commented 2 years ago

Hi @suyashkumar, thanks for the bump and the review. I did not see your comments and suggestions. I will get back to you as soon as possible.

suyashkumar commented 2 years ago

Hi @tcheever thanks for the responses, added a couple more comments for your consideration!

tcheever commented 2 years ago

@suyashkumar, if I'm understanding correctly, you do not want the option in the reader at all and you do not want the arg added to NewReader?

NMerzVerily commented 1 year ago

Having an option like this would also be useful for us.

rronan commented 1 year ago

We are also interested in that option, I'd be happy to help if needed.

suyashkumar commented 1 year ago

Thanks folks for the interest! I've circled back to this in #255, including making structural changes I wanted in #254 to support better option availability to read* methods in read.go (which was the subject of my comments in the review discussion here above).

Thanks @tcheever and others for the discussion and interest in this useful feature!

suyashkumar commented 1 year ago

Going to close this for now, since this feature was introduced in #255 with the new options availability (merged to main)! @NMerzVerily @rronan @b3n3d17 @tcheever please feel free to kick the tires on it and let us know how it goes.