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

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

Merged
merged 5 commits into from
Jun 6, 2022

Conversation

alxndrkalinin
Copy link
Contributor

@alxndrkalinin alxndrkalinin commented May 10, 2022

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
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
Member

Can one of the admins verify this patch?

@quasiben, could you please look at this one?

@quasiben
Copy link
Member

add to allowlist

@alxndrkalinin
Copy link
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
Member

@grlee77, could you please review this? 🙂

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, @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.

Copy link
Contributor Author

@alxndrkalinin alxndrkalinin 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 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.

python/cucim/src/cucim/skimage/measure/_regionprops.py Outdated Show resolved Hide resolved
python/cucim/src/cucim/skimage/measure/_regionprops.py Outdated Show resolved Hide resolved
python/cucim/src/cucim/skimage/measure/_regionprops.py Outdated Show resolved Hide resolved
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 @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