Pydicom: [ENH] Modular Re-organization

[vsoch]

This is a direct copy of the issue from https://github.com/pydicom/pydicom/issues/395#issuecomment-315609707 for which we started discussing how to re-organize pydicom to allow for different image handlers, having a more modular framework like nibabel. See the issue starting with the comment linked above to follow along. This should likely be for (not the next) but a future release, as it will require some thought.

---

okay, I just looked through some of the old posts, and I think I can make some suggestions. These aren't ordered in any particular way, and I'm not sure which I like best - just a brain dump at this point :)

Overview of Problem

Given that pydicom is a simple "read your dicom and then let developers take over" - the approach to read in raw data and then put burden on figuring it out goes to the developer. However, a lot of researchers that are pretty with programming aren't experts at dicom, and wouldn't know a thing about dealing with compression types. We are having a hard time because the entirety of pixel data, and other tags in the header to describe it, are all represented as just Tags without any logical linking, special functions, or what not.

I think what this suggests is that pydicom has outgrown its early boots. We need representation of the pixel data that allows it to be linked to functions, and some set of tags.

Idea 1: Extraction and Fresh Save

We can imagine a workflow that advocates for any manipulation of the data to extract it into its own object (call it PixelData) first. Using nibabel as inspiration, the workflow is to read in an image, extract image data / do stuff with it, and then generate a fresh object (with header fields, etc) that you want to save it to. The extracted image data is always updated to reflect changes to type / columns / rows, and upon save (non image related fields) can be updated with the original header data.

Nibabel Example

For nibabel, it might be something like:

import nibabel
original = nibabel.load("brainmap.nii.gz")
data = original.get_data()

...dostuff...

nii = nibabel.Nifti1Image(data,affine=numpy.eye(4))
nii.save(original,"/home/vanessa/Desktop/newimage.nii.gz")

Notably, the user can interact / view header stuffs about the image at load via the original object, but to manipulate and work with the data, it is extracted with a function get_data(). When I am working with data, I'm just working with a numpy array (data) and this fits well with a lot of use cases for machine learning, image processing, etc. When / if I'm happy with my data, then I put it back into a nifti object (nii) which will make sure that things like data type, dimensions, are represented correctly.

Then my new image object (nii) is the starting point for doing a save. I hand the original object (original) back to it if I want to retain all the non-image related fields. Looking at the commands, we can deduce the following about the save:

nii = nibabel.Nifti1Image(data,affine=numpy.eye(4))

here we assume that init of this image type (note that we could arguably make other kinds of images too) is going to expect a matrix of data and one or more optional fields like an affine transformation matrix. It likely will generate fields like type / dimension based on the data that it receives.

nii.save(original,"/home/vanessa/Desktop/newimage.nii.gz")

here we would assume that we can add (non image related) header fields to our fresh image, and save in one call.

Pydicom Example

For pydicom, it might go something like this:

from pydicom import read_file

dataset = read_file("squidward.dcm")

data = dataset.PixelData      # still as is now, for compatability
data = dataset.pixel_array() # but we advocate for this

...dostuff...

For the second data above, we aren't doing anything other than getting back a numpy array. Or we might have it be like nibabel, dataset.get_data() and then have several options / functions to let the user (likely a developer) decide how/what to return. Then after the user is done with the data, he/she wants to save a new image:

dcm = pydicom.Image(data, .... )

If we have more than one kind of Image, we might consider doing what nibabel does (note nifti, gifti, cifti, minc2, etc.), and have an image class for each type. My guess is that this might be a good strategy for the different kinds of image handlers you were talking about?

Then when we save, we save the new dcm (associated with a particular image handler) and can optionally hand it non image related tags from the original image:

dcm.save(dataset,"/home/vanessa/Desktop/new-squidward.dcm")

Important Points

• a dataset does not take bias about the kind of image handler the user wants, it still provides the same raw data / fields
• A user that wants to interact with the image data (as a data frame / numpy array / other) must specifically ask for it with get_data()
• A user that wants a particular kind of transformation / other applied to the data does so by instantiating a new object (eg, the Image or Nifti1Image in the examples)

Pros to this approach are that we can conserve most of the current code base, and having the additional handlers / save functions is like an extension of it. The cons are that using the module is still very developer oriented (less dumb "I don't know anything about dicom" user friendly) (@vsoch raises hand).

Idea 2: Separation of Header/Image via Modules

As said in the problem statement, some of the issues we are hitting could be helped if pydicom had another level of representation / control of the different data. For this idea, the dataset.py is still the entrypoint for creating a dataset, except instead of putting everything into one Dataset object with a bunch of tags, the generation of a dataset (under the hood) initializes a separate ImageData object (or whatever you want to call it) that carries its own set of functions / classes in its respective folder:

pydicom/
     dataset.py
     tags.py
     pixels/
         utils.py
         handlers.py
         transform.py
         __init__.py

Notably, the pixels module would be exposed to the user based on imports into __init__.py. The idea then would be the image object (let' call it dataset to be consistent) has always one image handler that also manages fields from the header related to the data. I don't have preference / opinion for a default, but from what I've seen on this board, it seems like the default of Bytes would be most wanted. So this means that when an image is first read in, we instantiate some BytesPixelData object to be associated with the image (that also handles the tags that are relevant to its size, etc.) and the user can access this via the dataset.PixelData Changes to dataset.PixelData would then be done via functions specific to the BytesPixelData class to update the corresponding tags associated with it.

But now the user wants to do something with the data - some transform / decompression / etc. For this, we would require them to do like with nibabel, to have some dataset.get_data() (then do manipulations) and then to put it back into a new image handler. Like:

data = dataset.get_data()

... do stuff...

We could either have them create a new dataset and specify the handler:

dataset = Dataset(data=data,                        # here is the raw data
                              handler=ArrayPixelData,  # use this handler
                              dataset=dataset)              # update with this dataset

or go with the nibabel approach, and make the image object first:

image = ArrayPixelData(data)
dataset = image.to_dataset(dataset=dataset) # update with dataset
dataset.save()

Important Notes

• This approach could be either user or developer friendly by way of setting a reasonable default handler, and giving several easy ways (eg, variable, environment variable, estimate from dataset) to choose the best handler.
• the logic of saving / transforming lives with the handlers, and the dataset is (still) more of a skeleton / wrapper to carry around an image handler and some set of fields (or I guess we call them tags).
  
  
  
  • This seems like a good approach because different kinds of images are going to need different functions, and possibly even have different associated tags.
  
  
  • It affords starting with one image and then generating several instances of the same image with different image handlers.
  
  

As a possible tweak to the above, you make the image and header bits entirely separate, and require the user to ask to load the image data, forcing him/her to choose (and no guessing on pydicom's part).

Overall

The high level idea is that instead of having one level of Dataset with tags, we have different submodules, each of which has some set of associated tags/data. Because if there are truly different kinds of images, and each has some pattern of tags, pydicom could be useful in capturing that. Perhaps a user that has a special kind of image, and tag / actions, could easily extend pydicom for that purpose by adding a new handler. A handler could even go beyond image transformation, and act as a filter for an image to do things like present a de-identified view, or conditional parsing.

Each handler (eg, the instance that PixelData points to) will be an intelligent class that knows how to do things like update the rows/columns given a function it owns changes them. It can manage the raw data (bytes) or provide some other version. Each of these classes also knows how to plug back into a dataset to save, etc.

I had another idea, but I need to go home! So I'll leave it at two for now. I do think (looking at the base of code) some re-organization of the base functions would be really great. Instead of a bunch of file*.py files, for example, you can have a folder that exposes the functions through init, eg:

pydicom/
    fileio/
        __init__.py
        base.py
        reader.py
        writer.py

and then in init:

from .read import read_file

and that is exposed like:

from pydicom.fileio import read_file

although for the developer, it's very intuitive to find the function organized under fileio/reader. The other benefits to this approach is that you can have lots of interaction between functions in a submodule (fileio) without needing to expose unnecessary ones to the user.

Ok, off to home! Chat more about this later.

[bastula]

I actually had a similar idea when I set out to make dicompyler/dicompyler-core which would have modules for DICOM RT. So far only one module (DVH) is fully complete as a standalone class, but each RT IOD has functions that are represented in dicomparser.py.

This could also serve as another example parallel to nibabel/nipy.

[massich]

I also had similar things on the head, and my idea was to start by the tests and then refactor things bit by bit.
Maybe we should find a time-slot to chat all together, align ourselfs to not step on each other and make the most of our efforts.

[darcymason]

So I've been thinking a lot about this issue, trying out different things in my head and in pseudo-code.

Let's first restate the problem very briefly:

pixel_array currently returns a numpy array only. If the image was decompressed, the pixel representation may have been changed by one of the handlers; each handler might change it differently. In such cases, other data elements in the dataset should be updated, but we don't want to do that as a 'side-effect' that the user is unaware of.

So the root of the problem is that the existing pixel_array is not sophisticated enough to communicate back to the user what has happened. We need to carry more information somehow. I think we all have been talking around something like option (1) below, but here I compare it to a couple of other options:

Option (1): Return a Module object with the decompressed image and associated data elements

The Module could be just a small Dataset. Then the user could just update (standard dict method) the original dataset if they wanted to convert it to the new values:

image = ds.ImageModule(options,...)
image.PixelData[:20, :20] = 0  # could use pixel_array rather than PixelData, different discussion
image.Rows = 50
...whatever you do to the image...
# Now set the pixel data (and other modified values) back into original dataset
ds.update(image)
ds.save_as(...)

This is very similar to the mid-section of @vsoch's Idea 2 modelled on nibabel, but instead of separate functions, it just uses the built-in update of python dicts (and hence Dataset).

Option (2): Return a new Dataset

Similiar to the above, but instead of an image module, return a copy of the entire dataset, not just the related data elements:

new_ds = ds.decompressed([options...]) # all data elements updated according to the returned image type
new_ds.PixelData[:20, :20] = 0
new_ds.save_as(...)

(2b): decompressed could instead be a module function:

from pydicom import decompressed  # or 'uncompressed'?
...
new_ds = decompressed(ds, options,...)

Option (2b) is akin to python's sorted function

Option (3): Modify dataset in-place:

ds.decompress(options,...)  # again, this is like `sort` rather than `sorted`
ds.PixelData[..] = ...
ds.save_as(...)

Option (3) is akin to python's list.sort method rather than sorted like in 2b.

For all of these, options could (eventually) include specifying particular conversion handlers, or desired pixel representation -- e.g. RGB, etc.

All of these options could be implemented and be mutually consistent: (2) and (3) could just use the ImageModule 'under the hood' and for (2) return a deepcopy of the original dataset (plus ds.update() with the ImageModule), or in (3) just do the ds.update() in-place.

All of these have the advantage that the decompression is an explicit step in the user code. With these options, any 'side-effects' would be part of the API -- if getting an ImageModule, the data elements would be updated along with the pixel data, if asking for decompressed (in-place or not) the other data elements will be updated accordingly. So if someone uses the in-place, they may need to check other data elements afterwards to know how to properly use the pixel data.

I would propose that the current pixel_array be kept, but only for uncompressed data. If requested on a compressed image dataset, an exception would be thrown, and the help message would point out the extra explicit step needed (whichever we choose). Or, at the least, an exception for any case where a handler needs to update another data element. For lossy compression, that would be all of them, because by the dicom standard, the image type should become "derived" for the uncompressed version of it.

Aside: I did look a bit at nibabel. I think this is somewhat similar, but instead of using separate 'header' information, it just uses a Dataset which is already so natural to dicom (and pydicom) with all the built-in machinery to access data elements containing the image information. It could happen as a separate piece (option 1) but then why not carry along all other information too? (option 2 and 3), and then we don't need to create this intermediate which has only part of the information.

Does anyone have any other options to add to the discussion?

Options 2 and 3 are not particularly hard to do. Option 1 would require more thought. Would the ImageModule be the General Image IOD or would it be a more detailed type (CT, MR, US, CR, NM,...); it would likely depend on the situation.

So, what are everyone's thoughts? I'm eager to get this issue behind us, it's the last big thing that must be done for the 1.0 release.

[vsoch]

Okay, I will just leave a dump of thoughts!

pixel_array currently returns a numpy array only. If the image was decompressed, the pixel representation may have been changed by one of the handlers; each handler might change it differently. In such cases, other data elements in the dataset should be updated, but we don't want to do that as a 'side-effect' that the user is unaware of.

I think we want to achieve two things:

• make it seamless and not painful for the average user that has no idea what compression / etc are, and don't hide anything
• allow for different handles / compression for developers / more advanced users.

If we take a strategy to default to the simplest (base / no special things done) case, the point 1 is satisfied, because the data is loaded, no changes are hidden. If the user wants to do* something with the data that isn't allowed given the type, then he/she is given an error/warning saying "You need to do this transformation / handler first"

I think there are a few ways of going about it - try to capture (in specific modules) all the different handlers that could be wanted, and then have like:

from pydicom import read_file
original = read_file("image.dcm")
transformed = original.decompress(tags_allowed=[....])

and that function to decompress would handle updating headers too. But there is issue in that it isn't clear what headers are changed. It could look more like this, which might better imply "I'm giving you a new thing" so minimally they could be compared.

from pydicom import read_file
from pydicom.handler import DecompressedImage

original = read_file("image.dcm")
updated = DecompressedImage(original)

and that would be a handler to update header, and could optionally take some custom header too:

from pydicom import read_file
from pydicom.handler import DecompressedImage

original = read_file("image.dcm")
header = original.get_header()

# <do stuff to header>

updated = DecompressedImage(data=original, header=header)

We would want any issues with the image handler and header to be loud and clear to the user, who is most likely a developer of some sort.

We could even do away with providing the handlers as specific dataset modules, and just leave it up to the user to handle the data transform via helper functions:

from pydicom import read_file
from pydicom.dataset import Dataset

original = read_file("image.dcm")
header = original.get_header()
data = original.get_data()             # this mirrors nibabel

from pydicom.handler import decompress_image
decompressed = decompress_image(data)

# <do stuff to image headers>

updated = Dataset(data=decompressed, header=header)

the drawback to that last one is that it might be dangerous to decouple header data and image data (if changes from the handler need to be reflected in the header), so this approach might not be best.

What I DO really like is the idea that we have some base skeleton for a "handler" that modifies image and header data in some way, and then others can contribute their specific recipes. For example, right now the module deid is doing some custom parsing and changes to headers / pixel data, and some of that work could be captured via a custom module that returns a cleaned dataset.

Option (1): Return a Module object with the decompressed image and associated data elements

The Module could be just a small Dataset. Then the user could just update (standard dict method) the original dataset if they wanted to convert it to the new values:

Oh this is a cool idea - so essentially return the "diff" of a current dataset against some transform, and then let the user decide what to update? I would want to make it something called a Handler, here is your example slightly modified:

from pydicom.handlers import DecompressedImage
image = DecompressedImage(ds)
image.PixelData[:20, :20] = 0 
image.Rows = 50

# Maybe some way to compare the two here?

ds.update(image)
ds.save_as(...)

And maybe there could be a "force save" option that would perform the ds.update, and return it

from pydicom.handlers import DecompressedImage
ds = DecompressedImage(ds, force=True)
ds.save_as(...)

[vsoch]

I clicked comment when I wasn't done yet :) Here is the rest!

Option (2): Return a new Dataset

Similiar to the above, but instead of an image module, return a copy of the entire dataset, not just the related data elements:

I generally think we should always return a new thing, unless the user specifies something like force or inplace. I could see either working ok, so I don't have a strong opinion on this one. A thought about this:

from pydicom import decompressed  # or 'uncompressed'?
...
new_ds = decompressed(ds, options,...)

Why would the user want to start with a ds to being with? Why not:

from pydicom.dataset import Dataset
from pydicom.dataset import DecompressedDataset
...
new_ds = DecompressedDataset("image.dcm")

Option (3): Modify dataset in-place:

yeah I like that one ok too, I think either is good as long as we are clear about it.

I would propose that the current pixel_array be kept, but only for uncompressed data. If requested on a compressed image dataset, an exception would be thrown, and the help message would point out the extra explicit step needed (whichever we choose).

That seems reasonable.

Aside: I did look a bit at nibabel. I think this is somewhat similar, but instead of using separate 'header' information, it just uses a Dataset which is already so natural to dicom (and pydicom) with all the built-in machinery to access data elements containing the image information. It could happen as a separate piece (option 1) but then why not carry along all other information too? (option 2 and 3), and then we don't need to create this intermediate which has only part of the information.

I agree that the intermediate could be a hassle, but on the other hand, if it were possible to extract simpler versions of essentially a data matrix and dictionary of values, do updates, and then write a proper image again, that seems very useful.

I don't have more thoughts on this, but I really like the idea of having flexibility to create custom (and contribute them!) handlers. I'm pro "stupid and simple and transparent."

[darcymason]

Thanks for the detailed response, @vsoch. I really like some of the ideas and comments, but I'll wait to see if there are other comments from @pydicom/pydcom-fff or anyone else who would like to contribute.

[vsoch]

Sounds good. I'll think more about this too - it's been a while!

[mrbean-bremen]

Just one quick thought - generally, I think that option 2 is the cleanest from the DICOM standpoint. As the new object is another DICOM object, with another SOP Instance UID, I think it is clearer for the user. Updating an existing data set which the user may already have used could be unexpected, as the data set remains the same object, so it is easier to introduce subtle bugs.
That being said there might be some performance advantages for the other options, though I'm not sure if that really matters.

[darcymason]

Back to comment on the responses. First from @vsoch:

It could look more like this, which might better imply "I'm giving you a new thing" so minimally they could be compared.

from pydicom import read_file
from pydicom.handler import DecompressedImage

original = read_file("image.dcm")
updated = DecompressedImage(original)

I'm intrigued by this DecompressedImage class, but would use DecompressedDataset instead because there is no Image class in pydicom. Its a bit different from a decompressed method of the Dataset class, but strikes me as more strongly explicit that something new is coming back. Perhaps the method of Dataset could be named decompressed_dataset() to make it more explicit.

Perhaps DecompressedDataset could actually be a subclass of Dataset, which would keep a copy of the original dataset, and be able to yield a list of differences, etc. Maybe on writing, the user could have the option of retaining things like the original instance UIDs, if they wanted to (for the use case of making a small tweak to some related set of files which reference each others' UIDs).

Why would the user want to start with a ds to being with? Why not:

from pydicom.dataset import Dataset
from pydicom.dataset import DecompressedDataset
...
new_ds = DecompressedDataset("image.dcm")

Ah, I forgot that here you have used DecompressedDataset name. This could be a convenience function to read the original and create the decompressed all in one line.

header = original.get_header()
data = original.get_data() # this mirrors nibabel
...
the drawback to that last one is that it might be dangerous to decouple header data and image data (if changes from the handler need to be reflected in the header), so this approach might not be best.

Yeah, I'm having trouble seeing the header concept transferring to pydicom. DICOM has much more than just an image header, with all the other image info, patient info, references to structure sets, etc. I don't see the value of separating the image data from all that information. It could save some space if we are making a copy of original, but the memory size of the image is far more than the associated data elements. And this goes back to my Module comments - which part is the 'header' - in DICOM there are a plethora of different image IODs. I think it would be a world of troubles to try to manage that. I'd rather see them carried together, and if the user wants the pixel data, they simply get it using PixelData or pixel_array. But I'm quite willing to understand if there is some advantage I am missing.

So, summarizing, at the moment I'm liking the idea of a DecompressedDataset class as a subclass of Dataset, which is like Option 2. We could also have a method of Dataset, decompressed_dataset(), which returns one, although it is a little more explicit if it is imported as its own class.

DecompressedDataset would keep the original info as well, and be able to offer a list of data elements which has been altered. So the user could do something like:

dds = DecompressedDataset(ds)  # or from a filename
if 'PlanarConfiguration' in dds.altered_elements:
   orig_planar = dds.original.PlanarConfiguration
   # do something with that if you want...

We could also add a flag to the newly named read_file, dcmread, to return the DecompressedDataset:

dds = pydicom.dcmread(filename, decompressed=True)

which saves the user importing the DecompressedDataset class directly.

... I really like the idea of having flexibility to create custom (and contribute them!) handlers. I'm pro "stupid and simple and transparent."

We do have a 'handlers' system set up now in master, so this could be extended to other uses too beyond just decompression. The same concepts could apply, like keeping an original and showing differences. Perhaps DecompressedDataset could be an alias for a more generally named ProcessedDataset class.

[darcymason]

Now from @mrbean-bremen:

generally, I think that option 2 is the cleanest from the DICOM standpoint. As the new object is another DICOM object, with another SOP Instance UID, I think it is clearer for the user. Updating an existing data set which the user may already have used could be unexpected, as the data set remains the same object, so it is easier to introduce subtle bugs.

Yes, I agree. The in-place update is more likely to result in troubles.

That being said there might be some performance advantages for the other options, though I'm not sure if that really matters.

I can't think of any siginificant performance issues. As I mentioned in my response to @vsoch, the image size almost always overwhelms all the remaining data. In any scenario we have discussed so far, we have both the original pixel data and decompressed pixel data, so there would be very little extra memory to copy the other data elements. But perhaps there might need to be a way for users to dump original pixels from memory if they were working with a huge number of images all in memory at once.

And in terms of time performance, the time to copy other data elements should be small compared with the time to decompress the image, so I don't see there being any issues there.

[vsoch]

Perhaps DecompressedDataset could actually be a subclass of Dataset, which would keep a copy of the original dataset, and be able to yield a list of differences, etc. Maybe on writing, the user could have the option of retaining things like the original instance UIDs, if they wanted to (for the use case of making a small tweak to some related set of files which reference each others' UIDs).

Definitely! And there are other weird ways you can add additional functionality to classes that are hacks to do the same thing. For example, for adding filters to dataset headers for deid, I just added the functions to the Dataset object. Although now that I think of it, that would mean that all subsequent Dataset instances would have those classes, so it's probably not the right thing to do.

We should generally make it easier for a conributor to subclass this "base" Dataset and add many customizations. For example, my use case was entirely to querying / filtering header fields, and another use case might be more relevant to the image data. If we have an easy template approach, then anyone with a need for "their special dataset / associated functions" could use it.

[darcymason]

Keeping this alive to revisit some of the Module concepts for v1.5 ... I'll note that the separate decompress() step was implemented.

[darcymason]

Again kicking the milestone a little further down the road. Some kind of module concept may still be useful.