Add spacing to regionprops and moments. #6296

tibuch · 2022-03-21T16:37:23Z

Description

Enhancement, closes #5112.

Checklist

Docstrings for all functions
Gallery example in ./doc/examples (new features only)
Changes are covered by an existing benchmark
Unit tests
Clean style in the spirit of PEP8
Descriptive commit messages (see below)

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.
There is a bot to help automate backporting a PR to an older branch. For
example, to backport to v0.19.x after merging, add the following in a PR
comment: @meeseeksdev backport to v0.19.x
To run benchmarks on a PR, add the run-benchmark label. To rerun, the label
can be removed and then added again. The benchmark output can be checked in
the "Actions" tab.

pep8speaks · 2022-03-21T16:37:27Z

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

In the file skimage/measure/_moments.py:

Line 195:80: E501 line too long (82 > 79 characters)
Line 255:17: E126 continuation line over-indented for hanging indent
Line 255:80: E501 line too long (85 > 79 characters)
Line 320:80: E501 line too long (99 > 79 characters)
Line 391:80: E501 line too long (82 > 79 characters)
Line 428:80: E501 line too long (96 > 79 characters)

In the file skimage/measure/_regionprops.py:

Line 306:80: E501 line too long (85 > 79 characters)
Line 546:80: E501 line too long (83 > 79 characters)
Line 553:80: E501 line too long (90 > 79 characters)
Line 560:80: E501 line too long (84 > 79 characters)
Line 585:80: E501 line too long (85 > 79 characters)
Line 592:80: E501 line too long (85 > 79 characters)
Line 622:80: E501 line too long (80 > 79 characters)
Line 638:80: E501 line too long (85 > 79 characters)
Line 644:80: E501 line too long (90 > 79 characters)
Line 651:80: E501 line too long (84 > 79 characters)
Line 866:80: E501 line too long (86 > 79 characters)
Line 1002:80: E501 line too long (90 > 79 characters)
Line 1017:80: E501 line too long (94 > 79 characters)
Line 1093:80: E501 line too long (84 > 79 characters)
Line 1095:80: E501 line too long (92 > 79 characters)
Line 1323:80: E501 line too long (91 > 79 characters)

In the file skimage/measure/tests/test_moments.py:

Line 27:1: E302 expected 2 blank lines, found 1

In the file skimage/measure/tests/test_regionprops.py:

Line 50:80: E501 line too long (94 > 79 characters)
Line 56:80: E501 line too long (86 > 79 characters)
Line 57:80: E501 line too long (82 > 79 characters)
Line 58:80: E501 line too long (97 > 79 characters)
Line 65:80: E501 line too long (94 > 79 characters)
Line 143:80: E501 line too long (85 > 79 characters)
Line 150:80: E501 line too long (95 > 79 characters)
Line 153:80: E501 line too long (80 > 79 characters)
Line 156:61: W504 line break after binary operator
Line 165:80: E501 line too long (91 > 79 characters)
Line 166:80: E501 line too long (90 > 79 characters)
Line 168:80: E501 line too long (94 > 79 characters)
Line 170:80: E501 line too long (83 > 79 characters)
Line 173:38: W504 line break after binary operator
Line 174:38: W504 line break after binary operator
Line 177:38: W504 line break after binary operator
Line 178:38: W504 line break after binary operator
Line 181:38: W504 line break after binary operator
Line 182:38: W504 line break after binary operator
Line 209:80: E501 line too long (81 > 79 characters)
Line 211:80: E501 line too long (81 > 79 characters)
Line 661:80: E501 line too long (88 > 79 characters)
Line 778:80: E501 line too long (106 > 79 characters)
Line 786:80: E501 line too long (106 > 79 characters)
Line 873:80: E501 line too long (111 > 79 characters)

Comment last updated at 2022-04-04 07:12:59 UTC

grlee77 · 2022-03-21T19:26:34Z

Thanks @tibuch, we are very keen to have this functionality. I will try to review this soon

jni

@tibuch awesome! Thank you for tackling this, apologies that I never got round to it! 😅 I really appreciate it. I have made a couple of suggestions, most are nitpicky and I would consider optional for approval, but the ones I really care about are adding indices or coords_scaled as a property (since coords are in many cases used for indexing so you don't want the scaling), and removing pixel_area as a property since it is a property of the image, not the region, and trivial to compute outside of the function.

Thank you again!

skimage/measure/_regionprops.py

jni · 2022-03-22T10:11:23Z

skimage/measure/_regionprops.py

+                'Non-integer image types are ambiguous: '
+                'use skimage.measure.label to label the connected'
+                'components of label_image,'
+                'or label_image.astype(np.uint8) to interpret'
+                'the True values as a single label.')
        else:
            raise TypeError(
-                    'Non-integer label_image types are ambiguous')
+                'Non-integer label_image types are ambiguous')


Do you mind reverting unrelated formatting changes? They will make git-blame harder to follow.

I hope I caught all of them 😇

jni · 2022-03-22T10:15:13Z

skimage/measure/tests/test_regionprops.py

@@ -198,51 +338,75 @@ def test_coordinates():
    prop_coords = regionprops(sample)[0].coords
    assert_array_equal(prop_coords, coords)

+    spacing = (1, 0.5)
+    prop_coords = regionprops(sample, spacing=spacing)[0].coords
+    assert_array_equal(prop_coords, coords * np.array(spacing))


For all of these tests, I'd prefer if there was a separate test function for unit spacing, isotropic spacing, and anisotropic spacing, perhaps using pytest.parametrize? It makes it much easier to find failing tests in the future...

Makes sense.

You will also notice that not all features are tested with anisotropy, because I could not find reference numbers. Do you know of some implementations that could be used?

I don't. Typically I would try to hand-check the numbers (going step by step in the implementation) and then save those as a reference. Or you can derive some from first principles, for example, drawing an ellipse with a particular orientation using skimage.draw, then taking away every other row of pixels and using spacing=(2, 1) and making sure that the results are close to the isotropic case.

jni

I'm approving, but will let this sit for a few days to give others a chance to review and @tibuch a chance to investigate some of the additional tests if possible. Thank you again @tibuch!

grlee77

Thanks @tibuch, this is looking pretty good. I just had some small fixes and suggestions.

skimage/measure/_regionprops.py

skimage/measure/_moments.py

skimage/measure/_regionprops.py

…tation.

tibuch · 2022-03-31T12:06:48Z

I implemented the requested changes.

Additionally I found that the normalized moments were wrong. Could you have a close look at this commit: 41322934dcfbb5db74b98e369d8264997465a43a

Thanks!

jni · 2022-04-01T12:44:53Z

Nice catch @tibuch! Looks good to me.

It looks like the tolerance on some of the tests needs to be reduced to decimal=0, unfortunately. I think that's fine. This is one of the failures. Since it doesn't fail in every Python version or OS, I guess it is machine-dependent. But still looks right to me.

>       assert_almost_equal(length, target_length, decimal=1)
E       AssertionError: 
E       Arrays are not almost equal to 1 decimals
E        ACTUAL: 17.955520462276805
E        DESIRED: 17.73962515156774

tibuch · 2022-04-04T07:42:10Z

Don't know why these are failing:

=========================== short test summary info ============================
FAILED scikit-image/skimage/data/tests/test_data.py::test_eagle - ValueError:...
FAILED scikit-image/skimage/io/tests/test_pil.py::test_imread_index_png_with_alpha
FAILED scikit-image/skimage/io/tests/test_pil.py::test_extreme_palette - Valu...
===== 3 failed, 8076 passed, 15 skipped, 24 warnings in 290.59s (0:04:50) ======

jni · 2022-04-04T12:26:27Z

Pillow 9.1 was released Apr 1 so I presume that is the culprit, nothing to do with your work @tibuch!

jni · 2022-04-04T12:28:19Z

Yeah, see #6314

alexdesiqueira · 2022-04-06T18:09:26Z

Thank you @tibuch! How do you like this one after the changes, @grlee77?

grlee77

Thanks, @tibuch. The tests are looking good now. Thank you for fixing the normalized moments. I may take a quick look about using pytest.parametrize as @jni requested, but that is just a test style concern, so I wil go ahead and approve now.

tibuch · 2022-10-05T07:44:12Z

Hi,

I was wondering when this code will make it into an official release?

Cheers

lagru · 2022-10-05T12:24:25Z

Hey @tibuch. We've just started with the preparation of the next release 0.20. We are not certain on the precise target date yet but somewhere around 2022-10-24. You can follow along our 0.20 milestone if you want to stay informed on the progress.

…d regionprops (#422) related to #419 This PR ports features related to image moments and `regionprops` from scikit-image 0.20 to cuCIM - Improve performance of ``cucim.skimage.measure.moments_central`` by ~2x by avoiding redundant computations (adaptation of scikit-image/scikit-image#6188 to the GPU) - Support anisotropic images with different voxel spacings. Spacings can be defined with the new parameter ``spacing`` of the following functions in ``skimage.measure``: ``regionprops``, ``regionprops_table``, ``moments``, ``moments_central``, ``moments_normalized``, ``centroid``, ``inertia_tensor``, and ``inertia_tensor_eigvals`` (scikit-image/scikit-image#6296) - Voxel spacing is taken into account for the following existing properties in ``skimage.measure.regionprops``: ``area``, ``area_bbox``, ``centroid``, ``area_convex``, ``extent``, ``feret_diameter_max``, ``area_filled``, ``inertia_tensor``, ``moments``, ``moments_central``, ``moments_hu``, ``moments_normalized``, ``perimeter``, ``perimeter_crofton``, ``solidity``, ``moments_weighted_central``, and ``moments_weighted_hu``. (scikit-image/scikit-image#6296) - Raise a specific error message when accessing region properties from skimage.measure.regionprops when the required intensity_image is unavailable (scikit-image/scikit-image#6584). Authors: - Gregory Lee (https://github.com/grlee77) Approvers: - Gigon Bae (https://github.com/gigony) URL: #422

imagesc-bot · 2023-06-02T03:34:54Z

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/scaling-regionprops-for-non-isotropic-images/61971/5

Add spacing to moments and regionprops.

e4e3ab6

tibuch mentioned this pull request Mar 21, 2022

Add spacing to regionprops grlee77/scikit-image#2

Closed

6 tasks

grlee77 added the ⏩ type: Enhancement Improve existing features label Mar 21, 2022

Add pixel_area spacing = None test.

fb6ae43

jni requested changes Mar 22, 2022

View reviewed changes

tibuch added 3 commits March 22, 2022 12:07

Remove pixel_size and add num_pixels.

9125782

Remove spacing from coords and add coords_scaled.

c815607

Revert formatting.

2836167

jni approved these changes Mar 22, 2022

View reviewed changes

Fix coords_scaled column type.

a1a9dd2

grlee77 requested changes Mar 23, 2022

View reviewed changes

skimage/measure/_regionprops.py Outdated Show resolved Hide resolved

skimage/measure/_regionprops.py Outdated Show resolved Hide resolved

skimage/measure/_moments.py Outdated Show resolved Hide resolved

skimage/measure/_regionprops.py Show resolved Hide resolved

tibuch added 8 commits March 31, 2022 13:47

Add test for central moment computation with spacing and fix implemen…

ea7d496

…tation.

Add moments test with spacing.

ad19d71

Add test for normalized moments with scaled spacing.

4cd53ab

Fix moments normalized implementation.

4132293

Add tests for not implemented exceptions.

363111f

Remove new features from PROPS and COL_DTYPES.

d3fc1c1

Refactor condition.

e68ea9c

Add anisotropic spacing tests to minor/major axis length.

02afe85

Reduce test precision due to test failing on some machines.

bb9e960

grlee77 approved these changes Apr 7, 2022

View reviewed changes

grlee77 merged commit 6ac7c80 into scikit-image:main Apr 7, 2022

This was referenced Apr 7, 2022

parameterize moments tests #6323

Merged

measure.regionprops_table does not have num_pixels as a property #6337

Closed

tcompa mentioned this pull request Jul 20, 2022

Create core feature measurements fractal-analytics-platform/fractal-client#69

Closed

lagru added this to the 0.20 milestone Oct 5, 2022

grlee77 mentioned this pull request Nov 1, 2022

analytical moments computations, support pixel spacings in moments and regionprops rapidsai/cucim#422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add spacing to regionprops and moments. #6296

Add spacing to regionprops and moments. #6296

tibuch commented Mar 21, 2022 •

edited

pep8speaks commented Mar 21, 2022 •

edited

grlee77 commented Mar 21, 2022

jni left a comment

jni Mar 22, 2022

tibuch Mar 22, 2022

jni Mar 22, 2022

tibuch Mar 22, 2022

jni Mar 22, 2022

jni left a comment

grlee77 left a comment

tibuch commented Mar 31, 2022

jni commented Apr 1, 2022 •

edited

tibuch commented Apr 4, 2022

jni commented Apr 4, 2022

jni commented Apr 4, 2022

alexdesiqueira commented Apr 6, 2022

grlee77 left a comment

tibuch commented Oct 5, 2022

lagru commented Oct 5, 2022

imagesc-bot commented Jun 2, 2023

Add spacing to regionprops and moments. #6296

Add spacing to regionprops and moments. #6296

Conversation

tibuch commented Mar 21, 2022 • edited

Description

Checklist

For reviewers

pep8speaks commented Mar 21, 2022 • edited

Comment last updated at 2022-04-04 07:12:59 UTC

grlee77 commented Mar 21, 2022

jni left a comment

Choose a reason for hiding this comment

jni Mar 22, 2022

Choose a reason for hiding this comment

tibuch Mar 22, 2022

Choose a reason for hiding this comment

jni Mar 22, 2022

Choose a reason for hiding this comment

tibuch Mar 22, 2022

Choose a reason for hiding this comment

jni Mar 22, 2022

Choose a reason for hiding this comment

jni left a comment

Choose a reason for hiding this comment

grlee77 left a comment

Choose a reason for hiding this comment

tibuch commented Mar 31, 2022

jni commented Apr 1, 2022 • edited

tibuch commented Apr 4, 2022

jni commented Apr 4, 2022

jni commented Apr 4, 2022

alexdesiqueira commented Apr 6, 2022

grlee77 left a comment

Choose a reason for hiding this comment

tibuch commented Oct 5, 2022

lagru commented Oct 5, 2022

imagesc-bot commented Jun 2, 2023

tibuch commented Mar 21, 2022 •

edited

pep8speaks commented Mar 21, 2022 •

edited

jni commented Apr 1, 2022 •

edited