Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support parsing of big-endian & RGB files. Also refactoring. #38

Merged
merged 14 commits into from
Nov 13, 2019
Merged

Support parsing of big-endian & RGB files. Also refactoring. #38

merged 14 commits into from
Nov 13, 2019

Conversation

notZaki
Copy link
Member

@notZaki notZaki commented Oct 22, 2019

This PR started as an attempt to incorporate the additions from PR #18 but then it got out of hand.
Sorry about that.

With the PR, the package is now able to read big-endian data.
However, it still can not correctly write big-endian files.
A workaround is to change the transfer-syntax by

dcm_data[(0x0002, 0x0010)] = "1.2.840.10008.1.2.1"

and then dcm_write will save the data as little-endian.

This PR also changes the optional arguments to dcm_parse and dcm_write, so it can break older code.
The optional arguments are now keywords (described in the summary below).
Any suggestions to the keywords are welcome.

I want to manually test the PR on different machines before merging it and this will probably take me a week.
If there are no objections until then, then I will go ahead and merge.


Summary of changes:

  • Support parsing big-endian data (from Handle big-endian data #18)
  • Refactor dcm_parse() into smaller functions
    • In particular: the meta info (which is always explicit VR and little endian) is parsed in a separate function while the body (which can be implicit VR or big endian) is parsed by a separate function
      • The explicitness/endianness is determined after reading the meta info. I think this makes the logic clearer than the previous version
  • Shuffled the functions around in DICOM.jl
    • I prefer the main function to appear first, followed by the smaller/supporting functions. The old version had the smaller functions first, while the two main functions (dcm_parse and dcm_write) were at the end
  • Fix some of my poor choices back in PR Get dcm_write() to work. Switch to using Dict for handling data. #22
    • Changed the variable names from camelCase to snake_case since that's the more julian style
    • Changed the optional arguments to dcm_parse. They are now all keywords and are as follows:
      • return_vr::Bool controls whether to return the VRs with the parsed data. Default: false
      • preamble::Bool controls whether the 128-bytes + "DICM" preamble is present. This was previously header. Default: true
      • aux_vr is a user-supplied dictionary which can supplement/override the dictionary in src/dcm_dict.jl. This was previously named dVR.
    • Changed the optional argument to dcm_write. It used to not be a keyword, but now it has the keyword aux_vr.
  • Update readme so that it uses the newer/above syntax
  • Fix a bug with parsing pixeldata
    • Previously, the pixel data had dimensions of xr=rows, yr=columns, where rows and columns are values from the dicom header. The updated version has the order swapped, i.e. xr=columns, yr=rows.
    • This was not caught in any of the previous tests because they all had the same number of rows and columns. However, on non-square images, the old version produces artifacts.
  • Support RGB pixeldata
    • It currently assumes that the channels are seperated (i.e. Planar configuration = 1). If the channels are interlaced then the user will have to de-interlace them.
  • Added a new test which is big-endian & RGB
  • Added windows to the travis tests & replaced the code-coverage script with a simpler/default version

@codecov
Copy link

codecov bot commented Oct 22, 2019

Codecov Report

Merging #38 into master will increase coverage by 1.66%.
The diff coverage is 90.8%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
+ Coverage    87.3%   88.97%   +1.66%     
==========================================
  Files           1        1              
  Lines         252      272      +20     
==========================================
+ Hits          220      242      +22     
+ Misses         32       30       -2
Impacted Files Coverage Δ
src/DICOM.jl 88.97% <90.8%> (+1.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fcdf88b...2fc7878. Read the comment docs.

@notZaki notZaki merged commit 4e7166c into JuliaHealth:master Nov 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants