Support for DICOM Templates TID 3802 Cardiovascular Patient History, TID 3700 ECG Report and TID 2000 Basic Diagnostic Imaging Report

[HalvarLilienthal]

Hi,

at @Cardio-AI we've extended Highdicom to support the following DICOM Templates:

• TID 3802 Cardiovascular Patient History
• TID 3700 ECG Report
• TID 2000 Basic Diagnostic Imaging Report

As we've already discussed with you, we'd like to contribute our work to this project by creating a new feature branch for a pull request.
Our contribution consists of ~40 new classes of template implementations, content items, and units, it should contain everything that is needed to represent these three templates in Highdicom.
While we implement tests and verify we enforced your coding style guideline and PEP8 compliance, we'd like to know if and where in your codebase you'd like us to add these extensions.

[CPBridge]

Hi @HalvarLilienthal thank you for reaching out about this! This is a good question. The simple/default answer is that all new template implementations go into highdicom.sr.templates, but I worry that if we continue to add things there it may become rather too large.

I think what probably makes more sense would be to change highdicom.sr.templates into a sub-package/directory (it is now a module/file), and then add the classes related to these new top-level templates in three new sub-modules there (one for cardio, one for ecg, one for basic imaging). The current contents of templates.py would go into a new sub-module specifically for TID1500 measurement report (and we would figure out how to re-export them for backwards compatibility). There may also need to be a highdicom.sr.templates.common if some sub-templates are re-used. How does this this sound to you?

[CPBridge]

I.e. something like this

src/
└── highdicom/
    └── sr/
        └── templates/
            ├── common.py  # not sure whether this will be needed
            ├── tid1500.py  # contents of current template.py
            ├── tid2000.py
            ├── tid3700.py
            └── tid3802.py

[HalvarLilienthal]

Hi @CPBridge, thank you for the implementation suggestions!

I've already set up this file structure in a local branch and so far I've not run into any issues.
The backwards compatibility also shouldn't be a problem if we provide the currently exported classes in a templates/__init__.py-file.

We fixed remaining coding style and PEP8 compliance issues and are now working on adding the tests.
After that is done, I assume I just push it to a new feature/TID3802TID3700TID2000 branch and create a push request?

Btw, there are a few outstanding implementation details and I am interested in your opinion. We can postpone this discussion to after I've pushed the code so you can see it in detail but I'll give you a summary already:

1. Some Template Definitions seem to be missing necessary implementation details (TID 2002, TID 2001 and TID 3715)
2. Some UNITS are not defined in pydicom.sr.codedict.codes.UCUM (beats, bpm, MilliVolt, kg and MilligramPerDeciliter)
3. Since every DICOM template is made up of other templates, it seems arbitrary which template "deserves" its own file in the templates folder and which templates are in the common.py file. If you plan to add more templates in the future you might have to restructure the template folder code or run into circular dependency issues.

[CPBridge]

Hi @HalvarLilienthal

Excellent, thanks.

I assume I just push it to a new feature/TID3802TID3700TID2000 branch and create a push request?

Yes please

Some Template Definitions seem to be missing necessary implementation details (TID 2002, TID 2001 and TID 3715)

I'm not sure what you mean by "missing necessary implementation details".

Some UNITS are not defined in pydicom.sr.codedict.codes.UCUM (beats, bpm, MilliVolt, kg and MilligramPerDeciliter)

I am not an expert on UCUM but I believe pydicom contains a "convenient" subset of all possible UCUM codes (there are a huge number of possibilities as I understand it as they can be combined). It looks like at least some of the ones you need are there, e.g. Millvolt. Others are not, which perhaps isn't surprising as they not very common. But that's fine, you can just create the CodedConcept yourself manually for those cases. I will say though that kg not being there is rather baffling. Maybe we should create a PR to pydicom to add it

Since every DICOM template is made up of other templates, it seems arbitrary which template "deserves" its own file in the templates folder and which templates are in the common.py file. If you plan to add more templates in the future you might have to restructure the template folder code or run into circular dependency issues.

I agree this could be a problem and may require further thought. My initial thought was that templates that are typically "top level" have their own file (e.g. TID1500). Sub-templates that are shared between multiple top level templates go in common, and sub-templates that are generally only used within a single top-level template are placed alongside that top-level template

[HalvarLilienthal]

Good news, I've now created the new branch on our fork and opened up a draft pull request #365.

While the code, in its current form, is a good starting point which passes coding style checks and its tests, there are a few outstanding design choices we should take a look at before going ahead with the pull request.

• Units: Thank you for telling me about UCUM.Millivolt, I've missed that. The other missing units are now defined as CodedConcepts just as you suggested, let me know if this works for now.
• TID 3802: The Problem List has many Problem Properties which only differ by which specific Problem they describe. Should each of those be a class or should the ProblemList class enforce the correct Problem for each ProblemProperties included?
• TID 3715: Has three root elements and a Waveform Value Type without a concept name. I am not sure how to represent this in code.
• TID 1210: This template definition has two root elements. Due to it either inheriting from TextContentItem or from CodeContentItem, we've created two classes. This could potentially be merged into a single class.
• TID 2002: Three possible root elements one of which is a INCLUDE Value Type. I don't know what the best solution for this case is. Also we probably need to add TID 2001 as a class instead of simply representing it as a ImageContentItem.
• TID 3829: ConcernType should be a class itself but how are parameters like $Problem, $ModType and $ModValue handled? Is there an example in the highdicom codebase?

If you find other topics for discussion or problems please don't hesitate to tell me about them here or in the pull request, I really hope this will become a valued contribution.