Describe the bug
sky_view_factor computes horizon angles using ray distance in cells rather than ground units. The kernels (_svf_cpu and _svf_gpu in xrspatial/sky_view_factor.py) compute:
dz = elev - center
dist = float(r) # r is the ray step index, in cells
elev_angle = atan2(dz, dist)
dz is in elevation units (typically meters); dist is in cells. For any DEM whose cell size is not 1.0 in the same unit as elevation, the ratio is wrong by a factor of cell_size.
Concrete example
A 30 m SRTM tile with cell size 30 m and a neighbor 30 m higher should give a horizon angle of atan(30/30) = 45°. The current code computes atan(30/1) ≈ 88°, which collapses SVF toward 0 even on gentle terrain.
For an aerial photogrammetry DEM at 0.5 m cell size with a neighbor 1 m above, the true angle is atan(1/0.5) ≈ 63.4°. The current code returns atan(1/1) = 45°.
Expected behavior
Read cell size from the DataArray (via get_dataarray_resolution from xrspatial.utils) and compute true ground distance per ray step:
ground_dx = r * dx * cellsize_x
ground_dy = r * dy * cellsize_y
dist = sqrt(ground_dx**2 + ground_dy**2)
Allow callers to pass explicit cellsize_x / cellsize_y overrides for cases where the DataArray lacks usable coordinates.
Why existing tests miss this
test_known_svf_simple_ramp uses cell size 0.5 with a z = x ramp but only asserts 0.5 < svf < 1.0. Both the buggy 45° angle and the correct 63.4° angle satisfy that band, so the bug slips through.
Scope
_svf_cpu (CPU kernel)_svf_gpu (CUDA kernel)_run_numpy, _run_cupy, _run_dask_numpy, _run_dask_cupy- Public
sky_view_factor() API
- Tests covering non-unit cell size and anisotropic cell size
Categories: Cat 4 (missing projection / cell-size correction)
Severity: HIGH
Describe the bug
sky_view_factorcomputes horizon angles using ray distance in cells rather than ground units. The kernels (_svf_cpuand_svf_gpuinxrspatial/sky_view_factor.py) compute:dzis in elevation units (typically meters);distis in cells. For any DEM whose cell size is not 1.0 in the same unit as elevation, the ratio is wrong by a factor ofcell_size.Concrete example
A 30 m SRTM tile with cell size 30 m and a neighbor 30 m higher should give a horizon angle of
atan(30/30) = 45°. The current code computesatan(30/1) ≈ 88°, which collapses SVF toward 0 even on gentle terrain.For an aerial photogrammetry DEM at 0.5 m cell size with a neighbor 1 m above, the true angle is
atan(1/0.5) ≈ 63.4°. The current code returnsatan(1/1) = 45°.Expected behavior
Read cell size from the DataArray (via
get_dataarray_resolutionfromxrspatial.utils) and compute true ground distance per ray step:Allow callers to pass explicit
cellsize_x/cellsize_yoverrides for cases where the DataArray lacks usable coordinates.Why existing tests miss this
test_known_svf_simple_rampuses cell size 0.5 with az = xramp but only asserts0.5 < svf < 1.0. Both the buggy 45° angle and the correct 63.4° angle satisfy that band, so the bug slips through.Scope
_svf_cpu(CPU kernel)_svf_gpu(CUDA kernel)_run_numpy,_run_cupy,_run_dask_numpy,_run_dask_cupysky_view_factor()APICategories: Cat 4 (missing projection / cell-size correction)
Severity: HIGH