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

Support nD images and labels in label2rgb #5550

Merged

Conversation

Abhinavmishra8960
Copy link
Contributor

@Abhinavmishra8960 Abhinavmishra8960 commented Aug 30, 2021

#5501

As label2rgb do not support dimension greater than 2. Added Correct error condition and message.
sci
Changed code image

Please do mention changes required.

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.

@pep8speaks
Copy link

pep8speaks commented Aug 30, 2021

Hello @Abhinavmishra8960! 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-09-10 17:10:49 UTC

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 @Abhinavmishra8960. What you have here looks okay, but we will be better off making this function support nD. Please see my suggestion on how we can do this with very little work!

After applying that change this would need:

1.) Update the PR title to something like: Add nD support to label2rgb
2.) Add a new test case to skimage/color/tests/test_colorlabel.py with 3D (maybe 1D as well) input.
3.) Update the Parameters section of the docstring in this function to remove reference to "2D".

skimage/color/colorlabel.py Outdated Show resolved Hide resolved
@alexdesiqueira alexdesiqueira added the 🔧 type: Maintenance Refactoring and maintenance of internals label Aug 30, 2021
@Abhinavmishra8960 Abhinavmishra8960 changed the title Corrected error check for image greater than 2D Extended support for nD image, label2rgb Aug 31, 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.

One of my prior suggestions caused failures when image was grayscale (:frowning_face:). The new suggestions here fix that and expand the test cases.

skimage/color/colorlabel.py Outdated Show resolved Hide resolved
skimage/color/colorlabel.py Outdated Show resolved Hide resolved
skimage/color/tests/test_colorlabel.py Outdated Show resolved Hide resolved
@grlee77 grlee77 changed the title Extended support for nD image, label2rgb Support nD images and labels in label2rgb Sep 2, 2021
@grlee77 grlee77 added ⏩ type: Enhancement Improve existing features and removed 🔧 type: Maintenance Refactoring and maintenance of internals labels Sep 2, 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.

I had to push one fix to the suggested test case. When I took the 1D slice, it was originally only passing through one of the two labels, which resulted in a mismatch in the label color. I updated the label map used in the tests to have both label values intersect the 1D slice.

Assuming the tests pass now, this is ready to go.

Integer array of labels with the same shape as `image`.
image : array, shape (M, N) or (..., 3, ...), optional
image : array, shape (M, N) or (M, N, ...., 3), optional
Copy link
Member

Choose a reason for hiding this comment

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

Seeing this change, I wonder if shouldn't add the support for the channel_axis argument...

Copy link
Contributor

Choose a reason for hiding this comment

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

This function does have channel_axis already! I think the question is how to best indicate that labels is "nD spatial" while image is either "nD spatial" or "nD spatial with channels"

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that the docstring as currently written implies that channels are in the last position if present, which may be misleading

Copy link
Member

Choose a reason for hiding this comment

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

I suggest to remove the shapes from the label and image variables type and specify in the description that image should have the same shape as label with an optional additional RGB channel axis...

Copy link
Contributor

Choose a reason for hiding this comment

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

a pushed a commit with the suggested update. please review

@alexdesiqueira alexdesiqueira merged commit 8faf62d into scikit-image:main Sep 10, 2021
@alexdesiqueira
Copy link
Member

Thank you @Abhinavmishra8960 @grlee77!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⏩ type: Enhancement Improve existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants