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

WIP: Allow regionprops to reuse objects #4087

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jni
Copy link
Member

@jni jni commented Aug 6, 2019

Description

If we want to measure regionprops with the same objects over multiple channels, it is wasteful to run ndimage.find_objects for each channel. The long term fix would be to add multichannel support, but this is more general anyway and raises fewer API issues.

CC @VolkerH

Checklist

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.
  • Consider backporting the PR with @meeseeksdev backport to v0.14.x

Copy link
Member

@hmaarrfk hmaarrfk left a comment

Choose a reason for hiding this comment

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

Need a test

@rdgraham
Copy link

I might not be totally clear about the intended use-case here, but it seems like if implemented this way then it would be possible to get quite unexpected results unless the user is careful to use the new option in a specific way.

Is the intention to have one original labelled image as the 'master' segmentation, and then get the different metrics corresponding to different intensity images, but based on the same original segmentation? If so, I think it would be better to just make sure it is possible to make a copy of a RegionProperties object but with the intensity_image swapped out for the new channel.

If the intention is to generate new RegionProperties objects where the supplied slices for the objects don't match the label_image, then I think that could give quite confusing results. But it would be a supported usage.

@sciunto sciunto added the 🔧 type: Maintenance Refactoring and maintenance of internals label Sep 23, 2019
@sciunto sciunto added this to the 0.18 milestone Sep 23, 2019
@emmanuelle emmanuelle mentioned this pull request Dec 13, 2019
7 tasks
@jni
Copy link
Member Author

jni commented Apr 7, 2020

@rdgraham, thanks for chiming in! (A long time ago I know! 😅)

Is the intention to have one original labelled image as the 'master' segmentation, and then get the different metrics corresponding to different intensity images, but based on the same original segmentation?

Yes, that's the intention!

If so, I think it would be better to just make sure it is possible to make a copy of a RegionProperties object but with the intensity_image swapped out for the new channel.

Well, given our current API, it's a bit tricky, because:

  • We don't return a regionprops object, but rather a list of them. Swapping the intensity image for each wouldn't be hard but it would certainly be annoying/not pretty/elegant.
  • regionprops_table doesn't return Regionprops objects at all, so there is nothing much to do there.

It could be that adding channel_axis= would be the right approach (indeed it would make a joined table easier to generate). But that requires quite a bit more engineering...

@rdgraham
Copy link

rdgraham commented Apr 8, 2020

@rdgraham, thanks for chiming in! (A long time ago I know! sweat_smile)

Is the intention to have one original labelled image as the 'master' segmentation, and then get the different metrics corresponding to different intensity images, but based on the same original segmentation?

Yes, that's the intention!

If so, I think it would be better to just make sure it is possible to make a copy of a RegionProperties object but with the intensity_image swapped out for the new channel.

Well, given our current API, it's a bit tricky, because:

* We don't return a regionprops object, but rather a list of them. Swapping the intensity image for each wouldn't be hard but it would certainly be annoying/not pretty/elegant.

* `regionprops_table` doesn't return Regionprops objects at all, so there is nothing much to do there.

It could be that adding channel_axis= would be the right approach (indeed it would make a joined table easier to generate). But that requires quite a bit more engineering...

Interesting discussion which has come back to life again ..

I do have some code where I have exactly this use case; images with several different modalities for which I am measuring various properties. Thus far, I haven't really had a need for functionality like this; I generally just have an object wrapping a regionprops and use the mask directly when I need properties of the different modalities.

I often tend not to use the intensity_image because one of the problems that sometimes happens is that you can end up accidentally tying up a lot of memory if the reference in the regionprops is preventing the image data from otherwise being de-referenced when no longer needed.

That said, an easier built-in solution would still be a good idea.

Adding a channel axis option and stacking all the channels together seems simple at first, but I would discourage this approach as I see at least a couple of problems:

  1. You would loose the ability for different channels to have different dtype.
  2. You would have to allocate a lot of memory all at once, and it would not be able to be de-referenced until you are entirely done with all the channels. I can think of examples where I literally would not have enough system memory available to load all channels simultaneously.

In the guise of my original suggestion for 'making it easy' to change the intensity image, I would suggest adding a new container class for regionprops, and returning that. Could extend list, so as to maintain API and backwards compatibility. Then add a property for changing all the intensity images in the regions.

Incidentally, IMHO a container class would also make a much better place for a method that returns a table of properties than the current setup for the recently added regionprops_table. That would preserve the ability to change the intensity image.

@grlee77
Copy link
Contributor

grlee77 commented Sep 27, 2020

I see this issue is tagged for v0.18.

It looks good to me, but looks like it still needs tests (and a rebase)

@jni jni modified the milestones: 0.18, 0.19 Nov 26, 2020
Base automatically changed from master to main February 18, 2021 18:23
@grlee77
Copy link
Contributor

grlee77 commented Jul 8, 2021

It looks like this could be easy to add a test and complete, but it is mainly a question if this is the API we want. It seems reasonable, but I haven't thought about alternatives enough to have a strong opinion either way.

If this is not going to be revisited in the near future, please consider removing the 0.19 milestone.

@grlee77 grlee77 modified the milestones: 0.19, 1.0 Aug 19, 2021
@jarrodmillman jarrodmillman modified the milestones: 0.21, 0.22 May 19, 2023
@jarrodmillman jarrodmillman removed this from the 0.22 milestone Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔧 type: Maintenance Refactoring and maintenance of internals
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants