[Feature]: Save additional metrics to SR report for all annotations #460
+632
−286
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for dcmjs2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@sedghi Could you please review this or suggest anyone that can help with approving the PR in dcmjs? |
Contributor
|
@sen-trenser - could you:
|
|
@wayfarer3130 We found some errors when run dciodvfy on the example dcm file. Currently resolving these errors. |
|
@wayfarer3130
|
|
@wayfarer3130 We had some modifications to fix errors reported by the dciodvfy tool. Could you please check the attached data @nithin-trenser provided above? Please check the area marked with text |
Contributor
|
There is the concept of INFERRED FROM allowing references to a common layout to be used - that basically looks like below: (0040,a730) SQ (Sequence with undefined length #=3) # u/l, 1 ContentSequence (fffe,e000) na (Item with undefined length #=6) # u/l, 1 Item 2: NUM Measurement #1 (fffe,e000) na (Item with undefined length #=6) # u/l, 1 Item 3: NUM Measurement #2 |
Contributor
|
@pieper - for the saving of SR annotations, I'm proposing that second and subsequent measurements use references to the annotation definition in the first measurement value - that is, a structure like: measurement value 1: The TID 1500 canonical way I believe is: As I read part 16, I THINK either way is acceptable, but I'm not 100% sure. The point in either one is to avoid duplicating the image/graphic data references so that there is no chance that they are different. That is only done when they actually ARE the same - things like bidirectional are not the same annotations first/second. Might ask David what he thinks about this? I'd very much prefer not to go down something not standards compliant. |
|
@wayfarer3130 |
wayfarer3130
reviewed
Dec 1, 2025
src/utilities/TID300/Circle.js
Outdated
| ]; | ||
| if (radius) { | ||
| measurements.push({ | ||
| RelationshipType: "CONTAINS", |
Contributor
There was a problem hiding this comment.
I'm ok with this as it is, but it would be nice to have simple helper classes that just constructed this object via arguments, eg:
measurements.push(new RadiusReference(radius, annotationIndex))
Or maybe RadiusMeasuredValue.
wayfarer3130
reviewed
Dec 1, 2025
src/utilities/TID300/Ellipse.js
Outdated
| ]; | ||
|
|
||
| if (max) { | ||
| measurements.push({ |
Contributor
There was a problem hiding this comment.
Again, all of these would become simpler if you just reference a structure class like MaximumValueReference(max, modalityUnit, annotationIndex)
There was a problem hiding this comment.
Created helper function for this.
wayfarer3130
reviewed
Dec 1, 2025
src/utilities/TID300/Point.js
Outdated
| use3DSpatialCoordinates | ||
| }); | ||
|
|
||
| const graphicContentSequence = buildContentSequence({ |
Contributor
There was a problem hiding this comment.
I definitely find the new version easier to read
There was a problem hiding this comment.
Defined new class and used it here.
wayfarer3130
reviewed
Dec 1, 2025
src/utilities/TID300/Polyline.js
Outdated
| ]); | ||
| ]; | ||
|
|
||
| if (max) { |
Contributor
There was a problem hiding this comment.
You might even extract the full set of reference values here as a standard extractor for "area" measurements rather than including it in each measurement type. Again, not required, just suggesting ways to clean things up a bit more.
There was a problem hiding this comment.
Created helper function for this.
wayfarer3130
reviewed
Dec 1, 2025
| * Builds a DICOM SR ContentSequence block for geometric measurements | ||
| * that share the same structure across tools (Circle, Ellipse, Polyline, etc.) | ||
| */ | ||
| export default function buildContentSequence({ |
Contributor
There was a problem hiding this comment.
@pieper - is there a better name for this that says exactly what this does? content sequence is a bit too generic.
Also, the general dcmjs pattern for this is as a constructor.
How about:
class Tid320ContentItem
constructor({
graphicType,
graphicData,
use3DSpatialCoordinates = false,
referencedSOPSequence,
referencedFrameOfReferenceUID}
and then assign to this.
I believe that meets the pattern.
There was a problem hiding this comment.
Done as suggested. Created a class Tid320ContentItem for create content sequence.
wayfarer3130
reviewed
Dec 1, 2025
| if (!codingUnit) { | ||
| log.error("Unspecified units", units); | ||
| return MM_UNIT; | ||
| return generateUnitMap(units); |
Contributor
There was a problem hiding this comment.
Much as I like this change, it isnt actually valid and can result in ill defined units.
You can expand the list of UCUM units that are available, but not create a new one arbitrarily. If you want to do it this way, then use coding scheme designator "99dcmjsUnit and omit the scheme version, or set it to the version this got added in.
There was a problem hiding this comment.
I’ve updated the implementation to include support for all DICOM-defined UCUM-compatible measurement units.
DICOM explicitly states that:
DICOM has now adopted the Unified Codes for Units of Measurement (UCUM) standard for all units of measurement.
This Coding Scheme allows for the construction of pre-coordinated codes from atomic components.
To ensure correctness, the supported units now come from the official DICOM
context groups that define UCUM and UCUM-compatible extensions:
- CID 83, 84, 85 – Units of Measurement
https://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_83.html
https://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_84.html
https://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_85.html
cc: @sen-trenser
There was a problem hiding this comment.
In case you want to check the validity of any UCUM code values that you generate (or adopt from the DICOM standard or someone else's examples), you may find my UCUMAnalyzer helpful, available at this site.
Note that the unistofmeasure.org site referenced therein seems to be dead (archived at https://web.archive.org/web/20190310234558/http://unitsofmeasure.org/trac), and ucum.org seems to be the new site.
E.g.:
% java -cp ./pixelmed_ucum.jar:./antlr-2.7.5.jar com.pixelmed.ucum.UCUMAnalyzer "mm2"
Canonical form = ( / ( . 1.0E-6 m m ) ( . null ) )
% java -cp ./pixelmed_ucum.jar:./antlr-2.7.5.jar com.pixelmed.ucum.UCUMAnalyzer "[hnsf'U]"
Canonical form = ( / ( . 1.0 [hnsf'U] ) ( . null ) )
There are other UCUM tools available than mine, but I don''t have a current list.
wayfarer3130
requested changes
Dec 1, 2025
Contributor
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
There are two nice to haves to extract the functionality for creating TID320 content items and sets of content items, and one must change to avoid creating invalid UCUM units.
wayfarer3130
approved these changes
Dec 2, 2025
Contributor
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
@pieper - I'm good with this PR now. Are you ok with me merging it?
Collaborator
|
There's a test failing. CoPilot says it's due to unicode for mm2, can you look into making the tests pass? In general I'm good with this but haven't looked at the details. I would like to run the resulting files past @dclunie. Are you able to create some examples using the latest version of this code? Is it possible to run something like viewer.ohif.org/local using the PR branches and then share the files so they can be dropped onto the update ohif? |
|
@pieper |
Collaborator
|
Thanks for providing the examples, @abhijith-trenser , this helps a lot. I tried this with Slicer 5.10.0 and the latest Quantitative Reporting extension. It's not surprising that it's not working since I doubt this template has been seen before. It's not a blocker for this PR to resolve the loading in Slicer, but it would be nice to have it working for interoperability and demonstration purposes. I'll go over it with @fedorov and others to see if this is an easy fix. Also, I'm seeing some |
sedghi
reviewed
Dec 3, 2025
src/utilities/TID300/Point.js
Outdated
| @@ -1,4 +1,5 @@ | |||
| import TID300Measurement from "./TID300Measurement.js"; | |||
| import Tid320ContentItem from "./Tid320ContentItem.js"; | |||
Collaborator
There was a problem hiding this comment.
Why are we naming this differently from TID300? Let's aim for consistency.
There was a problem hiding this comment.
Naming changed to TID320ContentItem to keep consistency.
sedghi
reviewed
Dec 3, 2025
| stdDev, | ||
| modalityUnit, | ||
| ReferencedFrameOfReferenceUID, | ||
| annotationIndex |
Collaborator
There was a problem hiding this comment.
what is annotationIndex, seems like it is mandatory
There was a problem hiding this comment.
annotationIndex is used in the ReferencedContentItemIdentifier tag to specify which annotation the item is referencing.
sedghi
reviewed
Dec 3, 2025
| CodingSchemeDesignator: "UCUM", | ||
| CodingSchemeVersion: "1.4", | ||
| CodeValue: "mm2", | ||
| CodeMeaning: "SquareMilliMeter" |
Collaborator
There was a problem hiding this comment.
why SquareMilliMeter, since above you have millimeter
Collaborator
There was a problem hiding this comment.
also below i see there is space in code Meaning (e.g., proportinonal to counts), why not square millimeter
There was a problem hiding this comment.
We kept “SquareMilliMeter” because it was already being used in the existing implementation.
For the other CodeMeanings, we followed the terminology exactly as defined in the DICOM documentation:
CID 83 - Units
CID 84 - Area Units
CID 85 - Volume Units
References:
https://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_83.html
https://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_84.html
https://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_85.html
Collaborator
There was a problem hiding this comment.
non of those links has SquareMilliMeter, are you sure you linked correct references? I see in the second link mention of millimeter not milliMeter
There was a problem hiding this comment.
“SquareMilliMeter” was already being used in the existing implementation.
Link: https://github.com/dcmjs-org/dcmjs/blob/master/src/utilities/TID300/unit2CodingValue.js#L14
But this is removed now and used mm2.
sedghi
reviewed
Dec 3, 2025
| CodingSchemeDesignator: "UCUM", | ||
| CodingSchemeVersion: "1.4", | ||
| CodeValue: "cm2", | ||
| CodeMeaning: "Centimeter**2" |
There was a problem hiding this comment.
sedghi
reviewed
Dec 3, 2025
| { | ||
| CodingSchemeDesignator: "UCUM", | ||
| CodingSchemeVersion: "1.4", | ||
| CodeValue: "[hnsf'U]", |
Collaborator
There was a problem hiding this comment.
is this some sort of dicom rule to have [] or {}, sometime i see only string
There was a problem hiding this comment.
No - see the UCUM specification of square brackets and curly braces, but some aspects of the DICOM usage are discussed in PS3.16.
sedghi
reviewed
Dec 3, 2025
src/utilities/MeasurementBuilder.js
Outdated
| "Area", | ||
| area, | ||
| areaUnit, | ||
| annotationIndex |
Collaborator
There was a problem hiding this comment.
please provide documentation on what is annotationIndex
|
I took a look at the SR in 3.000000-AXIAL.ST.5.0.X.5.0-41783.zip and see a number of issues:
the Anyway, that's probably not everything, but hopefully it is helpful. David |
|
BTW. when I referred to "today's build" of the PixelMed SR validator, I meant what you can find in the current directory, which is dated The tool I am using (in addition to dciodvfy) is For documentation, see com.pixelmed.validate.DicomSRValidator E.g., for the example in question, it reports (with the whereas dciodvfy reports: Also possibly helpful, is And finally in pixelmed.jar is rather than one that is just DICOM data element based, like DicomAttributeBrowser.sh ( |
Contributor
|
@dclunie - do you have an example of the TID 1410 for an ROI? That seems like the right choice for area measurements. @abhijith-trenser - sorry about the units, I should have looked at that more closely. Probably we should ensure that all units we provide are UCUM units and just make sure the list we have is definitive. I'm not sure what happens for colour RGB value measurements @dclunie - what UCUM unit is used for RGB probe values? |
|
@wayfarer3130 - You can find a bunch of TID 1500 based SRs in IDC, e.g., search on modality SR AND CT or similar. One example with a single ROI with lots of derived measurements (though not, as it happens, area, and with the ROI defined by a referenced SEG object rather than SCOORDs) is the SR in Study It uses TID 1411 rather than TID 1410 since it is referring to a volume rather than a planar region, but you get the idea. Both have Row 5 versus Row 7 alternatives for SCOORD or IMAGE (SEG) specification of the region. You may recall that @fedorov et al worked on this sort of think in QIICR, but unfortunately the QIN-HEADNECK collection is no longer publicly available since TCIA's masters became concerned about faces, then their masters about controlled data access policy. |
|
Short version: (1,UCUM,"no units") if nothing more specific (like ([hnsf'U], UCUM, "Hounsfield unit")) known. TL;DNR: @wayfarer3130 - wrt. "Gy is the UCUM value that CS3D uses 'px' for" - "Grey" (UCUM 'Gy') is a unit of ionizing radiation dose, and has nothing to do with "greyscale" "intensity" interpretation of a pixel value. I gather from "what UCUM unit is used for RGB probe values" that you are looking for units for the pixel intensity in different settings, but don't confuse "units" with the "quantity" - e.g., see the discussion in the context of real-world value mapping in PS3.3. Compare CID 7180 Abstract Multi-dimensional Image Model Component Semantic with CID 7181 Abstract Multi-dimensional Image Model Component Unit. E.g., the measurement at a single point might be (112031,DCM,"Attenuation Coefficient") or (110852,DCM,"MR signal intensity"), or for a non-point region, the mean or similar thereof (perhaps described as derivation), but the units would then be ([hnsf'U], UCUM, "Hounsfield unit") or dimensionless if unspecified or unknown (e.g., by using (1,UCUM,"no units"), or ({0:255}, UCUM, "range: 0:255") if you wanted to specify the dimensionless range, e.g., derived from BitsStored). Note that for the physical quantity (not the unit), DICOM does define in CID 7180 Abstract Multi-dimensional Image Model Component Semantic codes for the R, G and B component of an image; but these were not intended to be used as units. I understand that it can be difficult to determine what to encoded as a finding, a measurement, a method, a derivation and what to specify as units, and how this relates to the concept of a quantity, and very often the concept is left to be implicit (and or bound into some pre-coordinated concept for the measurement rather than being post-coordinated). E.g., in the case of the radiomics example I referred you to earlier, we have: I.e., the fact that all of these measurements are measurements of attenuation coefficient is never explicitly described. BTW., In case you are not aware of it, the IBSI work is a great reference, and we use many of their codes in DICOM PS3.16 and recognize them as a coding scheme; see the manual and the Radiology paper and their github repo. They are grayscale MR focussed but most of their concepts are generalizable. |
|
Wrt. code meanings for UCUM measurements, PS3.16 is very inconsistent wrt. various different patterns for the "code meaning" (what HL7 calls the "display name"); in the past we have not obsessed about this since it is the code value that matters from precision of communication, and machine recognition perspectives. However, there are some folks who want to rationalize this, and to quote from a recent proposed new DICOM CP: Though the current text about what can be used in code meaning is fairly flexible:
it is possible this will get modified (e.g., to recommend singular rather than plural forms, or to more strongly emphasize the use of the UCUM "print" value when it is encodable in the default character set). It is also possible that explicit use of different code meanings in different contxet groups will change (e.g., to use one pattern uniformly in all context groups, despite the possibly variants that are recognized). The general rules for what to include in code meaning remain applicable though: Code Meaning (0008,0104) is a purely annotative, descriptive Attribute ... This does not imply that Code Meaning (0008,0104) can be filled with arbitrary free text. Available values from the Coding Scheme or translation in the chosen language shall be used. |
fedorov
reviewed
Dec 4, 2025
| CodingSchemeDesignator: "UCUM", | ||
| CodingSchemeVersion: "1.4", | ||
| CodeMeaning: "px" | ||
| }; |
Collaborator
There was a problem hiding this comment.
Looks like this one was missed - @dclunie commented that it should be fixed I believe.
@wayfarer3130 What would be the recommended way to represent pixels count? Measurement Units Code Sequence (0040,08EA) = (130922,DCM,"Number of pixels") |
Contributor
According to the earlier bit of the post from someone who wrote a good bit of the DICOM standard, the correct answer is:
If the attribute being measured is clearly in the context of a pixel/area measurement or probe, then no units is clearly equivalent to px as that is actually what we are trying to convey with px that there isn't a unit with it. I'd prefer something more exact but there doeesn't seem to be anything. |
This depends a lot on whether the count is relative to an unspecified area or volume (and therefore multiples of unity), or counts per a defined measure of space (e.g., count/mm2). So flavors of unity will often work, even some variant of the thing being counted ({pixels}), but sometimes you want to be more specific, e.g., see CID 4290, which is one of the few examples of this in PS3.16, which actually are specific about what they are counting to (cells). PS. We are not clear yet on whether the thing in curly braces should be singular or plural (e.g., '{pixel}' versus '{pixels}'), or if either is OK. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
Changes & Results
What are the effects of this change?
The following tools in Cornerstone3D, Length, Bidirectional, Ellipse, Circle, Rectangle, and Freehand now include all annotation metrics without requiring rendering.
This feature applies only to newly created SRs. Existing SR files will not reflect these changes unless re-saved.
Testing
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment