Skip to content

Accept target_values=None in surface_distance trio (#3712) - #3746

Draft
shaikn6 wants to merge 1 commit into
xarray-contrib:mainfrom
shaikn6:fix/3712-surface-target-values-none
Draft

Accept target_values=None in surface_distance trio (#3712)#3746
shaikn6 wants to merge 1 commit into
xarray-contrib:mainfrom
shaikn6:fix/3712-surface-target-values-none

Conversation

@shaikn6

@shaikn6 shaikn6 commented Sep 10, 2026

Copy link
Copy Markdown

Fixes #3712

Proposed Changes

  • surface_distance, surface_allocation, surface_direction now accept target_values=None (the value wrapper code passes when its own optional arg is unset), matching proximity / allocation / direction / cost_distance.
  • Changed the three signatures from target_values: list = [] to target_values: list = None and added the sibling if target_values is None: target_values = [] guard in the shared _compute dispatcher.
  • Added a regression test (test_target_values_none_matches_empty) parametrized over all three functions.

Why / evidence

The proximity trio and cost_distance all declare target_values: list = None and normalize to [] in the body. The three surface_* functions declared target_values: list = [] with no None guard, so target_values=None fell through to np.asarray(None, dtype=np.float64) (a 0-d array) and blew up before the numba kernel. On numpy 2.3 it surfaces as the existing ValueError: target_values must be a 1-D sequence; the issue reports a ~40-line numba TypingError on older numpy. Either way it is broken. [] as a default is also the mutable-default anti-pattern the siblings already avoid.

Confirmed with the reproduction from #3712 on this checkout: pre-fix, surface_distance/allocation/direction(target_values=None) raise while the other four return normally; post-fix all seven succeed.

Change

[] and None both mean "treat every non-zero finite pixel as a source", so callers passing an explicit list are unaffected and callers passing None go from a traceback to working code. No parameter renamed, no accepted input changes meaning, so no deprecation shim is needed.

Note: balanced_allocation carries the same target_values: list = [] default (flagged in #3712 as out of scope) and is left untouched here.

Verified

$ python -m pytest xrspatial/tests/test_surface_distance.py -q
81 passed, 19 skipped

$ python -m pytest xrspatial/tests/test_surface_distance.py -q -k target_values
6 passed, 94 deselected

$ python repro_3712.py   # reproduction script from the issue
proximity(target_values=None) -> OK
allocation(target_values=None) -> OK
direction(target_values=None) -> OK
cost_distance(target_values=None) -> OK
surface_distance(target_values=None) -> OK
surface_allocation(target_values=None) -> OK
surface_direction(target_values=None) -> OK

flake8 / isort: no new violations introduced by this change (the pre-existing surface_distance.py isort drift is tracked separately in #3710).

surface_distance, surface_allocation and surface_direction declared
target_values: list = [] while proximity, allocation, direction and
cost_distance use None and normalize to [] in the body. Passing
target_values=None (common when wrapper code threads an optional arg
through) reached np.asarray(None) and failed before the numba kernel.

Change the three signatures to None and add the sibling
'if target_values is None: target_values = []' guard in the shared
_compute dispatcher. [] and None both mean 'every non-zero finite pixel
is a source', so callers passing an explicit list are unaffected. Also
removes the mutable default argument the siblings already avoid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

surface_distance trio rejects target_values=None while proximity, allocation, direction and cost_distance accept it

1 participant