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

Infer object-dtype for if extra_property depends on mask size in regionprops_table #7136

Merged
merged 4 commits into from
Sep 21, 2023

Conversation

elena-pascal
Copy link
Contributor

@elena-pascal elena-pascal commented Sep 15, 2023

Description

Attempt to fix #7129 By using regions of different sizes to do the dtype check.

Checklist

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

Allow `extra_properties` of non-equal lengths to be passed correctly to 
`skimage.measure.regionprops_table`.

@stefanv
Copy link
Member

stefanv commented Sep 15, 2023

@lagru @jni If you have a moment to review, it would be much appreciated.

@elena-pascal
Copy link
Contributor Author

Sorry folks, I think it's a bit weedy and clearly my solution is failing the tests.

@lagru lagru self-requested a review September 15, 2023 19:10
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Thanks for taking a shot at this. I had to debug this locally. And to be honest, while doing so already came up with a possible solution. How about this patch?

Patch
Subject: [PATCH] Allow different extra prop shapes in regionprops_table

When the extra_property returns different shapes depending on the mask
shape, the infered dtype should be `np.object_`. The new approach
creates two masks with different shapes in a hopefully more explicit 
way.
---
Index: skimage/measure/tests/test_regionprops.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/measure/tests/test_regionprops.py b/skimage/measure/tests/test_regionprops.py
--- a/skimage/measure/tests/test_regionprops.py	(revision c2d31fbb2706763afba988c4c49bfbf93d24a5e2)
+++ b/skimage/measure/tests/test_regionprops.py	(date 1694860912577)
@@ -1280,6 +1280,11 @@
     return np.median(image_intensity[regionmask])
 
 
+def bbox_list(regionmask):
+    """Extra property who's output shape is dependent on mask shape."""
+    return [1] * regionmask.shape[1]
+
+
 def too_many_args(regionmask, image_intensity, superfluous):
     return 1
 
@@ -1337,14 +1342,19 @@
 
 
 def test_extra_properties_table():
-    out = regionprops_table(SAMPLE_MULTIPLE,
-                            intensity_image=INTENSITY_SAMPLE_MULTIPLE,
-                            properties=('label',),
-                            extra_properties=(intensity_median, pixelcount)
-                            )
+    out = regionprops_table(
+        SAMPLE_MULTIPLE,
+        intensity_image=INTENSITY_SAMPLE_MULTIPLE,
+        properties=('label',),
+        extra_properties=(intensity_median, pixelcount, bbox_list)
+    )
     assert_array_almost_equal(out['intensity_median'], np.array([2., 4.]))
     assert_array_equal(out['pixelcount'], np.array([10, 2]))
 
+    assert out["bbox_list"].dtype == np.object_
+    assert out["bbox_list"][0] == [1] * 10
+    assert out["bbox_list"][1] == [1] * 1
+
 
 def test_multichannel():
     """Test that computing multichannel properties works."""
Index: skimage/measure/_regionprops.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/skimage/measure/_regionprops.py b/skimage/measure/_regionprops.py
--- a/skimage/measure/_regionprops.py	(revision c2d31fbb2706763afba988c4c49bfbf93d24a5e2)
+++ b/skimage/measure/_regionprops.py	(date 1694860630250)
@@ -194,20 +194,17 @@
     dtype : NumPy data type
         The data type of the returned property.
     """
-    labels = [1, 2]
-    sample = np.zeros((3,) * ndim, dtype=np.intp)
-
-    regions = [(slice(0,),) * ndim, (slice(1, None),) * ndim]
-    sample[regions[0]] = labels[0]
-    sample[regions[1]] = labels[1]
-
-    propmasks = [(sample[region] == label) for (label, region) in zip(labels, regions)]
+    mask_1 = np.ones((1,) * ndim, dtype=bool)
+    mask_1 = np.pad(mask_1, (0, 1), constant_values=False)
+    mask_2 = np.ones((2,) * ndim, dtype=bool)
+    mask_2 = np.pad(mask_2, (1, 0), constant_values=False)
+    propmasks = [mask_1, mask_2]
 
     rng = np.random.default_rng()
 
     if intensity and _infer_number_of_required_args(func) == 2:
         def _func(mask):
-            return func(mask, rng.random(sample.shape))
+            return func(mask, rng.random(mask.shape))
     else:
         _func = func
     props1, props2 = map(_func, propmasks)

Please, let me know if I should push the patch directly or if you want to improve or iterate on it. :)

@lagru lagru added the 🩹 type: Bug fix Fixes unexpected or incorrect behavior label Sep 17, 2023
@elena-pascal
Copy link
Contributor Author

The patch looks good to me. Thank you for untangling this :)

When the extra_property returns different shapes depending on the mask
shape, the infered dtype should be `np.object_`.
Copy link
Member

@lagru lagru left a comment

Choose a reason for hiding this comment

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

Looks good to me. The CI failures should be unrelated and addressed in #7140.

Thanks @elena-pascal for collaborating! :)

@lagru lagru marked this pull request as ready for review September 18, 2023 12:57
Copy link
Member

@mkcor mkcor left a comment

Choose a reason for hiding this comment

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

Thanks both for working on this! I just spotted these details; I'll try the code locally now.

skimage/measure/tests/test_regionprops.py Outdated Show resolved Hide resolved
skimage/measure/tests/test_regionprops.py Outdated Show resolved Hide resolved
Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>
@elena-pascal
Copy link
Contributor Author

Ah ok, I accidently triggered the CI again. Sorry polar bears

@lagru lagru added this to the 0.22 milestone Sep 20, 2023
@lagru lagru merged commit de8f04a into scikit-image:main Sep 21, 2023
20 checks passed
@lagru lagru changed the title in check_dtype let masks be different sizes Infer object-dtype for if extra_property depends on mask size in regionprops_table Sep 21, 2023
@lagru
Copy link
Member

lagru commented Sep 21, 2023

Thanks!

@elena-pascal elena-pascal deleted the regions-are-different-sizes branch September 21, 2023 13:39
grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 6, 2024
grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 6, 2024
grlee77 added a commit to grlee77/cucim that referenced this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🩹 type: Bug fix Fixes unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

measure._regionprops._infer_region_props_dtype does not account for regions of different sizes
4 participants