Accept target_values=None in the surface_* trio like the proximity trio

[brendancol]

Closes #3712

surface_distance, surface_allocation and surface_direction declared target_values: list = []. The four sibling functions in the same family (proximity, allocation, direction, cost_distance) declare None and normalize it to [] in the body, so target_values=None worked against four of the seven and raised a 40-line numba TypingError against the other three.

• Change the three defaults to None and normalize once in _compute.
• Update the target_values docstring so it matches the new default.

Not a breaking change. None and [] both mean "treat every non-zero finite pixel as a source", so callers passing an explicit list see no difference and callers passing None stop getting a traceback. No parameter was renamed, so no deprecation shim is needed.

Backend coverage

The normalization sits in _compute, ahead of the backend dispatch, so all four backends are covered. The new parity test runs against numpy, cupy, dask+numpy and dask+cupy; it was run on a CUDA host, so the two GPU variants executed rather than skipped.

Test plan

• test_target_values_none_matches_empty_list — None and [] produce identical output for all three functions on all four backends (12 cases)
• test_target_values_default_is_not_mutable — the public signatures no longer carry a mutable default
• pytest xrspatial/tests/test_surface_distance.py — 50 passed (37 before, 13 new), GPU tests included
• flake8 clean on xrspatial/surface_distance.py

Found by /sweep-api-consistency on the surface_distance module.

One note on something this PR does not touch: xrspatial/tests/test_surface_distance.py:234 has a pre-existing F841 (unused sd in test_allocation_consistency). It is unrelated to this change, so I left it alone.

[brendancol]

PR Review: Accept target_values=None in the surface_* trio like the proximity trio

Small, well-targeted change. The normalization lands in _compute (xrspatial/surface_distance.py:1300), which is the single dispatcher ahead of the numpy / cupy / dask+numpy / dask+cupy branch, so one guard genuinely covers all four backends and the two accessor classes, which forward **kwargs. No numeric path is touched.

Three things to tighten before merge, none of them blocking correctness.

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

• xrspatial/surface_distance.py:1426, :1487, :1529 -- the annotation is now target_values: list = None, which the default contradicts. Before this PR list = [] was at least type-correct; the PR is what makes it wrong, so it should carry the fix. The file already has from __future__ import annotations (line 28) and the repo already uses PEP 604 in xrspatial/hydro/stream_order_d8.py (method: str | None = None), so target_values: list | None = None costs nothing and needs no import. proximity and cost_distance have the same wrong annotation, but copying their mistake is not what "match the siblings" should mean here.

• xrspatial/tests/test_surface_distance.py:362 -- the parity test only exercises one of the two dask branches. _make_raster defaults to chunks=(3, 3) and res=1.0, so max_distance=2.0 gives pad = int(2.0 / 1.0) + 1 = 3, which fails the pad < max(chunks_y) and pad < max(chunks_x) test at surface_distance.py:1206 and routes to _sd_dask_iterative. The bounded map_overlap route in _surface_distance_dask_bounded never runs. Parametrizing max_distance over a bounded value below 2.0 and np.inf would cover both.

• xrspatial/tests/test_surface_distance.py:373-375 -- the fixture is thin. With that elevation ramp (0.5 per cell, so a cardinal step costs sqrt(1 + 0.25) = 1.118) and max_distance=2.0, only 4 of the 36 pixels come back finite; the other 32 comparisons are NaN against NaN. It does assert something real, but a wider budget would make the None-vs-[] claim much harder to satisfy by accident.

Nits (optional improvements)

• test_target_values_default_is_not_mutable imports inspect inside the function body. That matches the surrounding file, which does the same for cost_distance and unittest.mock, so this is fine as-is; noting it only so the next reader does not flag it.

What looks good

• The guard sits before _validate_raster, so None is resolved before anything else can trip over it. No per-backend duplication.
• Non-breaking, and correctly identified as such: None and [] both mean "every non-zero finite pixel is a source", so no deprecation shim is warranted. Good call not adding one.
• The docstring was updated alongside the default rather than left to drift, which is how this class of issue gets created in the first place.
• The issue carries a runnable reproduction with real observed output, and the same script now prints OK for all seven functions.

Note on something this PR does not cause

While reading the dask path I saw surface_direction return different bearings on numpy {0, 90, 270} and dask+numpy {0, 126.87, 135, 270} for identical input. That is the block-local-vs-global source index bug in _finalize_direction, already filed as #3713 and #3719. It predates this PR and does not affect the new test, which only compares None against [] within a single backend. Flagging it so nobody blames this change for it.

Checklist

• Algorithm matches reference -- no algorithm changed
• All implemented backends produce consistent results -- new test covers all four, executed on a CUDA host
• NaN handling is correct -- unchanged
• Edge cases are covered by tests -- see the two test suggestions above
• Dask chunk boundaries handled correctly -- unchanged
• No premature materialization or unnecessary copies
• Benchmark exists or is not needed -- not needed, no perf change
• README feature matrix updated (if applicable) -- not applicable
• Docstrings present and accurate

[brendancol]

PR Review (follow-up pass): Accept target_values=None in the surface_* trio

Re-reviewed after ca17a3fa. All three suggestions from the first pass are addressed.

Disposition of the earlier findings

• Annotation contradicts the default -- fixed. All three signatures now read target_values: list | None = None (xrspatial/surface_distance.py:1426, :1487, :1529). Confirmed at runtime: inspect.signature reports list | None with default None for each. proximity and cost_distance still carry the older list = None spelling; that is their modules' problem, not this PR's.
• Only one dask route exercised -- fixed. max_distance is now parametrized over 1.5 and np.inf (xrspatial/tests/test_surface_distance.py:366). At res=1.0 and chunks=(3, 3), 1.5 gives pad = 2, which passes the pad < max(chunks) gate and routes through _surface_distance_dask_bounded; np.inf skips the finite check entirely and routes through _sd_dask_iterative. Both branches now run for all three functions.
• Thin fixture, mostly NaN-vs-NaN -- fixed, and better than asked. Rather than only widening the budget, the test now asserts np.isfinite(with_none).any() before comparing, so a future change that quietly turns the output all-NaN fails instead of passing vacuously.
• The import inspect nit was raised as already-fine and needed no change.

Blockers

None.

Suggestions

None.

Nits

None.

Verification

• pytest xrspatial/tests/test_surface_distance.py -- 62 passed, up from 37 on main. The 24 new parity cases (3 functions x 4 backends x 2 budgets) all ran; this is a CUDA host, so cupy and dask+cupy executed rather than skipped.
• flake8 xrspatial/surface_distance.py -- clean.
• The pre-existing F841 at xrspatial/tests/test_surface_distance.py:234 is untouched and predates this branch (confirmed against HEAD).

The surface_direction numpy-vs-dask bearing divergence noted in the first pass is still there and still belongs to #3713 / #3719, not here.

[brendancol]

Integration note: PR #3718 (api-consistency sweep) and PR #3720 (error-handling sweep) both add target_values guards to _compute() in the same /deep-sweep run, within about twenty lines of each other. The intents are complementary, not competing, but the order matters:

• Accept target_values=None in the surface_* trio like the proximity trio #3718 normalizes target_values=None to [] ahead of the backend dispatch, so None becomes a valid way to say "all non-zero finite pixels".
• Reject NaN and negative max_distance in the surface distance functions #3720 rejects non-1-D target_values by reading .ndim and .tolist(), which assumes the value has already been coerced to an array.

Whichever merges second will need a rebase, and the composed result must keep the None normalization before the dimensionality check. If the check runs first, None coerces to a 0-d object array and gets rejected by the very guard meant to catch scalars, which would break #3718's 24-case None parity test.

Worth running both PRs' tests against the merged tree rather than trusting each PR's own green run.