Skip to content

Fix: return object-typed properties as NumPy arrays in skimage.measure.regionprops_table - #272

Merged
ajschmidt8 merged 5 commits into
rapidsai:branch-22.06from
alxndrkalinin:branch-22.06
Jun 6, 2022
Merged

ajschmidt8 merged 5 commits into
rapidsai:branch-22.06from
alxndrkalinin:branch-22.06

Conversation

@alxndrkalinin

@alxndrkalinin alxndrkalinin commented May 10, 2022 •

Copy link
Copy Markdown
Contributor

This PR fixes adding properties whose results are objects to the dictionary that is returned by skimage.measure.regionprops_table.

Minimal example reproducing the issue:

from cucim.skimage.measure import regionprops_table

SAMPLE_MULTIPLE = cp.eye(10, dtype=cp.int32)
SAMPLE_MULTIPLE[3:5, 7:8] = 2

props = regionprops_table(SAMPLE_MULTIPLE, properties=['coords'])

raises TypeError: Implicit conversion to a NumPy array is not allowed., while returning correct values is expected (also see modified unit tests).

For some reason, only slice property was checked for, although the list of such properties already exists in OBJECT_COLUMNS.

@GPUtester

Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@alxndrkalinin
alxndrkalinin requested a review from a team as a code owner May 10, 2022 05:02
@jakirkham

Copy link
Copy Markdown
Member

Can one of the admins verify this patch?

@quasiben, could you please look at this one?

@quasiben

Copy link
Copy Markdown
Member

add to allowlist

@alxndrkalinin

Copy link
Copy Markdown
Contributor Author

I fixed a small formatting issue that was making tests fail, now it's all green besides label checker, but I can't assign those.

@jakirkham jakirkham added improvement Improves an existing functionality non-breaking Introduces a non-breaking change labels May 11, 2022
@jakirkham
jakirkham requested a review from grlee77 May 11, 2022 18:13
@jakirkham

Copy link
Copy Markdown
Member

@grlee77, could you please review this? πŸ™‚

@grlee77 grlee77 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @alxndrkalinin, the main fix here looks good to me!

I have one request to revert changes to the non-object code path in _regionprops.py as I think it will be slower. If you measured an improvement, though, please let me know.

Co-authored-by: Gregory Lee <grlee77@gmail.com>

@alxndrkalinin alxndrkalinin left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @grlee77!

Please see 2 small suggestions I have – if you think they're unnecessary, I'll be happy to just revert back to the list for non-object props.

@grlee77 grlee77 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @alxndrkalinin, this looks good now.

@jakirkham
jakirkham self-requested a review June 1, 2022 22:36
grlee77 pushed a commit to scikit-image/scikit-image that referenced this pull request Jun 2, 2022
@ajschmidt8
ajschmidt8 merged commit cbc16c2 into rapidsai:branch-22.06 Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants