Page MenuHomePhabricator

[MultiLabel Segmentation] Default parameter (unsigned int layer = 0) is unnecessary and causes problems
Open, NormalPublic

Description

Several functions inside mitkLabelSetImage.h use unsigned int layer = 0 as default parameter / value. I remember that this led to a lot of problems:
Developers did not get a compile-error when using no argument (e.g. calling GetActiveLabel()) but the wrong active label is returned, if the user is working on a different layer (e.g. layer 2).
I don't see any necessity to provide a default parameter so I would like to remove it and replace all calls to functions with default parameters to function calls with a specific layer argument.

Related Objects

StatusAssignedTask
OpenNone
Openkalali
OpenNone
OpenNone
Opens349i
Resolvedfloca
Resolvedfloca
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
OpenNone
Resolvedkalali
Resolvedfloca
Openfloca
OpenNone
OpenNone

Event Timeline

kalali triaged this task as Normal priority.Jan 3 2021, 4:04 PM
kalali created this task.

Just out of curiosity I removed all default parameters for the following functions:

  • mitkLabelSetImage::MergeLabel
  • mitk::LabelSetImage::MergeLabels
  • mitk::LabelSetImage::UpdateCenterOfMass
  • mitk::LabelSetImage::RemoveLabels
  • mitk::LabelSetImage::EraseLabel
  • mitk::LabelSetImage::EraseLabels
  • mitk::LabelSetImage::GetActiveLabel
  • const mitk::LabelSetImage::GetActiveLabel
  • mitk::LabelSetImage::GetLabel
  • mitk::LabelSetImage::GetLabelSet
  • const mitk::LabelSetImage::GetLabelSet
  • mitk::LabelSetImage::GetNumberOfLabels
  • mitk::LabelSetImage::CreateLabelMask

And got the following errors:

image.png (774×2 px, 133 KB)

I just stumbled upon the following:

Inside of the LabelSetImagethere are two functions (four with const):

  • mitk::Label *GetActiveLabel(unsigned int layer = 0);
  • const mitk::Label* GetActiveLabel(unsigned int layer = 0) const;

and

  • mitk::LabelSet *GetActiveLabelSet();
  • const mitk::LabelSet* GetActiveLabelSet() const;

Inside the GetActiveLabelSet-functions to access the vector GetActiveLayer is always used. This makes sense as the name of the function suggests.
Inside the GetActivelabel-functions to access the vector the given (possibly default) argument is used. This does not make sense and is counterintuitive. This is a special case where the default layer parameter is not only unnecessary but completely misleading. I have never seen a call to GetActiveLabel without the typical argument (GetActiveLabel(GetActiveLayer()), so removing the default parameter here will probably do no harm but remove obscurity. Since it was said to not work too much on the LabelSetImage, I just ping @floca and @kislinsk here. At least this should be considered for a new data structure, but it could also be fixed quickly.

in the new data structure it won't happen as there is no active label anymore. But you are right I thought the same when I looked through the class. My motiviation to refine this obsolete class was only limited 🙈 .