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

Function for assigning labels to points #5180

Merged
merged 9 commits into from
Feb 1, 2021

Conversation

harish-vnkt
Copy link
Contributor

Description

This is a function for the feature request at #5087. It takes an array of coordinates and an image as input and returns a mask where each coordinate is assigned a unique integer label. Thanks in advance for your feedbacks. 😄

Questions/clarifications due to being a first-timer

  • General design question - The function deals with coordinates on 2 axes only, i.e, fails if coords.shape[1] != 2. Should this be extended to a general case where the coordinates can have arbitrary number of axes as long as the indexing works, i.e, passes as long as coords.shape[1] == image.ndim? @jni
  • In this function, if the coords are [44, 52], the labelling happens at row=42 and col=52. Is this correct behaviour? I opened a related issue Clarification: coordinate conventions in watershed example #5159 to clarify this for the Watershed Segmentation example

Checklist

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.

@harish-vnkt harish-vnkt changed the title Points to labels Function for assigning labels to points Jan 11, 2021
Copy link
Contributor

@grlee77 grlee77 left a comment

Choose a reason for hiding this comment

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

Thanks @harish-vnkt, this looks like a great start.

I do think we will want to make this n-dimensional, but fortunately that seems fairly easy to do (see comment).

To avoid ambiguity, we should add a Notes section to the docstring, stating that pixels are considered to fall on integer coordinate locations (starting from zero).

Other considerations are what to do with:
1.) out of range coordinates (we could either explicitly check for this or just relying on NumPy to raise an IndexError. Either way, we should document the behavior add a test case for it)
2.) negative coordinates -> explicitly disallow?
Another alternative to raising errors, could be clipping indices to fall within the image shape.

if coords.shape[-1] != 2 or coords.ndim != 2:
raise ValueError("'coords' must be of shape (n, 2)")

h, w = image.shape[:2]
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to support use of this function on multichannel and n-dimensional images, then we should probably add a multichannel keyword argument (defaulting to False). This way we don't have to try and guess based on shape if the image is multichannel. If this multichannel argument is present then, we can get the shape of the spatial dimensions using something like:

shape  = image.shape[:-1] if multichannel else image.shape

This can be moved above the shape check above so that you can check if coords.shape[-1] != len(shape) instead of assuming 2 dimensions.

Copy link
Member

Choose a reason for hiding this comment

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

Huh? We shouldn't use multichannel here, all we need is the output shape. ie don't pass in an image as an argument, only the shape.

@emmanuelle
Copy link
Member

Thank you @harish-vnkt ! In addition to @grlee77 's comments, could you also think of a gallery example which would show how to use this new function?

Comment on lines 13 to 14
image: ndarray
Image for which the mask is created
Copy link
Member

Choose a reason for hiding this comment

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

The second argument should only be a shape, not an image

Comment on lines 11 to 12
coords: ndarray
An array of 2D coordinates with shape (n, 2)
Copy link
Member

Choose a reason for hiding this comment

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

Just make this nD:

Suggested change
coords: ndarray
An array of 2D coordinates with shape (n, 2)
coords: ndarray, shape (N, D)
An array of N coordinates of dimension D.

Returns
-------
label_mask: ndarray
A 2D mask of zeroes containing unique integer labels at the `coords`
Copy link
Member

Choose a reason for hiding this comment

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

Remove 2D

Comment on lines 35 to 39
if not isinstance(coords, np.ndarray):
raise TypeError("'coords' must be an ndarray")

if coords.shape[-1] != 2 or coords.ndim != 2:
raise ValueError("'coords' must be of shape (n, 2)")
Copy link
Member

Choose a reason for hiding this comment

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

Remove these checks, instead check only that coords.shape[1] == len(output_shape), and raise a value error saying the dimensionality of the points should match the dimensionality of the image.


h, w = image.shape[:2]
np_indices = tuple(np.transpose(np.round(coords).astype(np.int)))
label_mask = np.zeros((h, w), dtype=np.uint64)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
label_mask = np.zeros((h, w), dtype=np.uint64)
label_mask = np.zeros(output_shape, dtype=np.uint64)

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

@harish-vnkt as @grlee77 said, we do want this to be nD, but it's quite a quick fix! I think between his comments and mine we can get there!

@jni
Copy link
Member

jni commented Jan 13, 2021

@emmanuelle instead of a new gallery example, we can use this function in this code cell:

https://scikit-image.org/docs/dev/auto_examples/applications/plot_human_mitosis.html#segment-nuclei

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Thank you @harish-vnkt for your contribution.
I think that the file name must be more generic and hidden: skimage/util/_label.py. This keeps door open to adding other util functions in the same vain (who knows 😉).
The label_points function may also be exposed in the skimage.util module (by adding it to skimage/util/__init__.py).

@pep8speaks
Copy link

pep8speaks commented Jan 31, 2021

Hello @harish-vnkt! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-02-01 21:15:01 UTC

@harish-vnkt
Copy link
Contributor Author

Hi guys, sorry for the late updates. I have accounted for all of the changes mentioned in the reviews. I have gone with @jni's suggestion for the output shape argument and updated the code and tests accordingly.

Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Thank you @harish-vnkt, I left some comments, this is pretty close ;)

skimage/util/_label.py Outdated Show resolved Hide resolved
skimage/util/_label.py Outdated Show resolved Hide resolved
skimage/util/_label.py Outdated Show resolved Hide resolved
skimage/util/_label.py Outdated Show resolved Hide resolved
…to labels, set copy=False when converting `coords` to int
Copy link
Member

@rfezzani rfezzani left a comment

Choose a reason for hiding this comment

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

Thank you @harish-vnkt, I left some minor corrections to PEP8, otherwise it looks good for me 🎉


Notes
-----
- The labels are assigned to coordinates that are converted to integer and considered to start from 0.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- The labels are assigned to coordinates that are converted to integer and considered to start from 0.
- The labels are assigned to coordinates that are converted to
integer and considered to start from 0.

- Negative coordinates raise a ValueError
"""
if coords.shape[1] != len(output_shape):
raise ValueError("Dimensionality of points should match the output shape")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
raise ValueError("Dimensionality of points should match the output shape")
raise ValueError("Dimensionality of points should match the "
"output shape")

if np.any(coords < 0):
raise ValueError("Coordinates should be positive and start from 0")

np_indices = tuple(np.transpose(np.round(coords).astype(np.int, copy=False)))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
np_indices = tuple(np.transpose(np.round(coords).astype(np.int, copy=False)))
np_indices = tuple(np.transpose(np.round(coords).astype(np.int,
copy=False)))

Copy link
Member

@jni jni left a comment

Choose a reason for hiding this comment

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

Awesome! This looks ready to me. Thank you @harish-vnkt!

@jni jni dismissed grlee77’s stale review February 1, 2021 23:08

contributor made requested changes

@jni jni merged commit 7c3c24f into scikit-image:master Feb 1, 2021
@harish-vnkt harish-vnkt deleted the points-to-labels branch February 1, 2021 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants