Value conversion to native Python types

[ZviBaratz]

Hi maintainers.

TL;DR: Would VR-informed conversion to native Python types be a welcome addition to pydicom's functionality?

A few years ago, I needed to be able to extract a lot of DICOM metadata as native Python types (e.g., DA to datetime.date, DS to float, PersonName to dict, etc.), and I ended up writing some code to do so. Eventually, I packaged the code and published it as dicom_parser, and even moved it to an organizational account (“Open DICOM”), in which I had hoped to centralize some higher-level testing and parsing resources. This never really came to fruition, mostly due to a personal lack of time to push it forward in the last couple of years.

I am hoping to soon have a little more time on my hands, and realized that while dicom_parser ended up providing some useful functionality in addition to the type conversion (such as a Series class to access and summarize whole series, and automatic Siemens' mosaic parsing), ideally, I actually think it would have been better if it was simply integrated into pydicom. However, I realize this might also be considered somewhat out of scope – pydicom does what it does very well, and adding type conversion heuristics, let alone vendor-specific parsing schemes, could add substantial complexity to the codebase.

dicom_parser isn't written particularly well. There are a couple of foundational mistakes in its (lack of) design that, I think, would justify a rewrite if I intend on maintaining it. Before I do so, I wanted to check what's the general feeling towards simply deprecating it and incrementally moving its functionality into pydicom. Again, I entirely understand this might be out of scope, but since it is not necessarily the case and I couldn't find any relevant statement on this matter in the docs, I thought it would be better to simply raise it here for discussion.

In any case, thank you very much for all your amazing work on pydicom 🙏 ✨

[darcymason]

Hi,

First, thanks very much for your interest and offer to contribute to pydicom - it is much appreciated.

My TL;DR on initial first look would be: it depends, we could look into it a bit more. The idea of a Series class is certainly one that I think is due to be incorporated into pydicom because it is so frequently asked about and everyone needs it.

The longer (initial thoughts) answer is: I was just about to put out a call for ideas on the future of pydicom, and start asking for input from various stakeholders, and try to develop a roadmap for pydicom for the next few years. So I would prefer to wait on any new major contributions until we have a better sense of what might come out of that bigger discussion. I'm quite busy this week so that process will probably start next week.

Now, to get into a little more detail, I'd also like to look at your code when I can. Based on your post above, I'm not clear on how some of your type conversions are different than pydicom, e.g. pydicom does offer datetime conversion as an option, and DS as float. So again, when I have time next week I could look into that more. Also, if you haven't seen it, perhaps you could look at discussion #1742 about dict access and see if that has any relevance - I had wondered about adding a function like that to pydicom.

Again, thanks for your interest and offer to contribute - this post is quite timely, as it does motivate me to put out that bigger call for ideas and a future roadmap as soon as I can. I encourage you (and anyone else interested) to watch for that discussion and contribute your thoughts.

[ZviBaratz]

Hi @darcymason, thank you very much for your detailed response.

First, I would like to apologize for my mistake – I can see now that pydicom does provide automatic datetime conversion by setting the config.datetime_conversion flag. I was not aware of this at all, which is embarrassing, but on a more constructive note, I think it could be made easier to find in the documentation. If you agree, I would be happy to come up with a PR. The same goes for the real attribute of IS and DS elements.

If you do look at dicom_parser's repo, you'll notice that the conversion functionality is extremely over-engineered. This is the result of (a) my lack of experience, (b) my incorrect estimation of the proportion of VRs that will require conversion, and (c) the original purpose of the code; the reason I started working on this at all was as part of a Django app for managing a database of DICOM files (django_dicom). What became dicom_parser was originally just a module with some utilities in django_dicom. These decisions made more sense at the time.

I actually did happen to see #1742. That functionality is available in dicom_parser, and used by default to display headers, e.g.:

             Keyword                      VR                VM  Value                       
Tag                                                                                         
(0008, 0000)                               Unsigned Long     1   906                        
(0008, 0005)  SpecificCharacterSet         Code String       1   ISO_IR 100                 
(0008, 0008)  ImageType                    Code String       4   ('ORIGINAL', 'PRIMARY', ...
(0008, 0012)  InstanceCreationDate         Date              1   2022-12-28                 
(0008, 0013)  InstanceCreationTime         Time              1   14:45:31.508000            
(0008, 0016)  SOPClassUID                  Unique Identifer  1   1.2.840.10008.5.1.4.1.1.4  
(0008, 0018)  SOPInstanceUID               Unique Identifer  1   1.3.12.2.1107.5.2.43.660...
(0008, 0020)  StudyDate                    Date              1   2022-12-28                 
(0008, 0021)  SeriesDate                   Date              1   2022-12-28                 
(0008, 0022)  AcquisitionDate              Date              1   2022-12-28                 
(0008, 0023)  ContentDate                  Date              1   2022-12-28                 
(0008, 0030)  StudyTime                    Time              1   14:18:25.270000            
(0008, 0031)  SeriesTime                   Time              1   14:45:31.504000            
(0008, 0032)  AcquisitionTime              Time              1   14:40:55.712500            
(0008, 0033)  ContentTime                  Time              1   14:45:31.508000            
(0008, 0050)  AccessionNumber              Short String      0                              
(0008, 0060)  Modality                     Code String       1   Magnetic Resonance         
(0008, 0070)  Manufacturer                 Long String       1   SIEMENS                    
(0008, 0080)  InstitutionName              Long String       1   Tel-Aviv University        
(0008, 0081)  InstitutionAddress           Short Text        1   Street StreetNo,.,Distri...
(0008, 0090)  ReferringPhysicianName       Person Name       0   {'name_prefix': '', 'giv...
(0008, 1010)  StationName                  Short String      1   AWP66024                   
...

Sequences can also be interpreted as dataframes, e.g.:

Sequences
=========
Tag:            ('0008', '1140')
Keyword:        ReferencedImageSequence

                   Keyword                   VR                VM  Value                    
Index Tag                                                                                   
0     (0008, 0000)                            Unsigned Long     1   94                      
      (0008, 1150)  ReferencedSOPClassUID     Unique Identifer  1   1.2.840.10008.5.1.4.1...
      (0008, 1155)  ReferencedSOPInstanceUID  Unique Identifer  1   1.3.12.2.1107.5.2.43....
1     (0008, 0000)                            Unsigned Long     1   94                      
      (0008, 1150)  ReferencedSOPClassUID     Unique Identifer  1   1.2.840.10008.5.1.4.1...
      (0008, 1155)  ReferencedSOPInstanceUID  Unique Identifer  1   1.3.12.2.1107.5.2.43....
2     (0008, 0000)                            Unsigned Long     1   94                      
      (0008, 1150)  ReferencedSOPClassUID     Unique Identifer  1   1.2.840.10008.5.1.4.1...
      (0008, 1155)  ReferencedSOPInstanceUID  Unique Identifer  1   1.3.12.2.1107.5.2.43....

Again, the code relies on the overcomplicated DataElement class within dicom_parser, which wraps all of pydicom's data elements. Another example of functionality which, I think, could be great in pydicom, and is somewhat poorly implemented in dicom_parser.

In any case, thank you again for the comprehensive answer and all your great work. I would be happy to hear more about upcoming plans and the future of pydicom, and perhaps also contribute, if I can.

[darcymason]

I think it could be made easier to find in the documentation. If you agree, I would be happy to come up with a PR.

Sure, I agree it could be easier to find. After a quick look at the pydicom documentation again, I get the feeling that it is fairly good at the 'quick-start' end, and at the reference guide end, but there could be a lot more in the middle, particularly around how types are represented and so on. There is a page with details, but perhaps it would be nice to introduce something a little more advanced from the beginning of the user guide, that talks about a few types with examples, then points to that page. Anyway, if you have some ideas about how to position something like this, I'd be happy to hear them.

the code relies on the overcomplicated DataElement class within dicom_parser, which wraps all of pydicom's data elements.

I think you are right in worrying about too much wrapping (although as noted before I haven't looked at your code in any detail yet). From time to time, when I have looked into several other libraries that use pydicom, I do find they often wrap pydicom classes, so your code is certainly not alone in that. But IMO, when you have a standard like the DICOM Standard, it makes sense to stick to it as much as possible. We have really tried to do that with pydicom, so contributions should most likely stay with the existing classes, unless by wrapping they are mapping to some kind of object in the DICOM standard that we don't already cover..

[ZviBaratz]

I get the feeling that it is fairly good at the 'quick-start' end, and at the reference guide end, but there could be a lot more in the middle, particularly around how types are represented and so on. ... Anyway, if you have some ideas about how to position something like this, I'd be happy to hear them.

Sure, give me another week or two to submit my thesis and recuperate. I'll come up with something.

IMO, when you have a standard like the DICOM Standard, it makes sense to stick to it as much as possible.

I absolutely understand and agree. That was also part of my motivation to start dicom_parser at the time. pydicom has a clear purpose and scope, and that should not be meddled with.
Perhaps it might be best for the pydicom organization to add one more package for higher-level functionality, particularly such as the Series class. This could help maintain a separation of responsibility, where pydicom/pydicom offers an extremely stable, standard-compliant, and somewhat lower-level API, and pydicom/?????? provides a higher-level, more flexible and experimental interface on top of it.

[darcymason]

Perhaps it might be best for the pydicom organization to add one more package for higher-level functionality, particularly such as the Series class. This could help maintain a separation of responsibility, where pydicom/pydicom offers an extremely stable, standard-compliant, and somewhat lower-level API, and pydicom/?????? provides a higher-level, more flexible and experimental interface on top of it.

Layering of other libraries with pydicom (within the Python in DICOM github org) is certainly one of the things to explore with the roadmap discussion. I hope to post that call for input within the next day or two.