Update DICOMSegmentationPlugin to use dcmqi instead of "old" seg2nrrd cli

[che85]

• [x] Update to dcmqi cli (@che85)
• [x] Create terminology from cli output json (@cpinter)

[fedorov]

@cpinter can you help with this task? There is a comment you put in the plugin, but the plugin has not been updated after terminology support has been revised: https://github.com/QIICR/Reporting/blob/master/Py/DICOMSegmentationPlugin.py#L145

[che85]

@cpinter @Sunderlandkyl: Do you still need that code[1] for exporting labels from the "old" Editor into DICOM SEG? Are you planning to adapt that code for the new terminologies?

[1] https://github.com/QIICR/Reporting/blob/master/Py/DICOMSegmentationPlugin.py#L393-L581

[fedorov]

@cpinter can you give us an idea if/when you might be able to look into this? This issue is in the critical path, and we need to plan who will work on resolving it. Thanks.

[cpinter]

Yes I'll look into this asap.

[fedorov]

Thank you!

[fedorov]

@cpinter you probably noticed this, but @che85 already did some work to update the plugin to use dcmqi. It probably makes sense to start with his pull request here: https://github.com/QIICR/Reporting/pull/86

[cpinter]

@che85 "Do you still need that code[1] for exporting labels from the "old" Editor into DICOM SEG"

The function you marked is the one that does the export, so we need that definitely. Although it does use features that have been changed since then (usage of color table node, LabelMapVolume base class, etc). I'll change that.

I don't know much about the segimage2itkimage CLI that this issue is about. What should I do about it exactly?

[che85]

@cpinter segimage2itkimage produces nrrd segmentation files for each segment from the DICOM SEG file. Futhermore it creates a json metafile with additional attribtutes about the segments.

Actually I would only need to know, how to get that information [1] into Slicer SegmentationNodes. And secondly how would you suggest to work with terminologies, when the context for the codes used for those segments is not defined in the files that you are loading when Slicer is loaded.

[1] https://github.com/QIICR/dcmqi/blob/master/doc/seg-example.json#L16-L33

[cpinter]

@che85 Should we have a hangout? I'm available in the next 20 minutes so can talk right now. Probably that would be fastest

[che85]

@cpinter I will have a meeting in a few minutes. Maybe in the afternoon? I will be available between 2pm and 8pm

[cpinter]

OK I'll let you know. I wanted to reach you on google hangouts but their interface is frustrating. I'll keep trying Update: Sent an invite

[fedorov]

Guys, keep me in the loop - I would like to join the meeting if I can

[cpinter]

Of course! When are you available this afternoon? I'm somewhat flexible, but cannot make it before 1:45 for sure

[fedorov]

how about 3pm?

[cpinter]

Works for me

[pieper]

I'd be interested in listening in on that conversation if you don't mind.

On Thu, Nov 17, 2016 at 11:46 AM, Csaba Pinter notifications@github.com wrote:

Works for me

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/QIICR/Reporting/issues/85#issuecomment-261300232, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHsfWYkFhnVQ-jRL9-g-8BgfnUn3rbaks5q_ITxgaJpZM4KyEFq .

[fedorov]

@pieper sure - let's hope @che85 is available at 3 and plan for that. I will initiate and send hangout links (and hangouts recognized my camera earlier today, so hopefully it will work!)

[che85]

Sure I will be available.

[cpinter]

@che85 I'm looking at the seg-example.json file you referenced, and I'm a bit confused. In the json terminology dictionaries used so far, the category, type, modifiers etc. are identified by the tags "codeValue", "codingScheme", "codeMeaning", but in the example file these are different: "CodeValue", "CodingSchemeDesignator", "CodeMeaning". The parser I implemented thus won't be able to parse the example file. Is this difference intentional? Is there a need to make the parser more flexible?

[fedorov]

Csaba, it's our fault. It should be capital case. This was fixed in the schemas for the converters, but not in the context JSONs, since we don't have a schema for the context, so this was not captured by the tests.

Can you update the context files in Slicer to have "CodeValue", "CodingSchemeDesignator", "CodeMeaning" and assume this convention going forward? I will fix in the dcmqi version.

The convention we've been using is that attributes that map directly to DICOM start with upper case, and other arguments start with lower case.

[cpinter]

Thannks, Andrey! I'll update the json files in the Slicer repo.

[cpinter]

What about Slicer4\Base\Logic\Resources\ColorFiles\Terminology\AnatomicRegionAndModifier.xml and the other xml in that folder?

[fedorov]

I don't know if they are used by anything in Slicer, but for our purposes, these are deprecated. The XML files were used by Colors logic, and that functionality is being phased out. Probably for now it is safer to leave that file in place since probably Colors logic and other things still expect its presence.

cc: @naucoin @pieper

[cpinter]

I worked today on identifying the codes by coding scheme and value instead of code meaning, and passing the custom color separately (it was not quite correct to squeeze in custom color info into the passed terminology information), while making it more robust. I'll work on the CLI tomorrow.

[cpinter]

This work turned out to be bigger than I thought, so I only finished this afternoon. I committed the capitalization fixes in the same commit as the CodingSchemeDesignator/CodeValue one.

Now I'm trying to load the test DICOM SEG into Slicer, but I'm facing an error that might be trivial but I still couldn't find the solution easily. The "import dicom" python command fails, saying there is no such package. I checked the CMake variables but I can't find anything related to this that I might have missed enabling. Any ideas?

[fedorov]

Are you working off the master with the updates from @che85 included? I tested with the nightly from yesterday, and everything works fine (including "import dicom") - it even populates the color, and the loaded segment shows up in Segment Editor, it's just that terminology is missing.

[lassoan]

Csaba, you need to build Slicer with SSL enabled to get pydicom.

[cpinter]

Thanks, I'll build with SSL then. I disabled it quite a while ago because it caused problems on Windows, I hope it works well now.

[lassoan]

SSL works for me with Qt that I build with Jc's one-liner script.

[cpinter]

It works now, thanks for the tip! I'll start on terminology loading tomorrow.

[cpinter]

How final is the segmentation descriptor json file (meta.json) structure? Because currently the terminology logic uses an in-memory terminology context json object for lookup, so I need to write a segmentation decriptor Json to terminology context Json (not file but in memory) converter so that it can be stored as a context properly. It won't be hard, but I'd like to know if I need to prepare for later changes.

A few minor comments (some of them may be redundant):

• print statements are not really useful
  • The ones saying trivial things like 'DICOM SEG modality found' or 'DICOM SEG load()' are only useful for debugging but otherwise just clutter the output. I suggest removing them once loading is tested and released
  • The ones logging error should use the logging package so that it shows up in the application log as error: logging.error('Message')
• There is a memory leak after loading
• It would be nice to delete the temporary files after loading finished successfully (after tested and released)

[lassoan]

For debug messages you can use logging.debug. Debug output is only shown in the log file and log window - does not pollute the console.

[cpinter]

@che85 One more question: I'm looking at JSONSegmentationMetaInformationHandler::createAndGetSegmentAttributes and am a bit puzzled about innerList. To me it seems redundant, but I'm not too experienced with Json. Is it really needed?

[che85]

@cpinter I am not sure if the segmentation descriptor is complete, but I am sure @fedorov can tell you.

It would be nice to delete the temporary files after loading finished successfully (after tested and released)

I totally agree about removing the temporary files afterwards and it's on my TODO list.

There is a memory leak after loading

Do you know where the memory is coming from? I guess from segimage2itkimage, but I am not sure.

JSONSegmentationMetaInformationHandler::createAndGetSegmentAttributes and am a bit puzzled about innerList. To me it seems redundant, but I'm not too experienced with Json. Is it really needed?

segmentAttributes is a list of lists. The outer list are different segmentation files, where each file can have several segments (that can have the same pixel value as one of another segmentation file).

[cpinter]

@che85 Thanks for the quick answers! The inner list makes complete sense this way. It's just if I look at the code in the function I mentioned then if it looks like to me that the innerList value will always have one element in it. https://github.com/QIICR/dcmqi/blob/master/libsrc/JSONSegmentationMetaInformationHandler.cpp#L129-L130

[che85]

You are right. Maybe @fedorov can answer you why: I also would expect >=1 segment(s) in each file.

https://github.com/QIICR/dcmqi/blob/master/libsrc/JSONSegmentationMetaInformationHandler.cpp#L96

[cpinter]

Thinking further, meta.json seems to correspond to exactly one segmentation object. It specifies InstanceNumber, SeriesDescription, and SeriesNumber, so unless a DICOM SEG series can be contained in multiple files (unilke RTSS), then having containers in it for multiple segmentation files is unnecessary.

[fedorov]

Sorry for the delay in reply, had to deal with multiple immediate and more urgent "fires"...

@cpinter I completely agree there are cleanup items we have to work on, and we will get to it after RSNA.

I expect the structure of meta.json will change, but we can take care of updating the plugin if/when this happens. If I understand correctly from @che85 all we need for now are setters for the terminology of the segments. How difficult is providing just those?

The inner list makes complete sense this way. It's just if I look at the code in the function I mentioned then if it looks like to me that the innerList value will always have one element in it. https://github.com/QIICR/dcmqi/blob/master/libsrc/JSONSegmentationMetaInformationHandler.cpp#L129-L130

Took me a little bit to recall the motivation for this. Yes, you are correct that while reading a segmentation object, we (for now) save each segment into a separate file, and there is a unique metafile associated with each label file. Each of those metafiles will describe just one segment. The reason for the inner list is that we have one single schema both for input and output. When we provide input metafile, we can have multiple input files, and each file can have multiple labels, that is why we need to have list of lists. Does this explanation make sense?

Again, I think it might be easier, given the little time left, if Christian works on parsing JSON, since he is very familiar with those objects. All we need really are the setters for terminology.

[cpinter]

@fedorov Thanks for the innerList explanation! Yes, it makes good sense.

I'm done with the Json parsing, and now I'm working on adding the terminology entries for the loaded segments. I'm also removing obsolete snippets from the plugin. wondering if this part is needed or not https://github.com/QIICR/Reporting/blob/master/Py/DICOMSegmentationPlugin.py#L267-L286

[fedorov]

wondering if this part is needed or not https://github.com/QIICR/Reporting/blob/master/Py/DICOMSegmentationPlugin.py#L234-L263

I guess it depends whether this is the proper way to read in segments from disk files or not -- if this is not the right way to do it, please replace or let us know how to improve!

[cpinter]

Sorry I just updated my comment. I meant the part under that which creates the merged label volume.

I'm not debating the method of loading right now :)

[fedorov]

Oh - so this time I was too quick! I can't win!! :)

I think this code is only relevant for the "legacy" Editor. Compatibility with legacy Editor is not a priority for us, so you can feel free to remove it. Looking in the "Blame" mode gives some perspective (commit https://github.com/QIICR/Reporting/commit/d204b8bd71c0015f3c64e52d80fe533736601051)

[cpinter]

Thanks a lot! I'll remove it in my pull request, and if needed later we can find it in the history.

[cpinter]

I think I'm making good progress. One question in the meantime: what should be default value for showAnatomy in a category? Because if we load the segmentation's terminology information to a brand new context, then the incomplete categories (no "cid" etc. only the three Code* values) from the descriptor Json file need to be initialized in some way. I vote for showAnatomy be true by default, otherwise there is no chance setting region information for a loaded DICOM SEG at the moment.

[fedorov]

yes, I agree - makes sense to show anatomy selection when showAnatomy is not available in the input context

[cpinter]

I committed the descriptor parsing part to Slicer core: http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=25563 And added the terminology information in the loaded segments in the DICOM plugin: https://github.com/QIICR/Reporting/pull/97

Let me know how it works and if there are any concerns/comments

[fedorov]

The remaining task was fixed in #97