imglib / imglib2-roi

Regions of interest (ROIs) and labelings for ImgLib2
Other
8 stars 8 forks source link

Simplify construction and decomposition of ImgLabeling #38

Closed maarzt closed 5 years ago

maarzt commented 6 years ago
tpietzsch commented 6 years ago

I explicitly made it hard to construct an ImgLabeling in this way by locking it down with the LabelingAccess. The reason is, that it is easy to create a non-functional ImgLabeling if the image and the labelSets don't match up. The labelSets need to have the empty set at index 0 and there must be no higher value in the image than labelSets.size(). I see that it is convenient to be able to construct ImgLabeling like that and allow people to shoot themselves in the foot if they want to. The way to do it then is to open it up really, the detour via LabelingAccess is not be necessary. Also, the requirements outlined above should be made very explicit in the javadoc of the new methods.

Extending AbstractList is okay, but also it should be made clear in the javadoc that it is an unmodifiable list.

And should be formatted with imglib2 code style

maarzt commented 6 years ago

I added javadocs & fixed the coding style.

I tried to completely remove the SerializationAccess, but I think that would invite people to much to shoot their foot. It would add the method imgLabeling.getMapping().setLabelSets(...). I cannot think of a consistent way to use this method after initialization.

imagejan commented 5 years ago

If you only have an index image and no labelSets, how would you go about creating the labelSets from the image? Are there utility methods already?

Would a new method ImgLabeling.fromImg(Img img) (that iterates over the image and collects all existing indices - ignoring 0 - to create the label set) make sense?

haesleinhuepf commented 5 years ago

Hey all,

there are some people on the forum asking for the functionality behind this PR. https://forum.image.sc/t/construct-labelregions-from-labelmap/20590 https://forum.image.sc/t/h-watershed-and-labelregions/26918

Is there a way to pull this PR through as requested?

Thanks!

Cheers, Robert

ctrueden commented 5 years ago

@haesleinhuepf The people formally responsible for reviewing PRs for this component are @tpietzsch, @StephanPreibisch and @axtimwalde. See the pom.xml, looking for reviewer role. So they are the folks to ping in this case. In general, check the pom.xml and ping whoever the reviewers are. If there are no reviewers, then I think pinging the lead and/or maintainers makes sense.

maarzt commented 5 years ago

@tpietzsch reviewed the PR already. I will make the requested changes:

  1. Add the warning in javadoc.
  2. Remove or deprecate LabelingAccess if possible.
maarzt commented 5 years ago

@tpietzsch I made the changes that you asked for.

Remark: I don't think this PR makes it more likely for user to shut themself in their foot. Previously users could have created malfunctioning ImgLabelings when they used non zero index images. The situation stays exactly the same: If you create an ImgLabeling, you need to have a matching index image.

tpietzsch commented 5 years ago

Thanks @maarzt