-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Support contours in future.manual_segmentation #2966
base: main
Are you sure you want to change the base?
Support contours in future.manual_segmentation #2966
Conversation
Hello @aaronsnoswell! Thanks for updating the PR. Cheers ! There are no PEP8 issues in this Pull Request. 🍻 Comment last updated on February 16, 2018 at 06:49 Hours UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quite useful enhancement! Couple of remarks from me, otherwise ok
@@ -49,6 +50,9 @@ def manual_polygon_segmentation(image, alpha=0.4, return_all=False): | |||
labels : array of int, shape ([Q, ]M, N) | |||
The segmented regions. If mode is `'separate'`, the leading dimension | |||
of the array corresponds to the number of regions that the user drew. | |||
contours : array of float, shape (Q, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shape of contours
is clearly not correct here. Has to be a list of lists or something similar.
|
||
|
||
def manual_lasso_segmentation(image, alpha=0.4, return_all=False): | ||
"""Return a label image based on freeform selections made with the mouse. | ||
"""Return a label image and list of contours based on freeform selections |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keep it a one-liner. If more detailed description is required, put it 2 lines below.
Also, fix a double whitespace.
@@ -160,6 +171,9 @@ def manual_lasso_segmentation(image, alpha=0.4, return_all=False): | |||
labels : array of int, shape ([Q, ]M, N) | |||
The segmented regions. If mode is `'separate'`, the leading dimension | |||
of the array corresponds to the number of regions that the user drew. | |||
contours : array of float, shape (Q, 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for the shape here.
@@ -212,6 +227,11 @@ def _on_lasso_selection(vertices): | |||
labels = (_mask_from_vertices(vertices, image.shape[:2], i) | |||
for i, vertices in enumerate(list_of_vertex_lists, start=1)) | |||
if return_all: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update return_all
parameter description for both functions to include also info on contours.
@aaronsnoswell hi! Did you have time to make the requested changes? |
This commit adds support for optoinally returning contours from the functions manual_lasso_segmentation and manual_polygon_segmentation. Sometimes a contour is more useful to you than a mask.
5acf2aa
to
579fa1e
Compare
While returning polygon vertices (PVs) / contour points (CPs) would be a nice addition to the manual segmentation functions, it comes here with a challenge. With I would advocate for keeping the manual segmentaiton tools as simple as possible (or, perhaps, even removing them at some point) and encouraging users to learn and utilize the professional tools for image annotation (such as Napari, ITK-SNAP, Slicer3D, etc). Since there has been no activity on this PR for a while and the implementation is somewhat early-stage, I propose to close the PR until further notice. |
Description
This PR updates the functions
future.manual_segmentation.manual_polygon_segmentation
andfuture.manual_segmentation.manual_lasso_segmentation
. The changes add a second return parameter that is the actual list of contour points. This is sometimes more useful to an end user than the mask only.NB: This will break previous APIs that depended on these functions. Old code might have looked like this (from the docstrings);
Now, users will have to add a temporary variable to catch the contour, even if they are not interested in it; viz.
Docstrings are updated and the changes were fairly minimal - I haven't read all of PEP8, but I believe it should still be compatible (unless multiple return statements are a no-no).
Checklist
[It's fine to submit PRs which are a work in progress! But before they are merged, all PRs should provide:]
./doc/examples
(new features only)Release notes message: