Fix several issues with relabel_sequential #3740

uschmidt83 · 2019-02-10T22:22:57Z

Description

Fixing several issues with skimage.segmentation.relabel_sequential (see added tests).
Slightly changing the semantic of the function by providing data type stability (if posssible).

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Benchmark in ./benchmarks, if your changes aren't covered by an
existing benchmark
Unit tests
Clean style in the spirit of PEP8

For reviewers

Check that the PR title is short, concise, and will make sense 1 year
later.
Check that new functions are imported in corresponding __init__.py.
Check that new features, API changes, and deprecations are mentioned in
doc/release/release_dev.rst.
Consider backporting the PR with @meeseeksdev backport to v0.14.x

…l-fixes

pep8speaks · 2019-02-10T22:22:59Z

Hello @uschmidt83! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on February 11, 2019 at 08:29 Hours UTC

skimage/segmentation/_join.py

skimage/segmentation/tests/test_join.py

hmaarrfk · 2019-02-10T23:40:41Z

skimage/segmentation/_join.py

        new_type = np.min_scalar_type(int(m))
        label_field = label_field.astype(new_type)
        m = m.astype(new_type)  # Ensures m is an integer
    labels = np.unique(label_field)
    labels0 = labels[labels != 0]
-    if m == len(labels0):  # nothing to do, already 1...n labels
+    required_type = np.min_scalar_type(offset + len(labels0))


min_scalar_type is a little annoying. It can return unsigned types. I think I've personally come around to not liking the use of "unsigned" unless you need specific "unsigned" behavior, that is, 255+2 == 1 is something you want to be true in your math.

Thoughts?

The whole required_type thing is really covering an edge case that I don't think will happen much in practice, unless someone purposefully uses restricted data types (especially uint8).

I frequently save label masks to disk as uint16 TIFF files. Hence, they have this type when loaded again from disk. I like working with 16-bit integers for space reasons, which can be important for large 3D arrays.

Want to allow a dtype parameter to ensure uint16???

I think the new logic (use the larger of min_dtype and input dtype) is sufficient here.

Co-Authored-By: uschmidt83 <uschmidt83@users.noreply.github.com>

hmaarrfk · 2019-02-10T23:49:16Z

skimage/segmentation/_join.py

@@ -114,20 +116,29 @@ def relabel_sequential(label_field, offset=1):
    >>> relab
    array([5, 5, 6, 6, 7, 9, 8])
    """
+    offset = int(offset)
+    if offset <= 0:
+        raise ValueError("Offset must be strictly positive.")


Nice one, yeah the error message was non-sensical before

In [14]: relabel_sequential(np.asarray([2, 3, 4]), -1) --------------------------------------------------------------------------- ValueError Traceback (most recent call last) <ipython-input-14-e5025e3e4264> in <module> ----> 1 relabel_sequential(np.asarray([2, 3, 4]), -1) ~/miniconda3/envs/owl/lib/python3.7/site-packages/skimage/segmentation/_join.py in relabel_sequential(label_field, offset) 129 labels = np.concatenate(([0], labels)) 130 inverse_map = np.zeros(offset - 1 + len(labels), dtype=np.intp) --> 131 inverse_map[(offset - 1):] = labels 132 relabeled = forward_map[label_field] 133 return relabeled, forward_map, inverse_map ValueError: could not broadcast input array from shape (4) into shape (2)

jni

@uschmidt83 thank you, very nice! I have one nitpicky suggestion and one bigger one regarding negative values, but imho that can happen in a separate PR if necessary. This is already a significant improvement.

skimage/segmentation/tests/test_join.py

jni · 2019-02-11T00:35:08Z

skimage/segmentation/tests/test_join.py

+        ar = np.array([1, 3, 2, 5, 4])
+    ar_relab, fw, inv = relabel_sequential(ar, offset=offset)
+    ar_relab_ref = ar.copy()
+    ar_relab_ref[ar_relab_ref > 0] += offset - 1


alternatively, ar_relab_ref = np.where(ar > 0, ar + offset - 1, 0)

Sure, that's better.

Should I change and commit this?

Yeah go for it!

jni · 2019-02-11T00:37:17Z

skimage/segmentation/_join.py

@@ -114,20 +116,29 @@ def relabel_sequential(label_field, offset=1):
    >>> relab
    array([5, 5, 6, 6, 7, 9, 8])
    """
+    offset = int(offset)
+    if offset <= 0:
+        raise ValueError("Offset must be strictly positive.")


skimage/segmentation/_join.py

jni · 2019-02-11T00:39:45Z

skimage/segmentation/_join.py

-    if m == len(labels0):  # nothing to do, already 1...n labels
+    required_type = np.min_scalar_type(offset + len(labels0))
+    if np.dtype(required_type).itemsize > np.dtype(label_field.dtype).itemsize:
+        label_field = label_field.astype(required_type)


uschmidt83 · 2019-02-11T08:09:03Z

skimage/segmentation/_join.py

        return label_field, labels, labels
-    forward_map = np.zeros(m + 1, int)
-    forward_map[labels0] = np.arange(offset, offset + len(labels0))
+    forward_map = np.zeros(int(m + 1), dtype=label_field.dtype)


Btw, I replaced m + 1 with int(m + 1) to fix an issue when m is of type np.uint64.

Is this intended behavior or a numpy bug?

>>> (np.uint64(5) + 1).dtype dtype('float64')

yikes! I think actually I came across this and the answer was that it is indeed intended, because (a) NumPy's type promotion rules do not depend on the input values, only on the types, and (b) they are supposed to be safe in the sense of being able to represent the result, and float64 is the only thing that can represent anything with a Python int, since they are unbounded.

At least, that's my memory of it. Perhaps @stefanv has more details. But, either way, thanks for the fix!

What is this sorcery!!!!

Looks like the argument Juan outlines is being followed here. Counter-intuitive, but probably correct? Same as (np.uint8(3)/2).dtype.

Co-Authored-By: uschmidt83 <uschmidt83@users.noreply.github.com>

uschmidt83 · 2019-02-11T21:22:50Z

skimage/segmentation/_join.py

        new_type = np.min_scalar_type(int(m))
        label_field = label_field.astype(new_type)
        m = m.astype(new_type)  # Ensures m is an integer
    labels = np.unique(label_field)
    labels0 = labels[labels != 0]
-    if m == len(labels0):  # nothing to do, already 1...n labels


Btw, the previous line

if m == len(labels0): # nothing to do, already 1...n labels

was only valid for offset = 1. This was only mentioned in the comment but not checked, i.e. it should've been

if offset == 1 and m == len(labels0): # nothing to do, already 1...n labels

Anyway, I guess hardly anyone uses offset != 1. Why was the previous function relabel_from_one replaced with relabel_sequential? (Since the offset introduces quite a bit of (subtle) complexity.)

Also, relabel_sequential assumes that the background has value 0, but I noticed color.label2rgb assumes (by default) that the background label has value -1.

Why was the previous function relabel_from_one replaced with relabel_sequential?

Someone requested the functionality and it seemed like the right thing to do and an easy fix. Oops. =)

Also, relabel_sequential assumes that the background has value 0, but I noticed color.label2rgb assumes (by default) that the background label has value -1.

Yes, historically, skimage used -1 as the background label, but we have slowly started homogenising to 0. But this will take time.

Good to know, thanks!

jni · 2019-02-15T08:38:12Z

@scikit-image/core anyone else want to review this? The Travis failures are due to the Qt 5.12 bug that was fixed on master.

uschmidt83 · 2019-04-04T07:16:40Z

Anything else I can do to get this merged?

stefanv · 2019-04-04T07:40:26Z

skimage/segmentation/_join.py

        return label_field, labels, labels
-    forward_map = np.zeros(m + 1, int)
-    forward_map[labels0] = np.arange(offset, offset + len(labels0))
+    forward_map = np.zeros(int(m + 1), dtype=label_field.dtype)


Looks like the argument Juan outlines is being followed here. Counter-intuitive, but probably correct? Same as (np.uint8(3)/2).dtype.

jni · 2019-04-04T15:28:44Z

Thanks for the ping @uschmidt83!

uschmidt83 added 2 commits February 10, 2019 23:10

Fix several issues with relabel_sequential

cd934f0

Merge remote-tracking branch 'upstream/master' into relabel_sequentia…

5307a3c

…l-fixes

uschmidt83 changed the title ~~Fixing several issues with relabel_sequential~~ Fix several issues with relabel_sequential Feb 10, 2019

pep8

995cfca

hmaarrfk reviewed Feb 10, 2019

View reviewed changes

skimage/segmentation/_join.py Outdated Show resolved Hide resolved

hmaarrfk reviewed Feb 10, 2019

View reviewed changes

skimage/segmentation/tests/test_join.py Outdated Show resolved Hide resolved

hmaarrfk reviewed Feb 10, 2019

View reviewed changes

Update skimage/segmentation/tests/test_join.py

16c771e

Co-Authored-By: uschmidt83 <uschmidt83@users.noreply.github.com>

hmaarrfk reviewed Feb 10, 2019

View reviewed changes

jni approved these changes Feb 11, 2019

View reviewed changes

uschmidt83 commented Feb 11, 2019

View reviewed changes

uschmidt83 and others added 2 commits February 11, 2019 09:28

update tests

eb2d299

Update skimage/segmentation/_join.py

f1c05f2

Co-Authored-By: uschmidt83 <uschmidt83@users.noreply.github.com>

uschmidt83 commented Feb 11, 2019

View reviewed changes

stefanv approved these changes Apr 4, 2019

View reviewed changes

stefanv merged commit fe94ee9 into scikit-image:master Apr 4, 2019

uschmidt83 mentioned this pull request Feb 19, 2020

Fix edgecase bugs in segmentation.relabel_sequential #4465

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix several issues with relabel_sequential #3740

Fix several issues with relabel_sequential #3740

uschmidt83 commented Feb 10, 2019

pep8speaks commented Feb 10, 2019 •

edited

hmaarrfk Feb 10, 2019

uschmidt83 Feb 10, 2019

hmaarrfk Feb 11, 2019

jni Feb 11, 2019

hmaarrfk Feb 11, 2019

hmaarrfk Feb 10, 2019

jni Feb 11, 2019

jni left a comment

jni Feb 11, 2019

uschmidt83 Feb 11, 2019

uschmidt83 Feb 11, 2019

jni Feb 11, 2019

uschmidt83 Feb 11, 2019

jni Feb 11, 2019

jni Feb 11, 2019

uschmidt83 Feb 11, 2019

jni Feb 11, 2019

hmaarrfk Feb 11, 2019

stefanv Apr 4, 2019

uschmidt83 Feb 11, 2019

jni Feb 12, 2019

uschmidt83 Feb 12, 2019

jni commented Feb 15, 2019

uschmidt83 commented Apr 4, 2019

stefanv Apr 4, 2019

jni commented Apr 4, 2019

Fix several issues with relabel_sequential #3740

Fix several issues with relabel_sequential #3740

Conversation

uschmidt83 commented Feb 10, 2019

Description

Checklist

For reviewers

pep8speaks commented Feb 10, 2019 • edited

Comment last updated on February 11, 2019 at 08:29 Hours UTC

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

jni left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jni commented Feb 15, 2019

uschmidt83 commented Apr 4, 2019

Choose a reason for hiding this comment

jni commented Apr 4, 2019

pep8speaks commented Feb 10, 2019 •

edited