Restructure templates file into a folder and add TID 2000, TID 3700 and TID 3802 #365
Conversation
|
Thank you! It will probably be a while before I have time to review this in depth: I want to get out version 0.26.0 soon with some important bug fixes, and that will require a bit of work on my part. Then I will return to look at this |
There was a problem hiding this comment.
Sorry for such a long wait for this, and thanks again for the PR.
I have gone through the common, tid1500, and tid2000 files and I'm sharing several important comments here. I have not yet gone through the tid3700 or tid3802 files, but I will try to do this over the next week.
At a high level, I like the restructuring into files for each top-level template. I think one of the most important concerns is the way you have been using context groups (CIDs), which are not template but define lists of codes that are suggested for use at certain places within a template.
|
|
||
| def __init__(self, | ||
| value: Union[Code, CodedConcept], | ||
| basic_diagnostic_imaging_report_observations: Optional[ |
There was a problem hiding this comment.
Generally I prefer names of parameters etc to match the language used in the standard to make things very explicit, as you have done here. However there are cases where names become too long or cumbersome and some abbreviation does make sense. I think this is probably one of those times!
How about naming this parameter observations, or perhaps report_observations?
|
|
||
| def __init__( | ||
| self, | ||
| report_narrative: Union[ReportNarrativeCode, ReportNarrativeText], |
There was a problem hiding this comment.
The items in TID2002 have multiplicity 1-n so I think you should allow a sequence here of any positive number of items of ReportNarrativeCode and/or ReportNarrativeText in any combination.
This also intuitively makes sense: there may be several items in a report, listing history, findings, impression, etc.
|
|
||
| def __init__( | ||
| self, | ||
| language_of_content_item_and_descendants: |
There was a problem hiding this comment.
I would follow the precedent we set for TID1500 here and make this an optional parameter and assume the library's default language (US English) if it is not specified by the user.
| ' "diagnostic_imaging_report_heading" ' + | ||
| 'must have type DiagnosticImagingReportHeading.' | ||
| ) | ||
| item.ContentSequence.extend(diagnostic_imaging_report_heading) |
There was a problem hiding this comment.
I think the nesting level here is wrong. It should be nested under the container item in row 8 of the TID2000 table (which currently you do not seems to be creating at all)
| """:dcm:`TID 1210 <part16/chapter_A.html#sect_TID_1210>` | ||
| Equivalent Meaning(s) of Concept Name Text | ||
| """ |
There was a problem hiding this comment.
We will need more complete docstrings than this before merging
| class AgeUnit(CIDUnits): | ||
| name = codes.DCM.SubjectAge | ||
|
|
||
| def __init__(self, year: Union[int, None] = None, | ||
| month: Union[int, None] = None, | ||
| week: Union[int, None] = None, | ||
| day: Union[int, None] = None, | ||
| hour: Union[int, None] = None, | ||
| minute: Union[int, None] = None) -> None: | ||
| self.units = [ | ||
| {'value': year, 'code_value': "a", 'code_meaning': "year"}, | ||
| {'value': month, 'code_value': "mo", 'code_meaning': "month"}, | ||
| {'value': week, 'code_value': "wk", 'code_meaning': "week"}, | ||
| {'value': day, 'code_value': "d", 'code_meaning': "day"}, | ||
| {'value': hour, 'code_value': "h", 'code_meaning': "hour"}, | ||
| {'value': minute, 'code_value': "min", 'code_meaning': "minute"} | ||
| ] | ||
|
|
||
|
|
||
| class PressureUnit(CIDUnits): | ||
| def __init__(self, mmHg: Union[int, None] = None, | ||
| kPa: Union[int, None] = None) -> None: | ||
| self.units = [ | ||
| {'value': mmHg, 'code_value': "mm[Hg]", 'code_meaning': "mmHg"}, | ||
| {'value': kPa, 'code_value': "kPa", 'code_meaning': "kPa"} | ||
| ] |
There was a problem hiding this comment.
I do not see a use for these classes.
It seems that you are trying to use them to contain multiple measurements, each encoded with a different unit. I can see why it might make logical sense to encode a patient's age as a number of years plus a number of months plus a number of days, or something similar. Perhaps that is what you are aiming for here?
Unfortunately though that is not allowed in the standard. See for example the table for TID 3704. You can see that the value multiplicity is exactly 1 for patient age, systolic pressure and diastolic pressure, meaning you must include exactly 1 NumContentItem for each of these rows in the table. So you can only give a patient's age in years OR months OR days, not a combination of these.
I think a better approach would be to get rid of these classes altogether, have the user pass a Code or CodedConcept item directly to the relevant template, and then check that the unit has one of the allowable codes defined in the relevant context group.
| BPM = Code( | ||
| value='bpm', | ||
| scheme_designator='UCUM', | ||
| meaning='beats per minute', | ||
| scheme_version=None | ||
| ) |
There was a problem hiding this comment.
bpm is not a valid UCUM code (you can check them here). I do not pretend to be an expert in UCUM, but my understanding is that a correct unit would simply be "/min", since "beats" is a dimensionless number. However, to clarify, you could include a description within curly braces to give "{H.B.}/min", which is the suggestion given in TID3713 so I would strongly suggest following that. I.e.
BPM = Code(
value="{H.B.}/min",
scheme_designator="UCUM",
meaning="BPM", # "beats per minute" could be an acceptable alternative
)| BEATS = Code( | ||
| value='beats', | ||
| scheme_designator='UCUM', | ||
| meaning='beats', | ||
| scheme_version=None | ||
| ) |
There was a problem hiding this comment.
Again this is not a valid UCUM expression without the curly braces. "beats" is just a number: it has no dimension. Therefore I think the best thing to do here is just to use"{beats}" as is done in the table for TID3713, i.e.
BEATS = Code(
value="{beats}",
scheme_designator="UCUM",
meaning="beats"
)| item = ContainerContentItem( | ||
| name=codes.LN.ECGReport, | ||
| template_id='3700' | ||
| ) |
There was a problem hiding this comment.
I noticed that the standard says this code should be (28010-7, LN, "ECG Report") but for whatever reason (I would need to look into it) codes.LN.ECGReport evaluates to Code(value='11524-0', scheme_designator='LN', meaning='ECG Report', scheme_version=None), i.e. the wrong value. For now, I would just hard code this code value
| 'have type LanguageOfContentItemAndDescendants.' | ||
| ) | ||
| content.extend(language_of_content_item_and_descendants) | ||
| if not isinstance(observer_contexts, (list, tuple, set)): |
There was a problem hiding this comment.
The usage here is 1-n, meaning at least one is required. We should therefore check that observer_contexts is not empty
| 'Argument "language_of_content_item_and_descendants" must ' + | ||
| 'have type LanguageOfContentItemAndDescendants.' | ||
| ) | ||
| content.extend(language_of_content_item_and_descendants) |
There was a problem hiding this comment.
The order of this template is significant, so the order of the table needs to be followed. Currently, the items are being added out of order (e.g. procedure reported should be before language of content items)
This comment applies to most of the other classes here, I have not left a comment on all of them
| self, | ||
| value: Union[CodedConcept, Code], | ||
| equivalent_meaning_of_value: Optional[str] = None, | ||
| ecg_findings: Optional[Sequence["ECGFinding"]] = None |
There was a problem hiding this comment.
I see why you've done it but think we need to avoid this recursive approach for at least two reasons.
- The lower level findings should have relationship type "INFERRED FROM" and the upper level findings should have "CONTAINS". Therefore you cannot use the same class for both. In the current implementation, the relationship types at the lower level are incorrect.
- Your current approach does not prevent more than two levels of findings, though this would not be allowed by the template.
A separate issue is that the parameter name ecg_findings does not in my opinion clearly convey the purpose of the lower level findings as being findings from which the upper level findings are inferred. The parameter should probably be renamed to something like inferred_from?
I think you will either need to define yet another class for the lower level "inferred from" findings, or have the user pass plain codes for the lower level findings and construct those content items internally. The latter approach does not allow for "equivalent meaning of value" text at the lower level, however.
| relationship_type=RelationshipTypeValues.CONTAINS | ||
| ) | ||
| content = ContentSequence() | ||
| if ecg_finding_text is None and ecg_finding_codes is None: |
There was a problem hiding this comment.
You should also check that they are not both empty sequences
| ecg_finding_text: Optional[str] = None, | ||
| ecg_finding_codes: Optional[Sequence[ECGFinding]] = None |
There was a problem hiding this comment.
Minor, but perhaps there could just be one parameter ecg_findings: Sequence[str | ECGFinding] and then you could check the type internally to figure out what to do with them.
There was a problem hiding this comment.
Also, maybe "ecg" in the parameter names isn't needed, given the context?
| def __init__( | ||
| self, | ||
| summary: Optional[str] = None, | ||
| ecg_overall_finding: Optional[Union[Code, CodedConcept]] = None |
| self, | ||
| summary: Optional[str] = None, | ||
| ecg_overall_finding: Optional[Union[Code, CodedConcept]] = None | ||
| ) -> None: |
There was a problem hiding this comment.
Prevent construction if both parameters are None?
|
Hi @CPBridge, thank you so much for your in depth review! I'll work on your improvement ideas over the next couple of weeks, use your suggestions where I agree and respond to the topics open for discussion whenever I find the time. Sorry for all the inconsistencies, multiple people contributed on our end but I am confident we can bring these classes up to your standard. |
Co-authored-by: Chris Bridge <[email protected]>
Co-authored-by: Chris Bridge <[email protected]>
Co-authored-by: Chris Bridge <[email protected]>
Co-authored-by: Chris Bridge <[email protected]>
2 participants
Implements #355
This pull request restructures the src/highdicom/sr/templates.py file into a
src/highdicom/sr/templatesfolder.Additionally it adds support for the DICOM templates
as well as all templates these three depend on.