Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sr update aim #197

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

Sr update aim #197

wants to merge 19 commits into from

Conversation

emelalkim
Copy link
Contributor

I have been working on AIM to DICOM SR conversion.
I am using the Cornerstone adapters because we are using Cornerstone and Cornerstone tools for our frontend
I have found some missing features and started adding them
We worked with David Clunie on AIM DICOM-SR harmonization efforts and I am using that as a reference

Currently I have added

  • comment
  • procedure reported from modality
  • patient info
  • qualitative evaluations

I still have more to implement but I wanted to put this here and ask your opinion/comments
Appreciate any input
Also Steve said Igor is taking over DICOM SR, what is his github handle? I'd like to ask his opinion too

@emelalkim
Copy link
Contributor Author

I also have a question I am not sure who is the best person to ask @JamesAPetts, is that you?
So each AIM has a name and Davis Clunie added as the tracking identifier
You are using auto generated tracking identifiers, and I saw some checks about valid cornerstore tracking identifier
Is it ok if I edit the tracking identifier from the options (like I did trackingUniqueIdentifier)?

@pieper
Copy link
Collaborator

pieper commented Apr 7, 2021

This is very timely and we should put our heads together on this. There has been a lot of SR activity also in python for highdicom and also for pathology annotations and AI results, so we should be sure to develop good conventions for interoperability and consistency. @hackermd @CPBridge @seandoyle @fedorov @dclunie maybe we should have a larger group discussion meeting?

@igoroctaviano can you also help review this?

@fedorov
Copy link
Collaborator

fedorov commented Apr 7, 2021

@emelalkim could you share some representative image series with the accompanying SRs that demonstrate the capabilities of your implementation?

@emelalkim
Copy link
Contributor Author

@fedorov I put the sample data set I am using and the draft DICOM SR here. I also put some screenshots. I am going to add some tests after I get a more mature conversion. (FYI, we are working on adding radelements so the AIM file has some made up entities for the codes, please ignore them)
@pieper I am certainly open to a larger or smaller group meeting. And looking forward to comments from @igoroctaviano and @JamesAPetts

@dclunie
Copy link

dclunie commented Apr 10, 2021

@emelalkim I took a look at your sample, which looks pretty good.

Wrt. TID 1500 compliance:

Content item 1.5.1.4 CONTAINS: CODE: (G-C0E3,SRT,"Finding Site") = (RID58,RadLex,"liver")

should have a "HAS CONCEPT MOD" relationship with its parent (125007,DCM,"Measurement Group"), not "CONTAINS" http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_A.html#para_67e2ac2d-62c4-47d1-851c-975e33b49fed

You are identifying the time point in a non-standard place with a non-standard code:

Wrt. codes:

If you are going to use RADLEX or RADELEMENT, please use the standard all-capitalized coding scheme designators, rather than the lower case ones used now:

dciodvfy reports:

Warning - Unrecognized defined term for value 1 of attribute
Warning - Unrecognized defined term for value 1 of attribute

See http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_8.html#para_f568450e-c147-41e3-ba6d-057ffd233e89

Also, it would be better to use SCT rather than SRT codes, since the former are deprecated.

E.g., instead of (G-A185,SRT,"Long Axis"), use (103339001,SCT,"Long Axis")

which is why dciodvfy reports:

Warning - Coding Scheme Designator is deprecated - attribute =

See http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_8.html#sect_8.1

You will probably not be convinced, but in DICOM we prefer to use SCT codes for anatomy, rather than RadLex, i.e., (10200004, SCT, "Liver") rather than (RID58,RADLEX,"liver")

For the finding, you might consider (108369006, SCT, "Tumor") instead of (RECIST_v2,99EPAD,"Tumor assessment").

For specifying it is a target lesion, you might consider (112016, DCM, "Baseline Category") = (112074, DCM, "Target Lesion at Baseline") (see the pattern at http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_TID_4102.html#para_929b7c79-ee18-49b2-9eb1-7b7452354fac]

For enhancing lesions, DICOM SEGs use (C113842, NCIt, "Enhancing Lesion") rather than (RID6055,RADLEX,"Enhancing") http://ncit.nci.nih.gov/ncitbrowser/ConceptReport.jsp?dictionary=NCI_Thesaurus&code=C113842

For the lesion "status", for the presence or absence of things, DICOM SR templates usually use the generic SNOMED CT codes, e.g., (52101004, SCT, "Present"); see http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_241.html

@emelalkim
Copy link
Contributor Author

Thank you very much @dclunie! This is very helpful. I appreciate you taking the time to go through it.
I have put some comments below and have some questions.

@emelalkim I took a look at your sample, which looks pretty good.

Wrt. TID 1500 compliance:

Content item 1.5.1.4 CONTAINS: CODE: (G-C0E3,SRT,"Finding Site") = (RID58,RadLex,"liver")

should have a "HAS CONCEPT MOD" relationship with its parent (125007,DCM,"Measurement Group"), not "CONTAINS" http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_A.html#para_67e2ac2d-62c4-47d1-851c-975e33b49fed

This is coming from dcmjs I think, I will correct it.

You are identifying the time point in a non-standard place with a non-standard code:

OK I'll move it there. Actually we were thinking of moving all the qualitative evaluations under Measurement Group as those are also about the ROI, in a similar manner to quantitative values. What do you think about that?
Small question, is the "Subject Time Point Identifier" right term to put? I would think something like a Study Time Point would fit better. Is there a term like that?

Good to know I'll use that if I need to put something without units later

  • there is no need to send an Image Library entry for the image if you are not going to say anything about it (provide any content items that describe it)

I think we have the library because the measurements refer to the images. no? I think we had this with the xslt transformation too. Am I wrong?

Wrt. codes:

If you are going to use RADLEX or RADELEMENT, please use the standard all-capitalized coding scheme designators, rather than the lower case ones used now:

dciodvfy reports:

Warning - Unrecognized defined term for value 1 of attribute
Warning - Unrecognized defined term for value 1 of attribute

See http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_8.html#para_f568450e-c147-41e3-ba6d-057ffd233e89

Will do

Also, it would be better to use SCT rather than SRT codes, since the former are deprecated.

E.g., instead of (G-A185,SRT,"Long Axis"), use (103339001,SCT,"Long Axis")

which is why dciodvfy reports:

Warning - Coding Scheme Designator is deprecated - attribute =

See http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_8.html#sect_8.1

Makes sense. we also have them in AIM but these are coming from dcmjs. @pieper is it ok for me to change them? I'll fix the AIM ones

You will probably not be convinced, but in DICOM we prefer to use SCT codes for anatomy, rather than RadLex, i.e., (10200004, SCT, "Liver") rather than (RID58,RADLEX,"liver")

These are coming from the RECIST template we have in ePAD. I'll talk with Daniel on how we can change it, for old AIMs especially

For the finding, you might consider (108369006, SCT, "Tumor") instead of (RECIST_v2,99EPAD,"Tumor assessment").

This is the template's controlled term saying this is a RECIST template. finding was the place you put the template coded term when we were doing the xslt transformation. Do you think it is a good idea to just put Tumor? I'll discuss with Daniel also

For specifying it is a target lesion, you might consider (112016, DCM, "Baseline Category") = (112074, DCM, "Target Lesion at Baseline") (see the pattern at http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_TID_4102.html#para_929b7c79-ee18-49b2-9eb1-7b7452354fac]

How important do you think this is? Because I'm not sure this is possible. In the RECIST template in ePAD, time point and lesion type are defined separately. I would have to alter the data/terms when converting to DICOM SR. which is doable but very specific and I think it would be very confusing for the user

For enhancing lesions, DICOM SEGs use (C113842, NCIt, "Enhancing Lesion") rather than (RID6055,RADLEX,"Enhancing") http://ncit.nci.nih.gov/ncitbrowser/ConceptReport.jsp?dictionary=NCI_Thesaurus&code=C113842

For the lesion "status", for the presence or absence of things, DICOM SR templates usually use the generic SNOMED CT codes, e.g., (52101004, SCT, "Present"); see http://dicom.nema.org/medical/dicom/current/output/chtml/part16/sect_CID_241.html

There 2 are also coming from the template. If we end up changing the template, which seems like we will, I'll fix it all

@pieper
Copy link
Collaborator

pieper commented Apr 14, 2021

Also, it would be better to use SCT rather than SRT codes, since the former are deprecated.

E.g., instead of (G-A185,SRT,"Long Axis"), use (103339001,SCT,"Long Axis")

which is why dciodvfy reports:

Warning - Coding Scheme Designator is deprecated - attribute =

See http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_8.html#sect_8.1

Makes sense. we also have them in AIM but these are coming from dcmjs. @pieper is it ok for me to change them? I'll fix the AIM ones

Yes, agreed, it would be great if you can update the coding in dcmjs.

Great to see progress on this @emelalkim and thank you @dclunie for your help.

@fedorov
Copy link
Collaborator

fedorov commented Apr 14, 2021

I have just one comments. "Tracking Identifier" is populated to what looks like a mapping to the application-specific tool used to do the annotation. I believe the intent for "Tracking identifier" is to be a human-readable identifier of the finding to facilitate tracking of the finding over time.

@emelalkim
Copy link
Contributor Author

Also, it would be better to use SCT rather than SRT codes, since the former are deprecated.
E.g., instead of (G-A185,SRT,"Long Axis"), use (103339001,SCT,"Long Axis")
which is why dciodvfy reports:
Warning - Coding Scheme Designator is deprecated - attribute =
See http://dicom.nema.org/medical/dicom/current/output/chtml/part16/chapter_8.html#sect_8.1

Makes sense. we also have them in AIM but these are coming from dcmjs. @pieper is it ok for me to change them? I'll fix the AIM ones

Yes, agreed, it would be great if you can update the coding in dcmjs.

Great to see progress on this @emelalkim and thank you @dclunie for your help.

Will do @pieper

@emelalkim
Copy link
Contributor Author

I have just one comments. "Tracking Identifier" is populated to what looks like a mapping to the application-specific tool used to do the annotation. I believe the intent for "Tracking identifier" is to be a human-readable identifier of the finding to facilitate tracking of the finding over time.

Thanks @fedorov, I'll put the annotation name. I just wanted to have @JamesAPetts's opinion first to see if it will mess up cornerstone as it seems to be doing some checks. I don't understand that part. I didn't hear back yet. If I don't hear back in a couple of days. I'll change it and try to see if it will mess up loading in cornerstone

@fedorov
Copy link
Collaborator

fedorov commented Apr 14, 2021

I just wanted to have @JamesAPetts's opinion first to see if it will mess up cornerstone as it seems to be doing some checks.

If it does - we should revisit this as part of the IDC work. I don't think it should!

@emelalkim
Copy link
Contributor Author

I just wanted to have @JamesAPetts's opinion first to see if it will mess up cornerstone as it seems to be doing some checks.

If it does - we should revisit this as part of the IDC work. I don't think it should!

ok. I'll proceed with adding annotation name then. Thanks

@seandoyle
Copy link

I have just one comments. "Tracking Identifier" is populated to what looks like a mapping to the application-specific tool used to do the annotation. I believe the intent for "Tracking identifier" is to be a human-readable identifier of the finding to facilitate tracking of the finding over time.

@fedorov - I don't want to hijack this thread - but I was wondering if the "Tracking Identifier" was meant to be immutable? Here's my use case: in the lumbar spine SR that I'm making I create both a tracking identifier and a tracking uid for labeling each lumbar spine disc (there is also a separate SNOMED code for the disc). But a user can shift the vertebral labels up and down if they disagree with the original labeling. I was planning in this case to change the "Tracking Identifier" but not the uid. We want to use the uid to track the changes that the user made.

An alternative would be to just use a tracking identifier of 'disc1', 'disc2' and make them immutable. This would avoid the problem of the user changing labels with the result being that they were inconsistent.

@igoroctaviano
Copy link
Collaborator

igoroctaviano commented Apr 15, 2021

I have just one comments. "Tracking Identifier" is populated to what looks like a mapping to the application-specific tool used to do the annotation. I believe the intent for "Tracking identifier" is to be a human-readable identifier of the finding to facilitate tracking of the finding over time.

Thanks @fedorov, I'll put the annotation name. I just wanted to have @JamesAPetts's opinion first to see if it will mess up cornerstone as it seems to be doing some checks. I don't understand that part. I didn't hear back yet. If I don't hear back in a couple of days. I'll change it and try to see if it will mess up loading in cornerstone

Yes, the current implementation uses a tracking identifier as a key to map the measurement group to its cornerstone annotation tool-type equivalent. In this issue: OHIF/Viewers#1215 we are discussing new alternatives of doing this because storing vendor-specific names in tracking identifiers seems too brittle and we should use a smart matching strategy or something along those lines e.g. bidirectional measurement properties matched? (e.g. match created based on Clunie's planner annotations paper) then assign the correct vendor tool type.

Example of planar annotation stowed using OHIF:

CodeMeaning - Tracking Identifier
TextValue - cornerstoneTools@^4.0.0:Bidirectional
Tracking Unique Identifier (112040 - DCM)
RelationshipType - HAS OBS CONTEXT

where the first bit of the string identifies a vendor, the second bit is the version, and the last bit the vendor annotation tool type. This string should be interpreted and given to MeasurementService in order to map the SR content to its corresponding annotation format using a mapper implemented by the vendor and registered in OHIF. This is not done currently since this service is still in progress, so what's happening is that inside OHIF the SR is interpreted directly and parsed into annotations. More about this service here: https://github.com/dannyrb/Viewers/blob/f99a6699107c932fb067168ef13ea787d00855c8/docs/latest/services/data/measurements.md

The measurement service + mappings ideal + ideal way of storing things is still an open discussion.

Your incremental changes don't seem to affect our STOWRS/SR logic for cornerstone annotations.
You're adding a comment property to bidirectional (which I think is not interpreted by cstools) and changing tracking unique identifiers which I think is currently ignored when generating the cornerstone report in DCMJS. @JamesAPetts feel free to correct me :)

@dclunie
Copy link

dclunie commented Apr 16, 2021

@igoroctaviano, that doesn't sound like it is going to work if you receive annotations in DICOM SR from another application that doesn't follow your magic convention for how to fill in Tracking Identifier. You should map based on the form of the annotation and allow the Tracking Identifier to be whatever it needs to be for the use case per local procedures (e.g., in a previous life, we required these to follow a certain convention per SOP, such as NT002 for non-target lesion #2, to match entries in our clinical database management system, and not to be constrained by the image annotation tool).

@dclunie
Copy link

dclunie commented Apr 16, 2021

@seandoyle, nothing should be immutable in the workflow (as opposed to a single serialized instance of an SR) - errors happen, so when a user-mislabeled lesion needs to be renamed (e.g., NT001 to NT002), it might (or might not) be appropriate to keep the same UID, depending on the workflow and local convention (e.g., what needs to happen when a new/updated SR with the corrected information is saved and ingested by the downstream systems that use the information, e.g., RECIST calculations).

In your intervertebral disk labeling use case, I might ask the questions:

"what does the user want to see?" to which the answer would seem to be "L2/3" not "disc A" or similar, corrected on a subsequent occasion or not, and

"does it matter if the UID changes" to which the answer is "it depends", on the impact on the downstream workflow, e.g, are there post-processing measurements/decisions indexed by that UID stored separately, such as "thickness", that should remain associated with that annotation even if its human-readable identifier has changed, or those that should not, such as semantic information such as (67459009, SCT, "Intervertebral disc, L2-L3")

Note that in tracking corrections like this, DICOM SR has a predecessor reference mechanism that can be used to determine the head (most recent) "version" to be used (and thence to suppress earlier versions from use), if changes need to be tracked.

@igoroctaviano
Copy link
Collaborator

@igoroctaviano, that doesn't sound like it is going to work if you receive annotations in DICOM SR from another application that doesn't follow your magic convention for how to fill in Tracking Identifier. You should map based on the form of the annotation and allow the Tracking Identifier to be whatever it needs to be for the use case per local procedures (e.g., in a previous life, we required these to follow a certain convention per SOP, such as NT002 for non-target lesion #2, to match entries in our clinical database management system, and not to be constrained by the image annotation tool).

Exactly, that's is the current OHIF approach which at the time was done just as a POC to demo stowrs I guess. This map you mentioned is what I meant in the last comment ...we should use a smart matching strategy or something along those lines e.g. bidirectional measurement properties matched? then assign the correct vendor tool type (e.g. match criteria created based on Clunie's planner annotations paper) . That instead of the current "magic"

I think it's not so easy to come with solid matchers for all tools but we can start with simple ones and then incrementing them as we go. I'll try working on at least one e2e example using measurement service + matching criteria + no tracking identifier and then we can get the ball rolling

@emelalkim
Copy link
Contributor Author

Hi @igoroctaviano and @dclunie,
How about inferring from the points in the SR. I mean SR already has the graphic types of POINT, MULTIPOINT, POLYLINE, CIRCLE and ELLIPSE.
The problematic one is POLYLINE, which can be length, bidirectional or freehand tool in cornerstone. and with bidirectional there are 2 lines. and you can compute if they are perpendicular. that is how we do it in AIM.
Another question is how to handle multiple shapes in one SR. I mean is it possible for the SR to include measurements for two separate lines or two freehand shapes for example? I assume it is, how to handle it then?

@emelalkim
Copy link
Contributor Author

Also @dclunie,
I talked with Daniel and I am going to update the template.
Can you clarify these for me?
-we were thinking of moving all the qualitative evaluations under Measurement Group as those are also about the ROI, in a similar manner to quantitative values. What do you think about that?
Small question, is the "Subject Time Point Identifier" right term to put? I would think something like a Study Time Point would fit better. Is there a term like that?
-I think we have the image library because the measurements refer to the images. no? I think we had this with the xslt transformation too. Am I wrong?

  • About RECIST_v2 coded term :This is the template's controlled term saying this is a RECIST template. finding was the place you put the template coded term when we were doing the xslt transformation. Do you think it is a good idea to just put Tumor?
  • Baseline=Target lesion suggestion: How important do you think this is? Because I'm not sure this is possible. In the RECIST template in ePAD, time point and lesion type are defined separately. I would have to alter the data/terms when converting to DICOM SR. which is doable but very specific and I think it would be very confusing for the user

@pieper
Copy link
Collaborator

pieper commented May 21, 2021

writing dicom sr for segmentations/referring them. does OHIF support that?

I'm pretty sure nobody has done that yet. What I would do is look at the structure of the QIN-HEADNECK data I linked above and just copy it into a new dataset object for conversion to part10. Then as far as I know the best way to test it would be to load it into Slicer's dicom database with the QuantitativeReporting extension installed. If everything is referenced the same as the QIN-HEADNECK conventions then doubleclicking the SR should offer to load the SEG and the referenced images as well. I have done that before for SEG from OHIF, and SR from CrowdsCure, but not SR that references SEG.

@hackermd
Copy link
Member

@pieper @emelalkim the classes in dcmjs.sr.templates should support that in principle. These are pretty much a 1-1 translation of the highdicom implementation. However, not all parts may have been extensively tested so far on the JS side.

@pieper
Copy link
Collaborator

pieper commented May 24, 2021

As we work on this can we add some tests for the sr code just to show how all the pieces should fit together?

@igoroctaviano
Copy link
Collaborator

Hi @igoroctaviano and @dclunie,
How about inferring from the points in the SR. I mean SR already has the graphic types of POINT, MULTIPOINT, POLYLINE, CIRCLE and ELLIPSE.
The problematic one is POLYLINE, which can be length, bidirectional or freehand tool in cornerstone. and with bidirectional there are 2 lines. and you can compute if they are perpendicular. that is how we do it in AIM.
Another question is how to handle multiple shapes in one SR. I mean is it possible for the SR to include measurements for two separate lines or two freehand shapes for example? I assume it is, how to handle it then?

Hey @emelalkim, I was rereading the issue and I think I missed this comment. As Danny said here, I think that's the case for related measurements that are linked by UID? (and span measurement groups)? some measurements will explicitly map to a POINT or POLYLINE, while others are implied or derived from neighboring info. Maybe @dclunie can shed some light on it?

Regarding the "in-progress" adapters code I mentioned earlier, just figured that it's not related to freehand or rect (me and my big mouth), it's just a very early adapters code which is basically what we have in dcmjs but scattered. Feel free to implement the freehand and rect adapters, I'll be happy to help to bring people to validate what you have in mind since this is something that's gonna be needed in OHIF as we add freehand and rect SR support. I can even try to reach out to @JamesAPetts or other team members that worked with the adapters before if needed.

@emelalkim
Copy link
Contributor Author

Hi @igoroctaviano and @dclunie,
How about inferring from the points in the SR. I mean SR already has the graphic types of POINT, MULTIPOINT, POLYLINE, CIRCLE and ELLIPSE.
The problematic one is POLYLINE, which can be length, bidirectional or freehand tool in cornerstone. and with bidirectional there are 2 lines. and you can compute if they are perpendicular. that is how we do it in AIM.
Another question is how to handle multiple shapes in one SR. I mean is it possible for the SR to include measurements for two separate lines or two freehand shapes for example? I assume it is, how to handle it then?

Hey @emelalkim, I was rereading the issue and I think I missed this comment. As Danny said here, I think that's the case for related measurements that are linked by UID? (and span measurement groups)? some measurements will explicitly map to a POINT or POLYLINE, while others are implied or derived from neighboring info. Maybe @dclunie can shed some light on it?

Regarding the "in-progress" adapters code I mentioned earlier, just figured that it's not related to freehand or rect (me and my big mouth), it's just a very early adapters code which is basically what we have in dcmjs but scattered. Feel free to implement the freehand and rect adapters, I'll be happy to help to bring people to validate what you have in mind since this is something that's gonna be needed in OHIF as we add freehand and rect SR support. I can even try to reach out to @JamesAPetts or other team members that worked with the adapters before if needed.

Hi @igoroctaviano, do you mean the tracking Unique Identifier? So if there are more than one ROI in one SR, they would have different tracking unique identifiers.
About '"in-progress" adapters code I mentioned earlier', you mean there is no code for planar annotations, right?

@igoroctaviano
Copy link
Collaborator

Hi @igoroctaviano and @dclunie,
How about inferring from the points in the SR. I mean SR already has the graphic types of POINT, MULTIPOINT, POLYLINE, CIRCLE and ELLIPSE.
The problematic one is POLYLINE, which can be length, bidirectional or freehand tool in cornerstone. and with bidirectional there are 2 lines. and you can compute if they are perpendicular. that is how we do it in AIM.
Another question is how to handle multiple shapes in one SR. I mean is it possible for the SR to include measurements for two separate lines or two freehand shapes for example? I assume it is, how to handle it then?

Hey @emelalkim, I was rereading the issue and I think I missed this comment. As Danny said here, I think that's the case for related measurements that are linked by UID? (and span measurement groups)? some measurements will explicitly map to a POINT or POLYLINE, while others are implied or derived from neighboring info. Maybe @dclunie can shed some light on it?
Regarding the "in-progress" adapters code I mentioned earlier, just figured that it's not related to freehand or rect (me and my big mouth), it's just a very early adapters code which is basically what we have in dcmjs but scattered. Feel free to implement the freehand and rect adapters, I'll be happy to help to bring people to validate what you have in mind since this is something that's gonna be needed in OHIF as we add freehand and rect SR support. I can even try to reach out to @JamesAPetts or other team members that worked with the adapters before if needed.

Hi @igoroctaviano, do you mean the tracking Unique Identifier? So if there are more than one ROI in one SR, they would have different tracking unique identifiers.
About '"in-progress" adapters code I mentioned earlier', you mean there is no code for planar annotations, right?

That's right. 👍

@fedorov
Copy link
Collaborator

fedorov commented Jul 1, 2021

Note that tracking UID is optional.

@dclunie
Copy link

dclunie commented Jul 1, 2021

Here is an example in the standard of using the same Tracking Unique Identifier in two different Measurement Groups to indicate that different kinds of measurements apply to the same finding. The example uses a SEG reference to define the ROI but the same approach would apply if one used a closed POLYLINE to define a contour around the lesion instead, since it may still be necessary to use separate Measurement Groups if one is defining an Area or Volume, and the other linear distance measurements.

http://dicom.nema.org/medical/dicom/current/output/chtml/part17/sect_RRR.5.html

Note also that for the bi-dimensional measurements, each of the two perpendicular lines is (a) encoded separately and (b) labelled with a specific code (long vs. short axis).

See also the discussion in:

https://docs.google.com/document/d/1bR6m7foTCzofoZKeIRN5YreBrkjgMcBfNA7r9wXEGR4/edit#heading=h.ag0f07ftbosu

@fedorov
Copy link
Collaborator

fedorov commented Jul 1, 2021

You would still need to compare the contours point-by-point, no matter whether they have the same UID or not, since I believe the semantics of the UID is to define the finding, not the ROI. So if you have the same finding annotated twice using potentially different contours (I don't think it is forbidden to have this done twice within the same session), I think the expected behavior is that they will have the same UID.

@dclunie
Copy link

dclunie commented Jul 1, 2021

??? Compare what with what for what purpose?

The ROI only needs to be defined once, and the measurements associated with it.

If one is comparing across time points (same lesion, different size/shape), then one can check the relevant entries identifying the time point explicitly or the date time the (set of) entries were made.

@fedorov
Copy link
Collaborator

fedorov commented Jul 1, 2021

David, I was thinking about a scenario where a planar ROI corresponding to the same finding can be present in two different measurement groups within the same SR instance. In this case the findings would have the same tracking UID, but the actual contours may or may not be the same. If the contour points are the same, it would probably be more desirable not to render the same ROI twice. Did I clarify my point?

@emelalkim
Copy link
Contributor Author

@dclunie, @fedorov Here is a sample screenshot of an SR with one ROI with multiple measurement groups. I'll put the DICOM in the releases later today

@dclunie
Copy link

dclunie commented Jul 1, 2021 via email

@dclunie
Copy link

dclunie commented Jul 1, 2021

That's not a good way to do it @emelalkim, you should define the planar ROI coordinates once, and have multiple measurement children.

@emelalkim
Copy link
Contributor Author

@dclunie I did it like that using the work we did during the XSLT transformation as a reference. here is a sample
but it makes sense. I looked through your dicom sr document a little closer and at the end of page 20 it says we should use either

  • TID 1401 to have shape and then list the measurements as children, or
  • TID 1500 and TID 300 and we can omit the geometric shape and we can assume all of the measurements under one measurement group belongs to the same ROI.
    Did I understand correctly?
    As dcmjs uses TID 300, I'd incline towards second option if I understood correctly and if everyone agrees. What do you think? @fedorov @igoroctaviano @pieper

@dclunie
Copy link

dclunie commented Jul 1, 2021

@emelalkim, the TID 1410 approach is much preferred; the shape is important, and one should avoid depending on assumptions.

@emelalkim
Copy link
Contributor Author

@emelalkim, the TID 1410 approach is much preferred; the shape is important, and one should avoid depending on assumptions.

ok. Any objections @igoroctaviano @fedorov @pieper
This would apply to freehand and rectangle I think.
Length and Bidirectional has one measurement each. but there can be more. actually ePAD computes and saves standard deviation and such along the lines. should we convert all to this format? Actually this would also fit to the planar samples Andrey sent better. but we would have to support both because of the old files

@pieper
Copy link
Collaborator

pieper commented Jul 2, 2021

No objections from me. I agree it sounds much cleaner to avoid having the points defined twice.

@JamesAPetts
Copy link
Collaborator

JamesAPetts commented Jul 2, 2021

All I recommend is that someone who is currently working on OHIF test these changes with tracking mode in OHIF v3, for which our original (rather bare bones) implemenation was built for.

Tagging @swederik, who has a closer connection to OHIF than I currently, who can get someone to look at this.

Otherwise this all looks pretty good, and I'm glad these parts of the library are progressing :)

@wayfarer3130
Copy link
Contributor

Hi @emelalkim ,
Are you still working on this PR? I have a smaller change that might make things easier for you:
#225
It mostly won't overlap much with your changes, but the change that does affect things is that there is now a bunch of conversion that is shared in a function for the various cornerstone elements, to read/write finding and findingSite, as well as to
extract some of the things like the pixel data.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants