Read dicom tags

[floryst]

This adds an API for reading DICOM tags from DICOM object files. Additionally, appropriate character set conversion is performed, decoding encoded strings into UTF-8.

Relevant issue: #394

Todo:

• add async version
• add browser version
• add charset decoder
• Add docs
• Make API and docs consistent

[floryst]

FYI @agirault

[agirault]

Can you add usage documentation @floryst ? or an example?

[floryst]

I'm not sure why I can't request reviewers, so here: @thewtex @agirault.

This is still in a rough shape, so apart from a code review there are a few questions/remarks remaining:

• The API is readDICOMTags(fileList, tags=[]). I think the API would make more sense if we simply read the tags from a single file instead, i.e. readDICOMTags(file, tags=[]). I pretty much copied this from readImageDICOMFileSeries, and in doing so kept the multiple file parameter fileList. Initially the rationale was that, performance wise, its probably better to spawn one webworker to get the tags for a set of files rather than many webworkers, but this could likely be remedied by letting the API pass in an existing webworker anyways.
• Would it make sense to support returning all tags, rather than specifying which ones the user wants? If so, I think setting tags=null|undefined can mean "return all tags".
• I haven't tested private tags. What would the expected behavior be for private tags, since we don't necessarily know the tag's VR? (This is partially a GDCM question.)
• Code style: is there an automatic c++ style enforcer for itk-js? My c++ code currently has a mix of the itk-js c++ coding style and the clang c++ codingstyle and I wanted to ask before making manual changes to match the itk-js c++ coding style.
• libiconv: if the docker container installed the requisite compilation headers for iconv (which might be provided by libc6 rather than a dedicated package), then I wouldn't need to add iconv as an external project. As it stands, the ExternalProject is the only way to use iconv, which is why I include it.

[floryst]

These parameters may change, pending API discussions.

• fileList: probably going to become a single file rather than a list of files
• tags: if we want a null/undefined tags arg to mean "give me all tags", then this will stay. Right now there is no logic for getting all tags.

[thewtex]

+1 for the single file. Specification of the tags-per-file is simpler this way, too. +1 for the current null returns all tags API.

[thewtex]

@floryst awesome!! 🚀 👨‍🎤

Additionally, appropriate character set conversion is performed, decoding encoded strings into UTF-8.

This is fantastic! Can we add a test to ensure its functionality?

I'm not sure why I can't request reviewers, so here: @thewtex @agirault.

@floryst after accepting the invitation to the repository (just sent), you should be able to request reviewers.

I think the API would make more sense if we simply read the tags from a single file instead

+1

Would it make sense to support returning all tags, rather than specifying which ones the user wants? If so, I think setting tags=null|undefined can mean "return all tags".

+1

I haven't tested private tags. What would the expected behavior be for private tags, since we don't necessarily know the tag's VR? (This is partially a GDCM question.)

In the future, if this is possible and needed, we could add a third argument, privateTags=false.

Code style: is there an automatic c++ style enforcer for itk-js? My c++ code currently has a mix of the itk-js c++ coding style and the clang c++ codingstyle and I wanted to ask before making manual changes to match the itk-js c++ coding style.

Thanks for asking! The code is looking very clean, btw. We should use the ITK clang-format style. To apply it to the current code, download the clang-format-8 executable, and run it on the source code files with the ITK/.clang-format configuration file.

We gave a pre-commit hooks setup for ITK that runs clang-format -- we could add this to itk.js sometime.

libiconv: if the docker container installed the requisite compilation headers for iconv (which might be provided by libc6 rather than a dedicated package), then I wouldn't need to add iconv as an external project. As it stands, the ExternalProject is the only way to use iconv, which is why I include it.

This is awesome. While we could add the package to the docker image, I think the ExternalProject is better because we can change the base image Linux distribution and Linux distribution version, and not have to worry about losing the required headers or them moving around. We can also independently update the libiconv version.

[thewtex]

Lovely!

[thewtex]

We want this to defined outside function scope so to re-use across multiple runs.

[thewtex]

+1 for the single file. Specification of the tags-per-file is simpler this way, too. +1 for the current null returns all tags API.

[floryst]

Good timing, I was just starting to rewrite this PR to read in a single file instead of a list. I'll push that out shortly, after some build resolutions.

[floryst]

• I've switched to using a single-file API with a reusable webworker for the browser API.
• passing null for tags (or really anything that fails the Array.isArray test) will return all tags.
• Tag maps are returned as Map objects rather than plain objects.

[thewtex]

@floryst beautiful!