Random Review comments

[Connor-Bowley]

This is a collection of random small comments or questions that don't necessarily warrant their own issue yet. I am putting individual topics in separate comments. If any of them rise to level of an individual issue, we use the "Reference in new issue" feature to create a new issue for it.

[Connor-Bowley]

The word contributors can be removed from the text string.

https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/e6cfcd3fad261feecc65b6a01d939213960a3e3f/CMakeLists.txt#L9

[Connor-Bowley]

Highly recommend making self.inputChoice an enum so it is self documenting what the choices are.

How to make an enum: https://docs.python.org/3.9/library/enum.html#creating-an-enum

Location: https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/e6cfcd3fad261feecc65b6a01d939213960a3e3f/prediction/prediction.py#L69

[Connor-Bowley]

In onResolutionChanged, you set self.resolution to an int, but in setup you set it to a string. Did you mean to convert to int in setup?

https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/e6cfcd3fad261feecc65b6a01d939213960a3e3f/prediction/prediction.py#L254-L255 https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/e6cfcd3fad261feecc65b6a01d939213960a3e3f/prediction/prediction.py#L144

[Connor-Bowley]

Recommend switching to use qMRMLNodeComboBox instead of just QComboBox for nodesComboBox. qMRMLNodeComboBox can be given the MRML scene so it will update automatically as nodes are added and removed. You can filter down to the types of MRML nodes you want.

See the new SReps for an example https://github.com/KitwareMedical/SlicerSRep2/blob/cb83edb0d9f62355ea6fe0fb5c30b2952597b5a5/SRepRefinement/qSlicerSRepRefinementModuleWidget.cxx#L83 https://github.com/KitwareMedical/SlicerSRep2/blob/cb83edb0d9f62355ea6fe0fb5c30b2952597b5a5/SRepRefinement/qSlicerSRepRefinementModuleWidget.cxx#L94 https://github.com/KitwareMedical/SlicerSRep2/blob/cb83edb0d9f62355ea6fe0fb5c30b2952597b5a5/SRepRefinement/Resources/UI/qSlicerSRepRefinementModuleWidget.ui#L27-L36

[Connor-Bowley]

Since you are adding an observer to the cliNode, you should probably remove it when you are done with it.

https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/e6cfcd3fad261feecc65b6a01d939213960a3e3f/prediction/prediction.py#L316

[Connor-Bowley]

Is this file even used? https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/main/predictioncli/_CMakeLists.txt

[MathieuLeclercq]

I have addressed all the comments that you have made on this issue. Thanks for the advice.

[MathieuLeclercq]

Are there any other comments? Can we close this issue?

[Connor-Bowley]

I still do not see where you are removing this observer.

https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/e6cfcd3fad261feecc65b6a01d939213960a3e3f/prediction/prediction.py#L316

This line (https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/a048c5bc7a4fbbf43f0d3c1b1742ba51f3a9bda4/CrownSegmentation/CrownSegmentation.py#L433) will not remove that observer, as it only removes observations added through the VTKObservationMixin base class (e.g. https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/a048c5bc7a4fbbf43f0d3c1b1742ba51f3a9bda4/CrownSegmentation/CrownSegmentation.py#L133) and not through the normal observation way (e.g. https://github.com/MathieuLeclercq/SlicerJawSegmentation/blob/a048c5bc7a4fbbf43f0d3c1b1742ba51f3a9bda4/CrownSegmentation/CrownSegmentation.py#L372)

You could rewrite the observation to be able to be removed via self.removeObservers() by doing something like

self.addObserver(self.logic.cliNode, vtk.vtkCommand.ModifiedEvent, self.onProcessUpdate)

[MathieuLeclercq]

I changed the code according to your suggestion. Thank you.