fix(granularity): guard division when subsampling collapses an axis#91
Conversation
The `get_granularity` function could encounter a division by zero error when an axis of the input mask collapses to a size of 1 during subsampling (e.g., when `subsample_size=0.25` and the axis length is < 8). This occurred in the `map_coordinates` scaling factor calculation. Introduce `_safe_ratio` to return 0 when the denominator is 0, correctly handling this edge case. Adds new regression tests for 2D and 3D collapsed axes (issue #90).
The `num` and `den` parameters are already typed as `float`, making the explicit `float()` casts redundant before division.
Benchmark —
|
| size \ objects | 16 | 64 | 256 |
|---|---|---|---|
| 256 | 1.02× | 1.00× | 1.01× |
| 512 | 0.99× | 1.00× | 1.08× |
| 1024 | 1.03× | 1.04× | 0.96× |
radial_zernikes
| size \ objects | 16 | 64 | 256 |
|---|---|---|---|
| 256 | 0.98× | 0.99× | 1.01× |
| 512 | 0.98× | 0.98× | 1.00× |
| 1024 | 1.08× | 1.00× | 0.99× |
zernike
| size \ objects | 16 | 64 | 256 |
|---|---|---|---|
| 256 | 1.00× | 0.99× | 1.00× |
| 512 | 0.99× | 0.98× | 1.02× |
| 1024 | 1.05× | 1.01× | 1.00× |
The `subsample_size` calculation could result in `new_shape` dimensions of 0 or 1, leading to: - Division by zero in coordinate mapping (when `new_shape[k] - 1 == 0`). - Empty array errors in subsequent morphology operations (when `new_shape[k] == 0`). This change clamps the calculated `new_shape` dimensions to a minimum of 2 to resolve these issues (addresses #90).
A meaningful granularity spectrum cannot be computed when subsampling reduces an axis to fewer than two samples. Return NaN to prevent crashes and indicate an undefined spectrum. Addresses issue
| effective_shape = ( | ||
| (orig_shape * subsample_size).astype(int) if subsample_size < 1 else orig_shape | ||
| ) | ||
| if (effective_shape < 2).any(): |
There was a problem hiding this comment.
Is granularity supported for 3D? I feel like this any check might cause unforseen complications if there's a third axis at play. Might be more robust to have that change conditional either on the shape length or the canonical positions for x/y
There was a problem hiding this comment.
It is, according to https://docs.google.com/spreadsheets/d/1_7jQ8EjPwOr2MUnO5Tw56iu4Y0udAzCJEny-LQMgRGE/edit?gid=0#gid=0. I'll see if that makes more sense.
There was a problem hiding this comment.
This .any() essentially checks that all dimensions have at least 2 samples, regardless of whether or not it's 2 or 3D.
| @@ -1,37 +1,18 @@ | |||
| """Regression tests for ``get_granularity`` (issue #90). | |||
| """Regression test for ``get_granularity`` (issue #90). | |||
There was a problem hiding this comment.
Probably shouldn't cite the issue in this line because we'll 100% forget to update this when we add more tests some day
There was a problem hiding this comment.
(fixed), removed the issue tagging
|
Merged, feel free to rebase the other ongoing changes into main. |
2 participants
Closes #90.
For inputs with an axis shorter than
1/subsample_size,new_shape[k]collapses to 1 and the back-projection upsample divides bynew_shape[k] - 1 == 0. Added_safe_ratioto return 0 in that case (the single output sample along the collapsed axis just readsback_pixels[0]), used at all five 2D/3D call sites. Bit-identical behavior whenevernew_shape[k] > 1.Tests cover the 2D and 3D paths.
cc @timtreis — this overlaps with the same upsample geometry your fused-operator perf PR rewrites in #76; you may want to fold the guard in when rebasing.