Skip to content

Bin dense radial profiles without sorting - #108

Merged
PierreRaybaut merged 1 commit into
developfrom
fix/radial-profile-dense-bins
Oct 27, 2026
Merged

PierreRaybaut merged 1 commit into
developfrom
fix/radial-profile-dense-bins

Conversation

@PierreRaybaut

Copy link
Copy Markdown
Contributor

Closes #107

Summary

Since #104, get_radial_profile always grouped the pixels with np.unique(..., return_inverse=True) before np.bincount, so that only the bins holding pixels were allocated. The sort it involves made radial profiles of large images 2 to 3 times slower than in Sigima 1.3 in the usual case (uniform coordinates, normal step), where the bins are far fewer than the pixels.

  • get_radial_profile now chooses the grouping strategy from the number of bins, with no tuning:
    • dense bins (largest bin index lower than the number of pixels, i.e. the usual case): direct np.bincount on the bin index, empty bins left out as before — no sort, no more entries than pixels. The bin index is converted to integers only after this test, since it is unbounded for tiny steps;
    • sparse bins (tiny steps): unchanged np.unique + np.bincount path.
  • The memory used still never exceeds the size of the image, whatever the step, and results are bit-identical in both paths (same bins, same order of accumulation).
  • The contract of radial_profile is unchanged (version 2). No release note: the slowdown was never released (Define image profiles in physical coordinates #98 and Allocate only the occupied bins of radial profiles #104 are later than v1.3.0).

Performance

Whole get_radial_profile, random image, off-pixel center, step 1, NumPy 2.4.4 (best of 7):

Image v1.3.0 (direct bincount) develop (np.unique) This PR
1000 × 1000 0.023 s 0.050 s 0.024 s
2000 × 2000 0.095 s 0.215 s 0.100 s
4000 × 4000 0.380 s 0.974 s 0.395 s

The small remaining difference with v1.3.0 comes from the input checks added by #104.

Validation

  • New tests (profile_physical_unit_test.py):
    • bit-identity with the former implementation now also covers sparse bins (tiny gap between two pixel centers, about 10⁵ bins for 300 pixels), the existing cases covering dense bins (float, integer and masked data);
    • guard against the slowdown: np.unique is not called for dense bins, and is called for sparse bins (this test fails on develop).
  • Full suite: 2366 passed, 63 skipped; --qualification-report: no problem (radial_profile v2 qualified, validation and replay exact).
  • DataLab develop against this branch: profile_app_test.py, provenance_processing_unit_test.py, interactive_processing_test.py pass (52).

Since #104, get_radial_profile always grouped the pixels with np.unique and
its inverse index before np.bincount, so that only the bins holding pixels
were allocated. The sort it involves made radial profiles of large images 2 to
3 times slower than in Sigima 1.3 in the usual case (uniform coordinates,
normal step), where the bins are far fewer than the pixels and accumulating
them directly by index was both safe and fast.

The bins are now accumulated directly by index when the largest bin index is
lower than the number of pixels (the bin index being converted to integers
only after this test), and the bins holding pixels are kept with np.unique
otherwise (tiny steps): the memory used still never exceeds the size of the
image, whatever the step, and the results are bit-identical in both cases
(same bins, same order of accumulation). The contract of radial_profile is
unchanged.

Closes #107

Assisted-by: Claude Opus 5.5
@PierreRaybaut PierreRaybaut added the bug Something isn't working label Oct 27, 2026
@PierreRaybaut
PierreRaybaut merged commit 73d6b9b into develop Oct 27, 2026
6 checks passed
@PierreRaybaut
PierreRaybaut deleted the fix/radial-profile-dense-bins branch October 27, 2026 06:37
@ThomasMalletCodra ThomasMalletCodra added this to the v1.4.0 milestone Oct 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants