acil-bwh / SlicerCIP

Slicer extension for the Chest Imaging Platform
BSD 3-Clause "New" or "Revised" License
15 stars 22 forks source link

ENH: add a lung mask segmentation node selector in Parenchyma Analysis #32

Closed rbumm closed 2 years ago

rbumm commented 2 years ago

only works if segmentations for "right lung", "left lung" and "other" are detected in the node

rbumm commented 2 years ago

Thank you, this will be great! I've added a few comments inline.

Note for the future: There are at least 3 different color tables here and label value definitions are scattered throughout SlicerCIP and ChestImagingPlatform at dozens of places. It would be hard to harmonize all these and get rid of the hardcoded label values, so the approach that is implemented here (convert segmentation to labelmaps using label values that the specific module expects) could work well. We would just need to move this conversion to some utility classes that can be accessed from multiple modules so that we don't need to repeat code. But this can be done when we want to add segmentation node support to more modules.

I made the change to a uniform lung mask selector which accepts labelmap and segmentation node. 

I renamed the color table in all places. 

Was not able to delete the temp labelmap and the color table at the end of onApply ()  because if the user presses "Apply" multiple times you get error messages because the temp labelmap is missing for the new analysis. Pls advise. These are only two files, maybe one could just leave them in the scene?

rbumm commented 2 years ago

Unfortunately, this produced another pull request. What should I have done instead and what should I do now resp tomorrow (good night)?

lassoan commented 2 years ago

The proper solution is to keep pushing your changes to the same branch (rbumm/develop) and they show up automatically here. Even if you have modified change history (e.g., amended an existing commit, squashed commits that were already pushed) you can still push and overwrite all previous content on a branch, you just need to force-push (in TortoiseGit push dialog: "Force: May discard known changes").

I would recommend to try to force-push your latest version of the to this branch and if you verified that you see the correct code here then close the other pull request.

lassoan commented 2 years ago

I see that you have pushed some changes. Let me know if the code is ready for review.

rbumm commented 2 years ago

Sry - should be ready for review, Andras -  the code now removes the temporary files from segmentation conversion (only after conversion is really done) and these files are now always created newly. Tested a few CTs and it works well.

rbumm commented 2 years ago

The segmentation to labelmap conversion now happens in the logic. I needed to define a new parameter "segmentationNode" in the logic. If this is present, the conversion takes place and overrides a "labelNode" parameter. Cleanup had to be done by call from from the widget because the widget needs the labelmap for the late statistics. Quite complicated ...

rjosest commented 2 years ago

@rbumm Thank you so much for making these changes. They look good from my end although I am not very familiar with the new segmentation infrastructure. @lassoan I totally agree about the need for unified util for colormap handling. We devise our internal color nodes because the CIP segmentation tools follow a label map convention that is specific to CIP. The least significant 8 bits encode the concept of "Region" and the 8 most significant bits encodes the concept of "Type".

If nobody objects, I can proceed with the merge of the pull request

rbumm commented 2 years ago

Maybe @lassoan should give his final ok. 

@rjosest I enjoyed working on this and use the Airway Segmenter and Parenchyma analysis quite often.

rjosest commented 2 years ago

@rbumm Glad to hear that CIP is useful for your research. Feel to share any improvements

lassoan commented 2 years ago

I'll have a look at the code later this afternoon, you could hold off the merge until then.

rbumm commented 2 years ago

@lassoan just a reminder here thanks

rbumm commented 2 years ago

@lassoan this probably got forgotten - could you check the code if you find the time? Thank you.

lassoan commented 2 years ago

Thanks for the reminder. I've been working on a regression in the Lung CT segmentation module caused by the update of Markups module last week. I've managed to fix that problem yesterday and will get back to review and test this code today.

rbumm commented 2 years ago

Closed, because now included in #34 .

rbumm commented 2 years ago

I've been working on a regression in the Lung CT segmentation module caused by the update of Markups module

@lassoan there is still a problem in Lung CT Segmenter concerning markup placement. green slice constantly switches back to red slice after adding the 7th marker (Preview Slicer 4.13.0-2021-09-28)