[Feature]: Save additional metrics to SR report for all annotations #460
Conversation
✅ 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? |
|
@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 |
|
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 |
|
@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 |
src/utilities/TID300/Circle.js
Outdated
| ]; | ||
| if (radius) { | ||
| measurements.push({ | ||
| RelationshipType: "CONTAINS", |
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.
src/utilities/TID300/Ellipse.js
Outdated
| ]; | ||
|
|
||
| if (max) { | ||
| measurements.push({ |
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.
src/utilities/TID300/Point.js
Outdated
| use3DSpatialCoordinates | ||
| }); | ||
|
|
||
| const graphicContentSequence = buildContentSequence({ |
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.
src/utilities/TID300/Polyline.js
Outdated
| ]); | ||
| ]; | ||
|
|
||
| if (max) { |
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.
| * Builds a DICOM SR ContentSequence block for geometric measurements | ||
| * that share the same structure across tools (Circle, Ellipse, Polyline, etc.) | ||
| */ | ||
| export default function buildContentSequence({ |
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.
| if (!codingUnit) { | ||
| log.error("Unspecified units", units); | ||
| return MM_UNIT; | ||
| return generateUnitMap(units); |
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
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
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?
|
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? |
|
@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. |
| CodingSchemeDesignator: "UCUM", | ||
| CodingSchemeVersion: "1.4", | ||
| CodeMeaning: "px" | ||
| }; |
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") |
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. |
|
@wayfarer3130 As we are aiming to include this MR in the upcoming OHIF 3.12 release, we would appreciate a review of the remaining open items, the proposed implementation approaches, and any clarifications needed on the points outlined below. 1. Replace non-standard or incorrect units with proper UCUM unitsRequested change Replace usages such as Status Planned. Some additional unit support is already introduced in this PR; remaining normalization can be completed. 2. Normalize UCUM CodeMeaning valuesRequested change Shorten the verbose CodeMeaning values Status Partially implemented Clarification needed Should we consistently prefer UCUM canonical form ( 3. Dimensionless measurementsRequested change Use Current behavior Measurements without units are currently treated as Clarification / Dependency This behavior currently originates from the CS3D layer.
4. SCT (SNOMED CT) code adoptionRequested change Replace SRT codes with SCT (SNOMED CT) codes e.g. Status Partially implemented 5. Avoid custom placeholder codes (99dcmjs)Requested change Avoid codes such as Use CID 100 or modality-appropriate standard codes, e.g. Status Planned 6. Appearance related data in SR contentRequested change Do not include appearance or UI-related elements like Related requirement For all SCOORD items, GraphicType is mandatory and must be populated. Status GraphicType handling appears fixed in generated SR, we will revisit and confirm this; Clarification and planning Does this break compatibility with already created SR reports and how end users uses the data? 7. Correct use of R-INFERRED FROM and content item referencingAlthough TID 1501/TID 320 R-INFERRED FROM is allowed, it must reference the correct hierarchically indexed shared graphic item content, using The current Status Planned.
Any changes on SR DICOM structure after dcmjs DICOM generation will break the reference numbers. Warn in documentation that users must not modify generated SR data on-before or on-after SR data generation or save. Reference correctness is under analysis. This may need an index tracker to ensure correct hierarchy levels are tracked and used for referencing data. 8. TID 1410 adoption and other compatibilityRequested change There is a strong recommendation to adopt TID 1410 (multiple measurements under a single ROI) in place of the current TID 1501 / TID 320–based structure. In addition, the generated SR structure should be aligned with established references and best practices, including
Impact This change represents a significant structural refactor and would require coordinated updates across multiple codebases, including
Additionally, a clear strategy for backward compatibility with existing stored SR measurements would need to be defined and implemented. Clarification and planning Given the scope and cross-repository impact of this change, and they may not be a blocker for this PR, we would like to request clarification on preferred approach. Should this be planned and executed as a separate, dedicated refactoring effort, or is it preferable to address this in a single coordinated change, to avoid multiple SR format transitions and potential compatibility issues? Thanks! |
|
@sen-trenser The tools you mention are not sufficient to detect failure to comply with the IOD constraints on value and relationship types, nor failure to comply with the template structure. You need to use is DicomSRValidator from PixelMed or similar for that. If you do not either (a) correct or (b) explain away the output of DicomSRValidator, then it is very likely that your SR objects are invalid.
|
|
Can whether a measurement is a linear distance, area, volume, or something else, not be determined from the measurement itself and not the units, i.e., from the ConceptNameCodeSequence of the NUM that is the measurement? You are using codes for Area, Perimeter, etc. for that, right?
|
|
We will verify with the Pixelmed DicomSRValidator as well. Thank you for creating these applications.
Yes. We can use the SCT code in ConceptNameCodeSequence for differentiating perimeter, area etc. Currently, the SR records AREA, Perimeter in SRT also. We should be converting those to SCT. However, we should look into backward compatibility with previous OHIF generated SR data as well.
Thank you! |
Do you mean that the remaining normalization can be completed in the CS3D codebase, or is there more work to be done in the dcmjs codebase?
Can this be completed as far as the dcmjs codebase is concerned?
Prefer mm2 UCUM canonical form please.
As suggested elsewhere, please include the no units in order to be compliant with the spec, and then determine what it means based on the fact that it is area/volume/max/min etc.
CS3D should be responsible for unit selection, and dcmjs should provide all the constants/mapping for the unit codes so that CS3D can easily just select the appropriate code and it could be fixed/updated in the future for specific text value etc.
Although a nice to have, this is a deprecated value that is permitted. It would be nice to update the remaining codes to SCT codes, but I don't think that should be required to accept the PR.
Lets get this one updated. Just use Imaging Procedure on anything unknown should be sufficient. Please ensure there is a constant exported for this so that CS3D can just use the exported constant.
I KNOW this isn't in keeping with the recommendation for semantic meaning only, but storing this in this way in useful for a lot of users. Make it work for real users trumps intended use of some of the data that is still compliant with the actual standard. Please keep this in. We should update the storage on the CS3D side to only include this when the position has been updated.
PLEASE do not remove appearance only items
Actually, the parsing side of things completely ignores the R-INFERRED FROM information, so as long as it is correct for the SR verify tool when created, I don't think you need to be too careful about it.
Please do this as a planned approach as a separate PR. I'd love to have it in this one, it just doesn't make sense to update this further when there are already existing creators out there doing it the way you have currently updated it to.
|
|
@wayfarer3130 Thank you for the quick reply.
The planned items are to be done in this dcmjs PR.
Yes
Okay. Thanks for confirming.
We will update it.
Okay. We will add the currently used mappings in dcmjs. Although not planned in the scope of this PR, as there are lots of SCT codes defined, and CS3D annotations can be extended, wouldn't it be a good idea for later to allow DCMJS consumers to register TID converters which can provide their own standard SCT
Okay. We will check if we can modify this without breaking SR backward compatibility.
Okay. We may need to recheck if validators complains about any other fields.
Okay. We will keep it as is. We may need to check if other validation tools will complain about these or not.
Eventhough OHIF may not be parsing this, we are not yet sure if this needs to be updated properly for other applications like slicer to properly read the data. We will start with correcting for the SR verify tool when addressing this.
Thanks! We shall split this into a feature request in dcmjs. tl;dr; We can skip items 4,6 and 8 from the above list as they are not required for merging the PR. Other review comments will be attempted in this PR itself. |
You are contradicting yourself here. If you measure area or perimeter, it won't be dimensionless - I don't think you can do "no units" for those. Are you suggesting that you would like to measure area in the units of pixels squared, and perimeter in pixels, instead of mm2? Is there a use case for such a measurement? |
px are a simple count of number of pixels and thus in UCUM are "no units". Those are the linear dimensions for measurements that aren't associated with any calibration. An area of an image that isn't calibrated is still a count of pixel^2, so is still dimensionless. |
I'll just chime in that I agree with this. |
I have to say that I also agree with the decision to keep those codes. Those are private codes, so it is up to the application to use them as needed. This approach is not ideal, but there are no good alternatives: adding support for presentation states to communicate this would probably be a huge effort risking alienating existing users and would be even more difficult to interoperate, deviating from the standard template or adding this content to private tags would be much worse in my view. While I understand combining SR with PR for this use case is ideal, I have yet to see any actual open-source implementations and examples that demonstrate how to do it right. So they would need to be trail-blazers most likely. Every time we try to implement something from the standard for the first time (meaning, when there are no open source tools and no sample datasets demonstrating how it can be done), it always requires dedicated effort and expertise to interpret the standard, go through CPs to fix issues, and major implementation effort. Trying to be that ambitious without having the appropriate resources risks killing the entire initiative altogether. |
| const measurements = [ | ||
| ...measurementConfigs | ||
| .filter(config => config.value !== undefined) | ||
| .map((config, index) => |
There was a problem hiding this comment.
Doesn't the .map already produce an array - so you don't need [ ...measurementConfigs ?
|
@abhijith-trenser - could you attach an updated SR with two measurements, Elliptical ROI and Planar Freeform (closed), without moving the text box annotation. I'd like to see how all the updates go together to create a new SR and run the Dicom SR Validator that was suggested to look at some of the internals. Would like to get this in quite soon if at all possible so we can include in OHIF 3.12 |
We are working on correcting the use of R-INFERRED FROM and content item referencing. We attempted a basic implementation based on the values suggested in your comments; however, the generated DICOM files still produce the same errors when validated with the PixelMed SR validator. @dclunie The generated SR report with updated value is attached. |
@wayfarer3130 Please find the required SR reports attached. cc: @sen-trenser |
|
I have a set of changes, NOT including the reference item - that one won't work at all unfortunately as the structure is wrong. The parsing is going to have to change a bit to fix things, but I think it will be ok with that. The changes are:
Find the EllipticalROI Measurement Group’s 0040A730.Value array (it starts with Tracking Identifier + Tracking UID, then Area, etc.). Insert this new item right after the Tracking UID item (or anywhere before the NUMs). 2a) Remove the SCOORD that’s currently under Area 2b) Remove the by-reference “INFERRED FROM” blocks under the stats NUMs
|
wayfarer3130
left a comment
wayfarer3130
left a comment
There was a problem hiding this comment.
We still need to get it passing the DICOM SR Validator - unfortunately, I was mistaken in what the inferred from block means - it is what OTHER measurements this measurement is inferred from, not what other graphic data it is inferred from.
I've attached a valid sample that passes the DICOM SR Validator checks and avoids duplicating the data. I think this is in fact the 1410 structure.
|
@wayfarer3130 Thank you for the detailed review and for raising the concern regarding TID 1410 compatibility. We appreciate the time and attention you have given to this pull request. After careful consideration with our team and PO, we would like to clarify that supporting TID 1410 compatibility and backward compatibility would require additional changes and significant effort outside of the intended scope of this contribution in this and other PRs. This pull request is planned for inclusion in the upcoming 3.12 release and is currently blocking two dependent changes (CS3D PR 2430 and OHIF PR 5600) that are also targeted for the same release. To avoid further delays and to keep this change focused and scoped appropriately, we would like to ask whether it would be acceptable to proceed without incorporating TID 1410 compatibility. Our goal is to keep this PR focused on the current functionality and get aligned with the 3.12 release timeline. We appreciate your guidance on whether proceeding on this basis would be acceptable. Thank you again for your review and for maintaining the project available for everyone! |
Unfortunately there isn't a good backwards/forwards compatible option for this that doesn't leave us with a lot of invalid DICOM. I understand it adds scope, but not adding scope means forever maintaining future scope for backwards compatibility for multiple attributes, as well as not passing connectathons and things like that on the stored data. |
8 participants
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