Skip to content

Replace the hotspots confidence ladder with boolean arithmetic (#3737) - #3741

Open
brendancol wants to merge 2 commits into
mainfrom
issue-3737
Open

Replace the hotspots confidence ladder with boolean arithmetic (#3737)#3741
brendancol wants to merge 2 commits into
mainfrom
issue-3737

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3737

  • _calc_hotspots_numpy classified each Gi* z-score through a nine-branch if/elif ladder that recomputed abs(zscore) up to six times per cell. The p-value half of that ladder never changed the output: each p_value < threshold test is implied by the |z| test beside it (|z| > 2.58 forces p = 0.0099, |z| > 1.96 and |z| > 1.65 both force p <= 0.0495). The whole thing reduces to 99 for |z| > 2.58, 95 for 1.96 < |z| <= 2.58, 90 for 1.65 < |z| <= 1.96, else 0.
  • The classifier now computes confidence = 90*(az > 1.65) + 5*(az > 1.96) + 4*(az > 2.58) and hot_cold = (z > 0) - (z < 0). A comment above the code states the collapsed thresholds so nobody has to re-derive them. Output dtype (int8) and signature are unchanged.
  • The CUDA device function _gpu_hotspots still uses the ladder. It is not on the timed CPU path and branch cost on the GPU is a separate question, so it is left alone here.

Timings

_calc_hotspots_numpy alone, 2000x4000 float64 z-scores, median of 5 after warmup, 20-core host:

input before after
random normal (sd 1.5) 18.1 ms 11.6 ms
smooth ramp 18.3 ms 12.6 ms
random normal, 10% NaN 18.9 ms 12.1 ms

Identical int8 results in every case.

Verification

The original function was extracted from origin/main into a scratch module and compared with np.array_equal against the new one on:

  • 2000x4000 rng.normal(0, 1.5) in float64 and float32
  • a dense np.linspace(-4, 4, 2_000_001) sweep that crosses every threshold at both signs
  • the exact values +/-1.29, 1.65, 1.96, 2.33, 2.58 and their float64 neighbours via np.nextafter
  • NaN, +inf, -inf, +0.0, -0.0

All equal. The threshold-neighbour and non-finite cases are now pytest tests (test_hotspots_classifier_thresholds_3737, test_hotspots_classifier_nonfinite_3737) with hand-derived expected values.

Backends: numpy and dask+numpy share this kernel and both benefit. cupy and dask+cupy use the untouched CUDA kernel.

Test plan

  • pytest xrspatial/tests/test_focal.py -x -q (353 passed, GPU tests included on a CUDA box)
  • A/B equality against the origin/main implementation on the inputs listed above
  • Before/after timing on 2000x4000 float64

No benchmark change: FocalHotspots in benchmarks/benchmarks/focal.py already covers hotspots().

_calc_hotspots_numpy classified each Gi* z-score through a nine-branch
if/elif ladder that recomputed abs(zscore) up to six times per cell.
The p-value step in that ladder never changed the output: every
p_value < threshold test is implied by the |z| test beside it, so the
whole thing reduces to 99 for |z| > 2.58, 95 for 1.96 < |z| <= 2.58,
90 for 1.65 < |z| <= 1.96, else 0.

Compute the confidence as 90*(az > 1.65) + 5*(az > 1.96) + 4*(az > 2.58)
and the sign as (z > 0) - (z < 0). On a 2000x4000 float64 z-score
array the classifier drops from ~18.1 ms to ~11.6 ms (random normal),
~18.3 ms to ~12.6 ms (smooth ramp) and ~18.9 ms to ~12.1 ms (10% NaN),
with identical int8 results. NaN still classifies to 0 and +/-inf to
+/-99.

Add tests pinning the classification at each threshold and its float64
neighbours at both signs, and for NaN, +/-inf and signed zeros. The
CUDA device function keeps the ladder.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Sep 4, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Replace the hotspots confidence ladder with boolean arithmetic (#3737)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

  • xrspatial/tests/test_focal.py:1544 (test_hotspots_classifier_thresholds_3737) runs the classifier on float64, but every production caller feeds it float32 (_hotspots_numpy casts the z-scores at focal.py:1553). Under numba the float32 value is promoted to float64 before the comparison, so np.float32(1.65) sits just below 1.65 and np.float32(1.96) just above it. That is the same behaviour as the old ladder, and the PR's A/B covered float32, but no test pins it. Add a float32 check, for example assert that _calc_hotspots_numpy(z.astype(np.float32)) equals _calc_hotspots_numpy(z.astype(np.float32).astype(np.float64)) for the same threshold grid, so a future dtype-specific change to the comparison cannot slip through.

Nits (optional improvements)

  • xrspatial/focal.py:1479 wraps confidence = (90 * (az > 1.65) + 5 * (az > 1.96) + 4 * (az > 2.58)) across two lines with parentheses. The expression fits on one line under the 100-column limit and reads better that way.
  • xrspatial/focal.py:1472 describes _gpu_hotspots as still spelling out the ladder. If the GPU kernel is collapsed later, this comment goes stale; a shorter comment that states the three thresholds and leaves the GPU cross-reference out would survive that change.

What looks good

  • The derivation in the PR body is right: |z| > 2.58 forces p = 0.0099, and |z| > 1.96 or |z| > 1.65 force p <= 0.0495, so each p_value < ... test in the old ladder was already true whenever its |z| test was. The 1.29 and 2.33 thresholds never reached the output.
  • hot_cold * confidence stays in int64 inside the kernel and is cast to int8 on store, and the largest magnitude is 99, so there is no overflow path.
  • NaN and +/-inf behaviour is identical to the ladder and now pinned by test_hotspots_classifier_nonfinite_3737.
  • test_hotspots_dask_cupy_matches_numpy compares the untouched CUDA ladder against the rewritten CPU classifier on standard-normal data, so the two implementations are cross-checked in the 90 and 95 bands on a CUDA box.
  • The labeler applied performance, and FocalHotspots in benchmarks/benchmarks/focal.py already times this path.

Checklist

  • Algorithm matches reference (same classification as the previous ladder, verified by A/B and by the derivation)
  • All implemented backends produce consistent results (numpy/dask+numpy share the kernel; cupy/dask+cupy unchanged and parity-tested)
  • NaN handling is correct
  • Edge cases are covered by tests (threshold neighbours, NaN, inf, signed zero); float32 pin suggested above
  • Dask chunk boundaries handled correctly (no change to the map_blocks wrapper)
  • No premature materialization or unnecessary copies
  • Benchmark exists
  • README feature matrix: not applicable, no public API change
  • Docstrings: not applicable, private kernel

The production callers hand the classifier float32 z-scores, but the
new threshold test only ran on float64. Add a test that the float32
result matches the classification of the same values widened to
float64, on a grid that hits every band at both signs.

Put the confidence expression on one line and drop the cross-reference
to the GPU kernel from the comment so it does not go stale if that
kernel is collapsed later.

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: follow-up on b7d0446 (#3737)

Second pass after the review fixes.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

None.

Disposition of the first-pass findings

  • Float32 pin: fixed. test_hotspots_classifier_float32_3737 (xrspatial/tests/test_focal.py:1572) builds the threshold grid, casts to float32, widens it by one float32 ulp on each side, and asserts the classification equals the float64 widening of the same values. It also asserts all seven output bands appear, so the float32 grid really crosses every threshold at both signs.
  • One-line confidence expression: fixed at xrspatial/focal.py:1478, 78 columns.
  • Comment cross-reference to _gpu_hotspots: removed. The comment now states only the three thresholds and the NaN behaviour.

What looks good

  • The A/B against the origin/main kernel was re-run after the reformat and stays bitwise equal on every input class; the timing is unchanged (about 0.61x to 0.66x of the old kernel on 2000x4000 float64).
  • Full test_focal.py passes (354 tests, GPU cases included).

No further changes requested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hotspots: replace the nine-branch confidence ladder with boolean arithmetic

1 participant