Separate dense and decomposed footprint API#7746
Separate dense and decomposed footprint API#7746lagru wants to merge 13 commits intoscikit-image:mainfrom
Conversation
Since this function is now exposed to all kinds of footprints, I tried to add sanity checks that didn't impact the performance to much and tried to come up with a sensible behavior for edge cases. It also removes the `dtype` parameter. Instead it just uses the dtype of `footprint`, allowing users to control the outputs dtype that way. I couldn't really think of use cases where the parameter would be more useful.
cd65537 to
cc4fbab
Compare
This should give users control on how correct the approximation is supposed to be and also catches cases where the footprint isn't convex.
| for dim in range(footprint.ndim): | ||
| mirrored = np.flip(footprint, axis=dim) | ||
| if np.any(footprint != mirrored): | ||
| msg = f"`footprint` isn't symmetric in dimension {dim}" | ||
| raise ValueError(msg) |
There was a problem hiding this comment.
With the addition of the max_error argument, this check isn't strictly necessary since an asymmetric footprint should usually exceed the allowed error rate. However, we may want to keep this because it provides a clearer warning?
Edit: not-convex -> asymmetric
| elif col_sums[i] < col_sums[i + 1]: | ||
| raise ValueError("`footprint` is not convex") |
There was a problem hiding this comment.
Same here. This catches not-convex footprints but only in some cases and only in one dimension. E.g. it misses the holes in
footprint = np.array([[0, 1, 1, 1, 0],
[1, 0, 1, 0, 1],
[1, 1, 1, 1, 1],
[1, 0, 1, 0, 1],
[0, 1, 1, 1, 0]])So we could just remove this check.
and deprecate `decomposition` argument in `footprint_rectangle`. It's a bit unfortunate that we need to do a deprecation warning because it was already released in 0.25.
This reverts commit 66c3998.
There was a problem hiding this comment.
Note the function footprint_from_sequence(footprints) which is a bit of an outlier in terms of dtype behavior. Unlike all other footprint functions, it doesn't allow specifying the dtype and it returns a bool array by default; all others default to np.uint8.
If we want to change this default behavior, we'd have to do so in skimage2. Unless we rename the function to something like footprint_compose (which I prefer).
Thoughts?
|
Hello scikit-image core devs! There hasn't been any activity on this PR for more than 180 days. I have marked it as "dormant" to make it easy to find. To our contributors, thank you for your contribution and apologies if this contribution fell through the cracks! Hopefully this ping will help bring some fresh attention to the review. If you need help, you can always reach out on our forum If you think that this PR is no longer relevant, you may close it, or we may do it at some point (either way, it will be done manually). If you think the PR is valuable but you no longer have the bandwidth to update it, please let us know, so that someone can take it over. 🙏 |
Description
Closes #7736; see that issue for more context. I'll expand this description as this PR shapes up.
_cross_decompositionas a new publicfootprint_cross_decompose.Checklist
./doc/examplesfor new featuresRelease note
For maintainers and optionally contributors, please refer to the instructions on how to document this PR for the release notes.