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

Add roi utils for creating masks #18

Closed
wants to merge 4 commits into from

Conversation

manics
Copy link
Member

@manics manics commented Feb 27, 2019

Adds two helper methods for creating masks from binary and labelled image arrays.

If there is an empty intermediate label it's unclear what the correct behaviour should be, so let the user choose between an exception and empty mask
:param t: Optional T-index for the mask
:param text: Optional text for the mask
:param raise_on_no_mask: If True (default) throw an exception if no mask
found, otherwise return an empty Mask
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure what the default should be for raise_on_no_mask.

Copy link
Member

Choose a reason for hiding this comment

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

If I read correctly the code, no mask means no non-zero pixels found. Depending on the use case, I can certainly see two behaviors:

  • return an empty Mask
  • return None or throw an exception

The former call is more friendly to the caller of this API as it ensures that a mask is always returned. However from an IDR consumer perspective, I am not sure that there is value in storing an empty Mask rather than no mask. Given this API already has some logic to find bounding boxes, I think raising exception by default but having the possiblity to change the behavior is totally acceptable.

src/omero_metadata/rois.py Outdated Show resolved Hide resolved
@will-moore
Copy link
Member

This sounds really useful (I always struggle to create masks for testing) but any pointers on how to use it would be great (without me having to read all the code)?
Also, to test a particular branch what do I need to do to enable this plugin? Build? Put on PYTHONPATH etc?

@manics
Copy link
Member Author

manics commented Feb 28, 2019

@will-moore I documented usage in the python method docs, but as you've pointed out that's displayed anywhere accessible. @sbesson Reckon we could experiment with generating docs on https://readthedocs.org/ ?

For testing: pip install https://github.com/ome/omero-metadata/archive/22ccc9d46fdb34e87d7d96b46b84276c10b7f243.zip or pip install https://github.com/manics/omero-metadata/archive/roi-utils.zip, adding to PYTHONPATH should also work.

@sbesson
Copy link
Member

sbesson commented Feb 28, 2019

Same thought as @will-moore, thanks for adding such utilities especially as we re-use the same pieces of code.

Main initial feedback is that the omero-metadata was specifically designed to deal with annotations (tables and maps primarily). Adding ROI functionality feels a bit like shoehorning features in an existing project.

Several discussion had been started on a library/plugin to deal with ROIs - see ome/design#80 or IDR/idr-metadata#292. Should we start opening this work in a separate dedicated project e.g. omero-roi? Is there code elsewhere that we would like to integrate? /cc @dom

Readthedocs is an interesting idea to use external infrastructure to host Python docs. Requires gaining knowledge so no objection to start experimenting on a new component.

@manics
Copy link
Member Author

manics commented Feb 28, 2019

If we're going for another repo we need to decide whether we want loads of small libraries or something like omero-gateway-extras.

@snoopycrimecop snoopycrimecop mentioned this pull request Mar 17, 2019
@manics
Copy link
Member Author

manics commented Apr 3, 2019

This was moved to https://github.com/ome/omero-rois/tree/v0.2.0

@manics manics closed this Apr 3, 2019
@manics manics deleted the roi-utils branch April 3, 2019 11:42
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.

4 participants