Seeds segmentation #1557

Britefury · 2015-06-14T17:38:07Z

I've knocked together an implementation of the SEEDS algorithm - or perhaps - an attempt at one. It produces colour based segmentations much like SLIC, except that it does not show the regular grid structure that is seen in the literature.
Much of my reference was based on David Stutz's implementation and to a lesser extent an implementation contributed to OpenCV. I hope my implementation is correct; I apologise if it isn't. If it is - with any luck - it can be used as a starting point for something better.

JDWarner · 2015-06-16T01:54:35Z

Thanks for your contribution, @Britefury! This looks really interesting.

Our Travis continuous integration build is having trouble because it uses Bento. The new Cython file needs to be added to the bento.info file at package root.

I'll do a full review once the CI builds are happy.

Britefury · 2015-06-21T11:14:02Z

Whew. That took a while to get right! :)

jni · 2015-06-22T04:14:22Z

skimage/segmentation/_seeds.pyx

+        self.w = metrics.w
+        self.block_h = metrics.block_h * 2
+        self.block_w = metrics.block_w * 2
+        self.n_blocks_y = metrics.n_blocks_y // 2


These need to be updated to use r/c instead of y/x. See our coordinate conventions guide. (This is a new standard, which is why you might see some xyz in the code still. But we aim to transition to r/c completely.)

jni · 2015-06-22T05:34:41Z

@Britefury thanks for submitting this! I hadn't heard of SEEDS and it looks to me like a great algorithm, worthy of inclusion in scikit-image.

That's the good news. The bad news is that I think the implementation will need a lot of refinement before getting merged. Functions like refine_blocks, refine_pixels, and seeds are huge monoliths that will be very difficult to maintain in the future.

A lot of the code can be simplified using scipy and numpy functions. In fact, from what I've seen, the implementation can be made n-dimensional and the code simplified, to boot. For example, instead of your 2D histogram downsampling function, you can use numpy.add.reduceat to downsample a histogram of any dimension:

import numpy as np
# 2D histogram for 9 x 16 image, 3 bins
h = np.random.rand(9, 16, 3)
hs = h
for ax in range(hs.ndim - 1):
    hs = np.add.reduceat(hs, np.arange(0, hs.shape[ax] - 1, 2), axis=ax)

hs will hold the downsampled histogram.

jni · 2015-06-22T05:39:57Z

Also, we tend to prefer a functional approaches here... The LevelMetrics class doesn't seem to add much functionality, so e.g. a dictionary might be more appropriate.

We're definitely willing to iterate with you on this! I don't want to discourage you! But it's going to take a few tries to get right, I think.

PEP8 fixes. _relabel_check* functions renamed to _relabel_will_disconnect*. Improved docstring of _relabel_check.

Britefury · 2015-08-16T08:32:45Z

@jni I benchmarked upscale_labels_by_2 vs ndimage.zoom. Given a 256x256 input image, dtype=int, upscale_labels_by_2 takes 1.34ms while ndimage.zoom takes 4.3ms.
Is performance an adequate reason to keep upscale_labels_by_2 around?

As for downscale_histogram vs the numpy based solution you provided, given 256x256x32 input histogram, downscale_histogram: 2ms, np.reduceat based solution: 30ms.

Admittedly, these functions that I have just benchmarked may not be on the critical path.

As for the LevelMetrics class, I used it in preference to a dictionary since I figured that fields on a Cython class would have less overhead than those in a dict.

ahojnnes · 2015-08-16T16:34:48Z

What's the overall runtime for a 256x256 input image?

JDWarner · 2015-08-16T20:49:57Z

We also need to know how they scale. 256, 512, and 1024 square benchmarks for both would be ideal.

Britefury · 2015-08-18T22:57:49Z

@JDWarner Have found a compromise: it seems that applying numpy.repeat a couple of times is even faster than the hand-rolled solution. Part of the original upscale_labels_by_2 implementation remains in order to repeat the last row/column when needed. upscale_labels_by_2 has been moved from _seeds.pyx to seeds_superpixels.py.

Some quick tests show that all 3 approaches (original hand-rolled, numpy.repeat and ndi.zoom) scale linearly w.r.t. number of pixels.

sciunto · 2016-08-21T09:43:27Z

This feature looks cool. What's the status of this PR guys?

alexdesiqueira · 2019-08-02T18:52:22Z

Hi @scikit-image/core,
is this something we still want in? Thanks!

jni · 2019-08-03T04:07:48Z

@alexandrejaguar if we can make it ND I'm all for it! =) Thank you for all your work on picking up dropped PRs, it's wonderful to see!

hmaarrfk · 2019-08-03T13:25:13Z

@alexandrejaguar if we can make it ND I'm all for it! =) Thank you for all your work on picking up dropped PRs, it's wonderful to see!

I think this ND requirements really need to be discussed.

jni · 2019-08-04T05:34:49Z

I think this ND requirements really need to be discussed.

You are right. I am firm in my assessment, though, but could be overruled... Assuming we get governance #3585 through soon! =\

Please see recent commits in #3544 for work I'm doing getting our transforms to support 3D/nD data. If we aim to be "maximally" nD compliant, which I think we do (as nD is important to a growing proportion of our users), then merging before code is nD amounts to taking on technical debt, of which we already have too much imho.

Part of this can be fixed with labeling function names as _2d(). That at least makes clear to a user interested in our 3D processing that they shouldn't even look at those functions. But imho in many cases, including this one, the code can be written as nD up-front with not too much extra effort, and that should be done before merging, because then it remains on a to-do list.

If we had users banging on the door asking for SEEDS in skimage, I'd be inclined to be more flexible, but 2D SEEDS is already available in OpenCV. What can we bring to the table? As someone who teaches git and open source contribution using GitHub on a regular basis, I very much sympathise with the “all contributions are good, let's have more of them!” mindset, but for a project like scikit-image, it needs to be balanced with maintenance cost. We should push to get contributions in if they meet a certain level of utility. imho, nD SEEDS would meet that threshold, while 2D SEEDS is borderline.

hmaarrfk · 2019-08-04T16:46:46Z

Gah. Sorry @jni, didn't see this when I sent out the email on the smiling list, should we move the discussion to an issue on GitHub?

jni · 2019-08-05T00:43:03Z

The smiling list! We should rename it that. =P

I think this is worth discussing in both places, as they reach different audiences. Your point about the API is a very good one.

hmaarrfk · 2019-08-05T00:45:42Z

skimage/segmentation/_seeds.pyx

+    cdef int[:,:] up_labels_2d = np.zeros((metrics.h, metrics.w), dtype=np.int32)
+    cdef int c, r, i, j, block_w, block_h
+
+    for r in range(metrics.n_blocks_r):


can you release the GIL here?

hmaarrfk · 2019-08-05T00:46:15Z

skimage/segmentation/_seeds.pyx

+    cdef int[:,:,:] hist_2d = np.zeros((metrics.n_blocks_r, metrics.n_blocks_c,
+                                        hist_size), dtype=np.int32)
+    cdef int c, r, i, j, block_h, block_w, n
+    for r in range(metrics.n_blocks_r):


can ou release the GIL here?

hmaarrfk · 2019-08-05T00:46:30Z

skimage/segmentation/_seeds.pyx

+    cdef int c, r, i
+    cdef int rem_r = hist_2d.shape[0] & 1
+    cdef int rem_c = hist_2d.shape[1] & 1
+    for r in range(h2):


can you release the GIL here?

hmaarrfk · 2019-08-05T00:46:49Z

skimage/segmentation/_seeds.pyx

+    cdef double l0n = 0.0, l1n = 0.0, bn = 0.0, l0int=0.0, l1int=0.0;
+    cdef int i
+
+    if label_0 != label_1:


can you release the GIL here?

hmaarrfk · 2019-08-05T00:47:44Z

skimage/segmentation/_seeds.pyx

+
+@cython.boundscheck(False)
+cdef bint _relabel_will_disconnect(int a0, int a1, int b0, int b1,
+                                   int c0, int c1):


should be declared nogil

hmaarrfk · 2019-08-05T00:49:04Z

skimage/segmentation/_seeds.pyx

+
+@cython.boundscheck(False)
+cdef bint relabel_will_disconnect_above(int[:,:] labels_2d, int r, int c,
+                                        int h, int w):


are these internal function? if so, these little C helper routines should be declared nogil

hmaarrfk · 2019-08-05T00:49:25Z

skimage/segmentation/_seeds.pyx

+
+
+@cython.boundscheck(False)
+cdef bint relabel_will_disconnect_above(int[:,:] labels_2d, int r, int c,


should lablels_2d be declared contiguous? int [:, ::1] labels_2d????

hmaarrfk · 2019-08-05T00:49:57Z

skimage/segmentation/_seeds.pyx

+    cdef int refine_it, r, c
+
+    # For each refinement iteration
+    for refine_it in range(refine_iters):


should be nogil

hmaarrfk · 2019-08-05T00:50:41Z

skimage/segmentation/_seeds.pyx

+    cdef int refine_it, r, c
+
+    # For each refinement iteration
+    for refine_it in range(refine_iters):


hmaarrfk · 2019-08-05T00:51:22Z

skimage/segmentation/seeds_superpixels.py

+           Proceedings of the European Conference on Computer Vision,
+           pages 13-26, 2012.
+    """
+    assert n_levels >= 1


These assert statements should be replaced by real if statements that raising meaningful error messages

hmaarrfk · 2019-08-05T00:52:31Z

skimage/segmentation/seeds_superpixels.py

+    seed_h = max(int(block_finest_h_f+0.5), 1)
+
+    # Get an index image; determine what to do.
+    if len(input_2d.shape) == 3 and input_2d.shape[2] == 3:


We've been working against this kind of "magic". Not too sure what to do in this case, but I'm just flagging it for now. We have been going with the multichannel keyword argument asking users to explicitely label their images.

hmaarrfk · 2019-08-05T00:52:59Z

skimage/segmentation/seeds_superpixels.py

+                                              iter=50)
+        index_2d = index_1d.reshape(input_2d.shape[:2])
+    elif len(input_2d.shape) == 2:
+        if input_2d.dtype == float or input_2d.dtype == np.float32:


you can check issubtype

hmaarrfk · 2019-08-05T00:56:00Z

skimage/segmentation/tests/test_seeds.py

+    assert_equal(labels[:16, 16:], 2)
+    assert_equal(labels[16:, 16:], 4)
+
+if __name__ == '__main__':


this is no longer necessary. Pytest will just find the test above.

hmaarrfk · 2019-08-05T00:56:46Z

skimage/segmentation/tests/test_seeds.py

+
+
+@test_parallel()
+def test_color_2d():


You take in so many different parameters, and check for all of the. You should really test many more keyword argument and "bad input" cases.

Especially since you take in and special case many different types of dtypes.

hmaarrfk

Didn't really dwelve too much into the code, but at first glance:

Try to release the GIL. Not sure if it is always possible.
If you are special casing dtypes, then you should test for all the dtypes you consider.

Britefury added 2 commits June 14, 2015 18:19

Implementation of SEEDS superpixel segmentation algorithm.

582e1f7

Added SEEDS example to plot_segmentation.

40fd791

Britefury and others added 9 commits June 18, 2015 20:33

Added SEEDS to bento.info

2815672

dtype fix and docstring fixes.

b4a326d

dtype fixes.

ee93954

Further dtype fixes.

fa837bd

type fix again.

a2c43d1

dtype fixes yet again.

f28835f

Added unit tests for SEEDS segmentation.

2e60393

Python 3 fix.

27d9225

Python 3 fix; xrange -> range.

8924947

jni reviewed Jun 22, 2015
View reviewed changes

Britefury added 3 commits July 2, 2015 20:44

y,x -> r,c convention compliance.

3c0c122

PEP8 fixes. _relabel_check* functions renamed to _relabel_will_disconnect*. Improved docstring of _relabel_check.

Merge changes from different clones.

3bc3a05

Some more y,x -> r,c convention fixes, along with a docstring style fix.

6724ab7

soupault added the new feature label Feb 6, 2016

soupault added the 🧐 Needs review label Jun 7, 2016

hmaarrfk reviewed Aug 5, 2019

View reviewed changes

Base automatically changed from master to main February 18, 2021 18:22

mkcor mentioned this pull request Feb 22, 2021

2021's calendar of community management #5169

Closed

grlee77 added 🙏 Feature request and removed type: new feature labels Feb 22, 2022

mkcor mentioned this pull request Feb 28, 2022

2022's calendar of community management #6165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seeds segmentation #1557

Seeds segmentation #1557

Britefury commented Jun 14, 2015

JDWarner commented Jun 16, 2015

Britefury commented Jun 21, 2015

jni Jun 22, 2015

Britefury Jul 2, 2015

jni commented Jun 22, 2015

jni commented Jun 22, 2015

Britefury commented Aug 16, 2015

ahojnnes commented Aug 16, 2015 via email

JDWarner commented Aug 16, 2015

Britefury commented Aug 18, 2015

sciunto commented Aug 21, 2016

alexdesiqueira commented Aug 2, 2019

jni commented Aug 3, 2019

hmaarrfk commented Aug 3, 2019

jni commented Aug 4, 2019

hmaarrfk commented Aug 4, 2019

jni commented Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk Aug 5, 2019

hmaarrfk left a comment



		@cython.boundscheck(False)
		cdef bint relabel_will_disconnect_above(int[:,:] labels_2d, int r, int c,



		@test_parallel()
		def test_color_2d():

Seeds segmentation #1557

Are you sure you want to change the base?

Seeds segmentation #1557

Conversation

Britefury commented Jun 14, 2015

JDWarner commented Jun 16, 2015

Britefury commented Jun 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni commented Jun 22, 2015

jni commented Jun 22, 2015

Britefury commented Aug 16, 2015

ahojnnes commented Aug 16, 2015 via email

JDWarner commented Aug 16, 2015

Britefury commented Aug 18, 2015

sciunto commented Aug 21, 2016

alexdesiqueira commented Aug 2, 2019

jni commented Aug 3, 2019

hmaarrfk commented Aug 3, 2019

jni commented Aug 4, 2019

hmaarrfk commented Aug 4, 2019

jni commented Aug 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmaarrfk left a comment

Choose a reason for hiding this comment