ImagingDataCommons / highdicom

High-level DICOM abstractions for the Python programming language
https://highdicom.readthedocs.io
MIT License
168 stars 35 forks source link

Changes HAS_PROPERTIES to HAS_CONCEPT_MOD for validator error messages #198

Closed seandoyle closed 2 years ago

seandoyle commented 2 years ago

SRs created using the highdicom example program generate errors in the pixelmed DicomSRValidator.sh program:

./DicomSRValidator.sh ~/Development/github/merge/highdicom/highdicom_doc_sr_original.dcm 
Found Comprehensive3DSR IOD
Found Root Template TID_1500 (MeasurementReport)
Error: Template 300 Measurement/[Row 1] NUM */[Row 2] CODE *: 1.7.1.5.2: /CONTAINER (126000,DCM,"Imaging Measurement Report")/CONTAINER (126010,DCM,"Imaging Measurements")/CONTAINER (125007,DCM,"Measurement Group")/NUM (131184002,SCT,"Area of defined region")/CODE (121402,DCM,"Normality"): Incorrect relationship - expected HAS CONCEPT MOD - found HAS PROPERTIES
Error: Template 300 Measurement/[Row 1] NUM */[Row 2] CODE *: 1.7.1.5.3: /CONTAINER (126000,DCM,"Imaging Measurement Report")/CONTAINER (126010,DCM,"Imaging Measurements")/CONTAINER (125007,DCM,"Measurement Group")/NUM (131184002,SCT,"Area of defined region")/CODE (121403,DCM,"Level of Significance"): Incorrect relationship - expected HAS CONCEPT MOD - found HAS PROPERTIES
Root Template Validation Complete
IOD validation complete

This PR contains small changes that removes these errors by changing the relationship type from HAS_PROPERTIES to HAS_CONCEPT_MOD in two places.

 ./DicomSRValidator.sh ~/Development/github/merge/highdicom/highdicom_doc_sr.dcm 
Found Comprehensive3DSR IOD
Found Root Template TID_1500 (MeasurementReport)
Root Template Validation Complete
IOD validation complete

(both SRs are attached for inspection).

However - it is not clear which is correct. TID 300 . The highdicom code appears to assume that the code

properties=hd.sr.MeasurementProperties(
            normality=hd.sr.CodedConcept(
                value="17621005",
                meaning="Normal",
                scheme_designator="SCT"
            ),
            level_of_significance=codes.SCT.NotSignificant
        )

is related to row 8 of the table for TID 300 which has the relationship HAS_PROPERTIES. The validator seems to assume that these properties are in ROW 2 and have the relationship HAS_CONCEPT_MOD. Which is correct?

sample_SRs.zip

hackermd commented 2 years ago

Thanks for reporting the issue @seandoyle. Since MeasurementProperties implements TID 310 "Measurement Properties", I tend argue that HAS_PROPERTIES is correct. @dclunie what are your thoughts on this?

CPBridge commented 2 years ago

Since MeasurementProperties implements TID 310 "Measurement Properties", I tend argue that HAS_PROPERTIES is correct.

My reading of the template definitions suggests this also

dclunie commented 2 years ago

For the TID 310 content items, the relationship should definitely be HAS PROPERTIES.

The validator struggles with top-down parsing ambiguities for this sort of thing, since TID 300 Row 2 is so general wrt. the concept name code sequence, in the absence of other specific constraints.

Send me a sample of your output with the HAS PROPERTIES relationship that the validator is complaining about, and I will see if I can make it do better.

hackermd commented 2 years ago

Send me a sample of your output with the HAS PROPERTIES relationship that the validator is complaining about, and I will see if I can make it do better.

Thank you @dclunie. @seandoyle has attached a ZIP archive with samples (see above): https://github.com/herrmannlab/highdicom/files/9390328/sample_SRs.zip

seandoyle commented 2 years ago

@dclunie - yes- the zip file is above with the SRs - the highdicom_doc_sr_original.dcm file contains the HAS_PROPERTIES which generates the errors from the validator. Please let me know if you need more information or want me to email this to you directly.

dclunie commented 2 years ago

Got them, thanks. I can confirm that they trigger the defect in the validator and will work on it.

BTW. There are other issues related to the basic encoding reported by dciodvfy, but these are presumably related to errors inherited from the referenced images.

hackermd commented 2 years ago

Thanks @dclunie for looking into the errors. I will close this PR. Please feel free to create a separate issue should any of the other errors turn out to be a bug in the library.