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

Manual segmentation tool with matplotlib #2584

Merged
merged 29 commits into from Apr 19, 2017

Conversation

@pskeshu
Copy link
Contributor

pskeshu commented Mar 24, 2017

I was looking for a way to segment images manually in python, and since I couldn't find one, I used matplotlib to make a very basic manual segmentation tool. The user will be able to use the cursor to draw around multiple objects, and generate a binary mask. I would very much like your valuable suggestions.

This is my first PR, so I apologize if this is too messy or not relevant to the project.

@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented Mar 24, 2017

Hello @pskeshu! Thanks for updating the PR.

Line 9:80: E501 line too long (87 > 79 characters)

Comment last updated on April 14, 2017 at 07:16 Hours UTC
@soupault

This comment has been minimized.

Copy link
Member

soupault commented Mar 24, 2017

Hi @pskeshu ! This is interesting!
Could you evaluate how complex it would be to make the following changes:

  1. Add support for multichannel (e.g. RGB) images;
  2. Speedup the mask creation;
  3. Handle multiple overlapping lasso selections in a more intuitive way (at the moment, it behaves in a complex way in intersection areas);
  4. Fill the regions with the corresponding color of high transparency (so to better see the region while keeping the background visible);
  5. Implement undo function (e.g. remove the latest region by pressing some button).
@pskeshu pskeshu force-pushed the pskeshu:manual-segmentation branch from ba5c4b4 to 677deb0 Mar 24, 2017
@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Mar 24, 2017

Hi @soupault ,

Thanks a lot for going through my PR.
You've pointed out some really good suggestions. I have implemented a few of them, and I'm thinking of the best way to implement the other changes you've recommended.

1 I have added support for RGB images. But I'm not sure if my implementation is the right way to go
about it.

3 For now, the function returns a bool image, and the current behavior is to merge intersecting areas. It could be a bit more intuitive than that, but I suppose that will make the tool a lot more complicated. Perhaps I can generate a separate mask for each selection, and return an array of masks.

5 This is done. Undo can now undo all previous selections, one at a time.

@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Mar 25, 2017

Hi @soupault ,

I have made further changes to the code to incorporate all the changes you'd suggested earlier.

  1. Speedup the mask creation;

Speeding up the mask is not quite easy, as the slowest part of the code is contains_points from the matplotlib.path.Path. Instead of fiddling with matplotlib, I have introduced an optional parameter "speed_up", which will essentially slice the list of vertices for a given polygon with a step-size as given by speed_up. By keeping half the number of vertices by slicing alternative coordinates of the polygon with a speed_up value of 2 has 2x better performance. With speed_up value of 4, mask creation is 4x faster. However, this will also destroy the smoothness of the segments to a very small extent.

  1. Fill the regions with the corresponding color of high transparency (so to better see the region while keeping the background visible);

The code has been modified to draw a polygon around the selected object with some transparency.

Copy link
Contributor

jni left a comment

Hi @pskeshu! Thanks so much for this PR, it's very cool and I think it'll make a great addition to the package! I think we'll need a few iterations to get it into a style that fits into skimage, but I'm sure we can get there! I've left some comments to get started.

Parameters
----------
image : (M, 2) array

This comment has been minimized.

Copy link
@jni

jni Mar 25, 2017

Contributor

(M, N[, 3]): it is a 2-dimensional object, with dimensions of size M and N, and optionally a third (channels) dimension.

if image.ndim not in (2, 3):
raise TypeError('Only 2-D images or 3-D images supported.')

manual.list_of_verts = []

This comment has been minimized.

Copy link
@jni

jni Mar 25, 2017

Contributor

Whoa, why is this going as a runtime attribute of the function? ie why not just list_of_verts = []? I think you're wanting to make this an OO interface but I don't think it's needed (yet) in this case. =)

manual.polygons_selection = []
manual.polygon_list = []

_select_lasso(image)

This comment has been minimized.

Copy link
@jni

jni Mar 25, 2017

Contributor

In general we prefer a functional programming style in scikit-image. So, similarly, in this case, I would go with verts = _select_lasso(image).

undo_button = matplotlib.widgets.Button(buttonpos, "Undo")
undo_button.on_clicked(_undo)

lasso = matplotlib.widgets.LassoSelector(_select_lasso.ax, _onselect)

This comment has been minimized.

Copy link
@jni

jni Mar 25, 2017

Contributor

Here, the way you've implemented it, _on_select needs to refer to a "global" attribute, manual.list_of_verts. Instead, you can create a nested closure, just above:

list_of_verts = []
def _on_select(verts):
    list_of_verts.append(verts)
    # ...
lasso = matplotlib.widgets.LassoSelector(_select_lasso.ax, _onselect)
return list_of_verts
all_pixels = np.vstack((x_grid.ravel(), y_grid.ravel())).T

for verts in manual.list_of_verts:
verts_ = verts[::speed_up]

This comment has been minimized.

Copy link
@jni

jni Mar 25, 2017

Contributor

I definitely don't like this way of speeding things up. Users expect that their clicks will have an effect. Objects containing "spiky" shapes will definitely fail with this approach.

Maybe try using skimage.draw.polygon instead of matplotlib.path.Path? I don't know whether it'll be faster but it's worth a shot.

@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Mar 26, 2017

Hi @jni ,

Thank you so much for going through the code! :)

I have cleaned up the code incorporating all the changes you'd suggested, and thank you so much for the tip on draw.polygon. It works so much faster than contains_points, and the mask generation takes a fraction of a second with it for the same vertices which used to take about 20s in contains_points.

I'd be glad if you could go through the code again and see if there is anything else that needs improvement. :)

ax.imshow(image)

buttonpos = plt.axes([0.85, 0.5, 0.1, 0.075])
undo_button = matplotlib.widgets.Button(buttonpos, "Undo")

This comment has been minimized.

Copy link
@jni

jni Mar 26, 2017

Contributor

You can probably make this button a bit smaller and nicer using the ↶ unicode character (21B6), which is supported in matplotlib 2.0. =) See the DejaVu Sans character coverage sheet here. There's a page for "arrows", I think this is the best choice for Undo. =)

OR, I just saw this in supplemental arrows A: ⟲, Unicode 27F2.

if image.ndim is 3:
image = np.squeeze(image[:, :, :1])

mask = np.zeros(image.shape)

This comment has been minimized.

Copy link
@jni

jni Mar 26, 2017

Contributor

Instead just say, mask = np.zeros(image.shape[:2]). Then you don't need the if statement above

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Mar 26, 2017

@pskeshu looks damn good currently. I don't have time for a full review but I've left a couple of minor comments. The unicode character is optional, but the mask shape definition should be changed. More later!

>>> camera = data.camera()
>>> mask = segmentation.manual(camera)
>>> # Use the cursor to draw objects
>>> io.imshow(mask)

This comment has been minimized.

Copy link
@jni

jni Mar 26, 2017

Contributor

These doctests fail currently. Add the string # doctest: +SKIP after mask = ... and after each following line.

@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Mar 27, 2017

Hi @jni,

Thank you once again for your input. I have addressed your previous comments. Please let me know if there is anything else that needs to be improved. :)

@soupault soupault self-assigned this Mar 27, 2017
undo_button.on_clicked(_undo)

lasso = matplotlib.widgets.LassoSelector(ax, _on_select)
plt.show()

This comment has been minimized.

Copy link
@jni

jni Mar 27, 2017

Contributor

Hey @pskeshu! I finally just tested this locally. It's awesome! =) But here, you need plt.show(block=True). Otherwise, when running the code in ipython --matplotlib, the call is non-blocking, the code continues, and the mask immediately returns empty.

>>> from skimage import data, segmentation, io
>>> camera = data.camera()
>>> mask = segmentation.manual(camera)
# doctest: +SKIP

This comment has been minimized.

Copy link
@jni

jni Mar 27, 2017

Contributor

Sorry, I wasn't clear: these need to appear at the end of the line of code, not in a new line. As in:

>>> mask = segmentation.manual(camera)  # doctest: +SKIP

(note: two spaces before #)

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Mar 27, 2017

This is so cool! @pskeshu I think we are very close.

Plus, I think it will not be too hard to make the mask "snap" to edges in the image, which would make this code extremely useful! (We don't have to do this in this PR though! ;)

@scikit-image/core Anyone else want to review? I know it's ambitious, but it would be awesome to have this in 0.13!

I suspect that this code will be hard to test, though. Any ideas?

pr = [r for r, c in verts]
pc = [c for r, c in verts]
rr, cc = polygon(pr, pc)
mask[cc, rr] = 1

This comment has been minimized.

Copy link
@jni

jni Mar 27, 2017

Contributor

@pskeshu actually, I think it would be relatively easy to return a list of masks, and it would make the function more useful (someone could paint a whole bunch of objects in one go). How about:

def _mask_from_lasso_vertices(verts, shape):
    mask = np.zeros(shape, dtype=bool)
    pr = [y for x, y in verts]
    pc = [x for x, y in verts]
    rr, cc = polygon(pr, pc, shape)
    mask[rr, cc] = 1
    return mask

(Note that I've changed the coordinate names and ordering because matplotlib uses x/y instead of row/column.)

Then:

masks = [_mask_from_lasso_vertices(verts, image.shape[:2]) for verts in list_of_verts]
return masks

This comment has been minimized.

Copy link
@jni

jni Mar 27, 2017

Contributor

Note that the above helper function definition should not be nested inside the function, but come before the definition of manual.

This comment has been minimized.

Copy link
@pskeshu

pskeshu Mar 27, 2017

Author Contributor

I have implemented an optional parameter to return a list of masks with each mask containing a separate selection, which will allow the user to draw overlapping regions and still get them all separately. Is that what you were asking for?

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Mar 27, 2017

@pskeshu @scikit-image/core Actually, scrap the idea of 0.13. I'm actually getting a lot of ideas for this manual segmentation module. It could also include a lazy snapping mode, for example (check out the examples at the end of the paper!). I don't think any of them should hold off the PR, but I want time to hash out the API before releasing it.

@pskeshu I hope that's ok with you!

@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Mar 27, 2017

@pskeshu I hope that's ok with you!

@jni I don't mind at all.

And lazy snapping looks pretty amazing, and it would definitely make this module so much more cooler and easy to use. Although I might need a lot of help from the community in trying to implement it along with this tool. :)

@pskeshu pskeshu force-pushed the pskeshu:manual-segmentation branch from a8c7661 to 78ab9b3 Mar 27, 2017
@soupault

This comment has been minimized.

Copy link
Member

soupault commented Mar 27, 2017

@jni 🤣 . I'm also very excited about this tool!
Actually, I was going to ask for some additions as well, but it appears you had taken the reins first... Will patiently wait and follow the PR. Let me know when you are done :).

What I suggest, in the meanwhile, is to think about the way we are going to advertise this feature. We could create a gallery example with the screenshots of the tool or something...

P.S. It might be a good idea to ship a simple version of this tool with 0.13.

@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Apr 12, 2017

Hi @jni ,

Thank you for the PR and your help with cleaning up the code! :)

I just encountered one problem with the implementation. When list_of_vertex_lists is empty, that is, when the user does not make any selection, reduce raises a TypeError as follows:

TypeError: reduce() of empty sequence with no initial value

I think it would be better to return an empty zeros_like array of the same size of the image if 'list_of_vertex_lists' is empty. What do you think?

@@ -117,7 +116,7 @@ def _extend_polygon(event):
# Store the vertices of the polygon as shown in preview.
# Redraw polygon and store it in polygons_drawn so that
# `_undo` works correctly.
list_of_vertex_lists.append(temp_list.copy())
list_of_vertex_lists.append(np.array(temp_list).copy())

This comment has been minimized.

Copy link
@jni

jni Apr 12, 2017

Contributor

What is the purpose of this change?

This comment has been minimized.

Copy link
@pskeshu

pskeshu Apr 13, 2017

Author Contributor

Python 2.7 doesn't have list.copy() so it throws an AttributeError.

This comment has been minimized.

Copy link
@jni

jni Apr 13, 2017

Contributor

@pskeshu ok but then there's the less readable but still preferable:

In [1]: x = [4, 5, 8, 9]

In [2]: y = x[:]

In [3]: y[2] = 1

In [4]: x
Out[4]: [4, 5, 8, 9]

In [5]: y
Out[5]: [4, 5, 1, 9]

😃

This comment has been minimized.

Copy link
@pskeshu

pskeshu Apr 13, 2017

Author Contributor

Okay, done.

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Apr 12, 2017

@pskeshu Oh yeah I encountered that error and then forgot to fix it! 😅 It's a simple fix: add an array of zeros as an extra "initial value". To save on memory, it's good to use np.broadcast_to:

return reduce(np.maximum, labels, np.broadcast_to(0, image.shape[:2]))
@jni

This comment has been minimized.

Copy link
Contributor

jni commented Apr 13, 2017

@pskeshu very cool! I'm happy with this PR. Thanks!

There is still the question of testing, but I'm personally happy to leave this for a later PR. (And this would mesh well with our recent discussion about not dragging PRs out for too long.

I've reached out to the MPL folks to ask about testing of interactive functions. Rather than wait for all the pieces to line up, I support merging this now and figuring out testing and API details in subsequent PRs.

@jni
jni approved these changes Apr 13, 2017
if return_all:
return np.stack(labels)
else:
return reduce(np.maximum, labels, np.broadcast_to(0, image.shape[:2]))

This comment has been minimized.

Copy link
@soupault

soupault Apr 13, 2017

Member
manual_lasso_segmentation(..., return_all=False) manual_polygon_segmentation(..., return_all=False)
image image

Hmm... Could you imagine the case where this kind of output is useful? In my opinion, it should rather be a intersection (binary) of partial masks. I mean it is really cool, and users can just threshold the masks, but why have an extra step?

This comment has been minimized.

Copy link
@jni

jni Apr 13, 2017

Contributor

@soupault the point is that the multi-labels are useful, not that the overlap is useful. And they are very useful: if you want to segment 10 objects in an image, for example. It's much more annoying adding the extra step of collapsing the masks, or labelling them (in which case you have to be really really careful that they don't touch), than it is to add a >0 if you need it. Which you probably don't because functions that deal with segmentations deal with integer labels. (And I think it's worth having that be consistent throughout the library.)

This comment has been minimized.

Copy link
@pskeshu

pskeshu Apr 14, 2017

Author Contributor

It is also useful if you want to segment within an object. It is particularly useful for me when I want to segment a cell, and then segment its nucleus, so I can look at the whole cell, if I want to, but I can also look at just the cytoplasm or the nucleus.
figure_1
figure_1-1

This comment has been minimized.

Copy link
@soupault

soupault Apr 14, 2017

Member

@jni Ah, I see, thanks!

"""
list_of_vertex_lists = []
polygons_drawn = []
patch_objects = []

This comment has been minimized.

Copy link
@soupault

soupault Apr 13, 2017

Member

This variable is unused.

"""
list_of_vertex_lists = []
polygons_drawn = []
patch_objects = []

This comment has been minimized.

Copy link
@soupault

soupault Apr 13, 2017

Member

This one also.

return_all : bool, optional
If True, an array containing each separate polygon drawn is returned.
(The polygons may overlap.) If False (default), later polygons

This comment has been minimized.

Copy link
@soupault

soupault Apr 13, 2017

Member

-> latter

ax.imshow(image, cmap="gray")
ax.set_axis_off()

def _undo(*args, **kwargs):

This comment has been minimized.

Copy link
@soupault

soupault Apr 13, 2017

Member

I've noticed that the undo button removes the last polygon/contour only when the mouse is back to image axes. Is it possible to fix this (for example, forcing a redraw operation at the end of this function)?

This comment has been minimized.

Copy link
@pskeshu

pskeshu Apr 13, 2017

Author Contributor

That's a good catch. Fixed it. :)

return polygon_object


def manual_polygon_segmentation(image, alpha=0.4, return_all=False):

This comment has been minimized.

Copy link
@soupault

soupault Apr 13, 2017

Member

@jni How do I [ ] Check that new features are mentioned in `doc/release/release_dev.rst`.? :D

This comment has been minimized.

Copy link
@jni

jni Apr 13, 2017

Contributor

@soupault you ask @pskeshu to add it to that file, or you merge and then you immediately submit a PR. =)

@pskeshu pskeshu force-pushed the pskeshu:manual-segmentation branch from 0a37cc3 to fc76ae7 Apr 13, 2017
Copy link
Member

soupault left a comment

One more point to discuss from my side (#2584 (comment)), otherwise is good to go!

last_poly = polygons_drawn.pop()
# ... then from the plot
last_poly.remove()
plt.draw()

This comment has been minimized.

Copy link
@tacaswell

tacaswell Apr 13, 2017

fig.canvas.draw_idle() would be better here than pyplot.

if image.ndim not in (2, 3):
raise ValueError('Only 2D grayscale or RGB images are supported.')

fig, ax = plt.subplots()

This comment has been minimized.

Copy link
@tacaswell

tacaswell Apr 13, 2017

It is better to optionally take in either fig or ax and only do this is the user does not supply one. This makes your function immediately usable by people embedding Matplotlib in larger GUI applications.

This comment has been minimized.

Copy link
@jni

jni Apr 14, 2017

Contributor

@tacaswell we use both figure and axes methods... Would you suggest we take in a (fig, ax) tuple, perhaps? The undo button placement could also be generalised for cases where the axes are only a subplot... =\ What do you think? Personally, I'm happy to leave this for a near-future PR.

This comment has been minimized.

Copy link
@tacaswell

tacaswell Apr 17, 2017

You are asking questions about a layout manager, which we have an open issue for someone to write ;)

raise ValueError('Only 2D grayscale or RGB images are supported.')

fig, ax = plt.subplots()
plt.subplots_adjust(bottom=0.2)

This comment has been minimized.

Copy link
@tacaswell

tacaswell Apr 13, 2017

This is also a Figure method, that is a better option.

last_poly.remove()
plt.draw()

undo_pos = plt.axes([0.85, 0.05, 0.075, 0.075])

This comment has been minimized.

Copy link
@tacaswell

tacaswell Apr 13, 2017

similar to above, better to use the Figure method version of this.

This comment has been minimized.

Copy link
@jni

jni Apr 13, 2017

Contributor

@tacaswell I looked for a Figure method but couldn't find it for some reason! It was probably morning. =P It's worth pointing out that all the relevant mpl gallery examples use plt.axes.

@tacaswell

This comment has been minimized.

Copy link

tacaswell commented Apr 13, 2017

This is probably better structured as an object that holds an ax and fig object + the state + callbacks and has block_for_input method with the current return values.

@JDWarner

This comment has been minimized.

Copy link
Contributor

JDWarner commented Apr 13, 2017

Completely agree with @tcaswell on the overall approach; seemingly minor tweaks but which will make this much more extensible.

Ideally those changes would happen before merge, though myself or another maintainer could do this after the fact.

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Apr 13, 2017

@tacaswell @JDWarner I generally much prefer a functional approach wherever feasible, and that's the philosophy of skimage more generally. In this case, just one closure per function gets us there. So I suggest that we stick to that until we have a compelling scenario where it becomes limiting. (And certainly within this PR.) However, passing in a Fig (and axes?) is a great idea. =)

@tacaswell

This comment has been minimized.

Copy link

tacaswell commented Apr 14, 2017

Ideally down-stream libraries should not make calls into plt at all as it gets in the way of embedding (either in GUIs or jupyter notebooks) and probably should not be making the choice to block for the user.

This style (via closures) is also awkward if you want to create a mask, get the points and run some code against it, and the go back and tweak the mask, run some code, and so on.

To a large degree 'functional' goes out the window when you start doing any sort of interactive user input ;) (the real world is annoyingly stateful).

@JDWarner

This comment has been minimized.

Copy link
Contributor

JDWarner commented Apr 14, 2017

This is one point where we should be forward-looking from the start, and @tacaswell has the right of it. In short, when working on top of Matplotlib one should always allow a Figure and/or Axis to be passed in, and then exclusively use the methods from those objects. Assume nothing else in, and people can safely build on top of your layer also.

The Matplotlib gallery is slowly being updated, and not everything is ideal style quite yet. There are a few corner cases (3D comes to mind, when last I checked) where this breaks down. Otherwise, it's really quite clear this is best practice.

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Apr 14, 2017

@tacaswell your use case is easily accommodated by an input argument including existing masks, which I wanted to do for testing anyway.

@JDWarner there's two issues here, let's not conflate them:

  1. Passing in a figure and axes and using methods from those instead of the plt state machine interface. This is a given, was not done due to a simple oversight only, and I'm happy to support it.
  2. Redesigning this whole PR to use a new class "InteractiveSegmentor" or whatever that holds everything needed. I am very much against this.
@tacaswell

This comment has been minimized.

Copy link

tacaswell commented Apr 16, 2017

Even if you take in fig or ax objects and an initial mask, as written the whole thing relies on plt.show() to block. In the context of a 'wizard' like work-flow this makes sense (python segment_my_image.py img.png etc), but it cuts off many other use cases.

With the current implementation you can not have 2 figure up with the selector at the same time and it is still not re-usable by people who are embedding into a GUI application. If you are using Matplotlib interactively (ex plt.ion() ) and you call plt.show(block=True) it will not un-block until the user closes all of the open figures (not just yours).

Providing a one-shot closure-based method is a 👍 idea, it is just a shame to not also provide the more reusable class-based version.

@jni

This comment has been minimized.

Copy link
Contributor

jni commented Apr 17, 2017

@tacaswell what is the "correct" way to get the masks back out of the MPL tools (Lasso/Polygon) without blocking? My hope is that we can come up with a simple function-based solution that can be easily used by more complex OO tools downstream.

@tacaswell

This comment has been minimized.

Copy link

tacaswell commented Apr 17, 2017

@jni I am worried we are talking past each other and are wandering into the fun world of asynchronous programming. We might do better with a phone call?

@jni jni merged commit 7a6a2e5 into scikit-image:master Apr 19, 2017
2 of 4 checks passed
2 of 4 checks passed
codecov/patch 14.56% of diff hit (target 85.46%)
Details
codecov/project 85.28% (-0.19%) compared to 4d4c799
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jni

This comment has been minimized.

Copy link
Contributor

jni commented Apr 19, 2017

@pskeshu thank you very much for your patience and for putting this awesome function together! I've discussed with @tacaswell and am writing an issue to make sure we address his concerns in future PRs. I think this will be the basis for some very cool things. Thanks again!

@jni jni mentioned this pull request Apr 19, 2017
0 of 4 tasks complete
@soupault

This comment has been minimized.

Copy link
Member

soupault commented Apr 19, 2017

@pskeshu Thank you a ton! That's an amazing contribution!

P.S. JFYI, I've already "sold" this feature to a number of people :).

@pskeshu

This comment has been minimized.

Copy link
Contributor Author

pskeshu commented Apr 19, 2017

Thank you, @jni @soupault and others for reviewing the code. This was fun! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.