scikit-image icon indicating copy to clipboard operation
scikit-image copied to clipboard

Add offset to regionprops

Open jni opened this issue 6 years ago • 4 comments

Description

To calculate region properties in a bigger volume than can be held in one image, we need to be able to specify the coordinates of the input image as an input to the regionprops function. Amazingly, as far as I can tell, this only affects the "coordinates" and "centroid" properties.

Checklist

  • [x] Docstrings for all functions
  • [ ] ~~Gallery example in ./doc/examples (new features only)~~ (Note: this will help dask-image, less helpful here...)
  • [ ] ~~Benchmark in ./benchmarks, if your changes aren't covered by an existing benchmark~~
  • [x] Unit tests
  • [x] Clean style in the spirit of PEP8

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

jni avatar Feb 02 '19 05:02 jni

Note: this doesn't quite address this SO question, but it gets us partway there.

jni avatar Feb 02 '19 05:02 jni

Why not just slice the image before computing regionprops?

stefanv avatar Feb 02 '19 06:02 stefanv

You slice it, then you can manually add in the slice start indices. Or you can slice it and add the appropriate offset= and get correctly-computed props in the global coordinate system.

jni avatar Feb 02 '19 06:02 jni

Does this seem reasonable @stefanv? @jni and I were interested in this because we were thinking how to handle a Dask-based regionprops implementation leveraging scikit-image under-the-hood.

jakirkham avatar Aug 05 '19 10:08 jakirkham

@jni This looks good to me (pending resolving the merge conflict and maybe rebasing on main). I am thinking about releasing 0.20.0rc0 tomorrow and wanted to check whether you thought we should merge this before or after 0.20. If before, feel free to make the updates and merge once the tests pass.

jarrodmillman avatar Jan 23 '23 00:01 jarrodmillman

@jni I'm afraid we need some test updates too.

stefanv avatar Jan 24 '23 07:01 stefanv

Sorry, I got distracted and wasn't able to investigate at the time. I've fixed the issue (with a force, had done the same thing and was easier than trying to do a bunch of pulling/merging/cherry-picking, sorry and thank you @jakirkham and @stefanv!), but there's a bit of an API design issue too, which is, self.slice in some use cases would itself have to include the offset. Just like we have coords and coords_scaled, we also might want slice and slice_offset. For example, if one did regionprops in a dask array, we would want to have the slice objects be valid not on chunks, but on the whole image.

However, there's several steps between this PR and dask_image.measure.regionprops. For example, we need to be able to combine features between chunks. So I don't think that this decision should hold up merge — this is already very useful to start experimenting with, I think.

jni avatar Jan 24 '23 10:01 jni

Thanks @jni! Feel free to open an issue for anything you still want to address.

stefanv avatar Jan 24 '23 17:01 stefanv

Thanks all for shepherding this one in! 😄

jakirkham avatar Jan 25 '23 18:01 jakirkham

👋 @jakirkham long time no see. 😊 let me know if you want to pair up on dask_image regionprops any time soon. 😃

jni avatar Jan 25 '23 20:01 jni