Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify construction and decomposition of ImgLabeling #38

Merged
merged 4 commits into from
Jul 5, 2019

Conversation

maarzt
Copy link
Contributor

@maarzt maarzt commented Jul 18, 2018

  • To easily setup and ImgLabeling from given data use:
    imgLabeling = ImgLabeling.fromImageAndLabelSets(img, labelSets)
  • To decompose a ImgLabeling use:
    Set labelSets = imgLabeling.getMapping();
    RandomAccessibleInterval img = imgLabeling.getIndexImg();

final RandomAccessibleInterval< I > img,
final List< T > labels)
{
List<Set<T>> labelSets = Stream.concat(Stream.<Set<T>>of(Collections.emptySet()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find this much less readable than

List<Set<T>> labelSets = new ArrayList<>(Collections.emptySet());
labelSets.addAll( labels );

Please change

@tpietzsch
Copy link
Member

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 maarzt force-pushed the add-imglabeling-factory-method branch from b6e95b9 to 2d8280b Compare July 24, 2018 16:12
@maarzt
Copy link
Contributor Author

maarzt commented Jul 24, 2018

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.

* To easily setup and ImgLabeling from given data use:
  imgLabeling = ImgLabeling.fromImageAndLabelSets(img, labelSets)
* To decompose a ImgLabeling use:
  List< Set< T > > labelSets = imgLabeling.getMapping();
  RandomAccessibleInterval< IntegerType > img = imgLabeling.getIndexImg();
@maarzt maarzt force-pushed the add-imglabeling-factory-method branch from 2d8280b to cae3253 Compare July 25, 2018 08:27
It has been a todo comment, and it makes a lot of sense.
@imagejan
Copy link
Member

imagejan commented Feb 1, 2019

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
Copy link

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
Copy link
Member

@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
Copy link
Contributor Author

maarzt commented Jul 4, 2019

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

  1. Add the warning in javadoc.
  2. Remove or deprecate LabelingAccess if possible.

We don't need it as there is the getLabelSets method. And users might
be confused by the List is not modifiable.
@maarzt
Copy link
Contributor Author

maarzt commented Jul 4, 2019

@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 tpietzsch merged commit d94403c into master Jul 5, 2019
@tpietzsch
Copy link
Member

Thanks @maarzt

@tpietzsch tpietzsch deleted the add-imglabeling-factory-method branch July 5, 2019 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants