allow passing extra measurement functions to regionprops and regionprops_table #4810
Conversation
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
|
Hello @VolkerH! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2020-07-06 15:21:14 UTC |
|
@scikit-image/core before we keep moving on this, it would be great to get some consensus on the API. Currently, it looks like this: rp = regionprops(
label_image,
intensity_image,
extra_properties=(mesh_surface, porosity, average_surface_curvature),
)Those properties can then be accessed with |
Shouldn't that be a dict instead of a tuple? |
dict mapping what to what? It's a sequence of functions. We can make a dict mapping name -> function, but Python makes that unnecessary/redundant since we can use |
|
I guess I was thinking about lambdas, but then it's easy enough to just define a new function.
|
Yes, I would leave this concern for future PRs. We could also accept both a sequence and a dict, but I fear that complicates things. |
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
Co-authored-by: Juan Nunez-Iglesias <juan.nunez-iglesias@monash.edu>
|
Thanks for those changes. Writing code in comments presents different challenges from just writing code. |
|
Everything is passing! @scikit-image/core this is ready for review! |
skimage/measure/_regionprops.py
Outdated
| the dtype is inferred by calling the function on a small sample. | ||
| If the name of an extra property clashes with the name of an existing | ||
| property the extra property wil not be visible and a UserWarning is | ||
| issued. |
There was a problem hiding this comment.
Shouldn't we specify that the first argument of the property function must be the mask of the considered region and the optional second argument must be the corresponding image?
There was a problem hiding this comment.
yes, we should. So far that could only be derived from the examples. I just added this, happy for suggestions about how to phrase it better.
| f"Extra property '{name}' is shadowed by existing " | ||
| "property and will be inaccessible. Consider renaming it." | ||
| ) | ||
| warn(msg) |
There was a problem hiding this comment.
Can't we dynamically edit COL_DTYPES to avoid the handling of dtype for extra properties in _props_to_dict?
| warn(msg) | |
| warn(msg) | |
| else: | |
| COL_DTYPES[func.__name__] = _infer_regionprop_dtype( | |
| func, | |
| intensity=intensity_image is not None, | |
| ndim=self._ndim, | |
| ) |
There was a problem hiding this comment.
@rfezzani here we chose to preserve existing behaviour rather than try to improve it, which could have unintended consequences. I suggest leaving dynamic inference of existing properties for a future PR.
There was a problem hiding this comment.
The suggestion is not an improvement to existing behavior but the modification of the extra properties (new feature) dtype handeling ^^. Avoiding namespace variable modification is a good point, no problem ;)
skimage/measure/_regionprops.py
Outdated
| the dtype is inferred by calling the function on a small sample. | ||
| If the name of an extra property clashes with the name of an existing | ||
| property the extra property wil not be visible and a UserWarning is | ||
| issued. |
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
|
Thanks very much for the review and the suggestions @rfezzani, I made the suggested changes apart from the dynamic addition to |
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
Co-authored-by: Riadh Fezzani <rfezzani@gmail.com>
|
🎉 The CI is green again (almost :p). Merging! |
|
Hello @VolkerH, Firstly I would like to say that I love this feature, thank you for this 🙏. scikit-image/skimage/measure/_regionprops.py Line 253 in 3a4dc58 Just for background, for my use case I need to know the |
|
Hi @refack , glad you like it, appreciate the feedback that it is useful. As for your second question: it simply did not occur to me at the time and I wanted a simple function signature for the If your extra property depends on other properties calculated for the region, I am not sure whether passing If your desired result depends on several properties, is it possible to calculate if from several columns of the I have not looked at the regionprops code since last July (and that is the only time I looked at it) so I might be missing something. It may also be possible to add what you want without breaking things. But @jni (who deserves at least equal credit for implementing this feature) will be better positioned to answer that. |
Me neither! But just today I found that I could have used it!
Actually this is not a concern — all the properties are dynamically calculated. It would be a concern if you wanted to have chains of custom functions that depended on each other. Then you would have to make sure that you provided them in the order they are needed. @refack I think this is certainly worth considering. Would you mind creating an issue specifically for this (e.g. "Proposal: optionally pass Regionprops object to custom properties") so that we can discuss the best way to do this? imho the argument should not be called "self" as that is a reserved-by-convention term in Python. Perhaps simply |
Description
This is a WIP implements the feature request discussed here: #4466
It extends
regionpropsandregionprops_tableby adding an extra argument, which is an iterable of callables/functions.A user might want to do this if a measurment they require is not included in the built-in properties. By specifying these functions users won't have to duplicate the functionality that loops over the regions. Possible use cases:
Each provided function must either take one (
region) or two (region,intensity_image) (names for illustration, this is by order) arguments.In the issue, there was some discussion on whether the name and return dtype of the measurement function need to be passed in addition, as these are needed to build the regionprops table. Both the name of new property and the dtype are now inferred.
The name is inferred simply as the function name, and a warning is issued if there is a name clash. The return dtype is determined by passing a tiny test image to the function and inspecting the return value.
At this stage, the functionality is working but tests and examples still need to be added.
Code contributions:
Much of code in this PR was co-authored by @jni during a VS Code LiveShare session (somehow VS code only appended the co-authorship note to one of the commit messages).
Checklist
./doc/examples(new features only)./benchmarks, if your changes aren't covered by anexisting benchmark
For reviewers
later.
__init__.py.doc/release/release_dev.rst.