Fix viewshed crash on NaN cells and mark them INVISIBLE (#2857)#2876
Merged
Conversation
NaN cells used to generate ENTERING/CENTER/EXITING sweep events. A NaN on the observer row to the right was never inserted into the status structure, so its EXITING event made _delete_from_tree raise ValueError: node not found. _init_event_list now skips NaN cells entirely and returns the event count so the caller trims unused trailing rows (which would otherwise sort as CENTER events at cell (0, 0) and spuriously mark it visible). NaN cells keep their INVISIBLE fill value, which downstream != INVISIBLE checks treat as not visible. Convert the xfail regression at test_viewshed.py into passing tests covering all NaN positions, the observer-row-right case, the (0, 0) leak guard, and the numpy + dask backends.
brendancol
commented
Jun 2, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Fix viewshed crash on NaN cells and mark them INVISIBLE (#2857)
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- The new
if np.isnan(inrast[1][j]): continuereadsinrast[1][j], the same value the line above stores intoe[E_ELEV_1](viewshed.py:1165). Reusinge[E_ELEV_1]for the guard would drop the second index, but readinginrast[1][j]directly next to the comment is clearer. Wash. Leave as-is.
What looks good
- Root cause is right. NaN cells emitted EXITING events with no matching insert (the pre-insert loop at viewshed.py:1388 guards on
not np.isnan), so_delete_from_treeraised. Skipping event creation for NaN cells removes the asymmetry. - The trailing-row trim (
event_list = event_list[:count_event]) is load-bearing, not cosmetic. Without it the leftover all-zero rows sort as CENTER events at (0, 0) and would mark that cell visible.test_viewshed_nan_does_not_leak_visibility_to_origincovers that path. - Sentinel choice is correct. NaN cells stay at the INVISIBLE fill value, which is what visibility.py:287 and experimental/min_observable_height.py:171 test against. A NaN output would have read as visible; INVISIBLE does not.
- Verified two more edge cases locally: a NaN observer cell and an all-NaN raster both return cleanly (observer 180, rest INVISIBLE) with no crash.
- The dask Tier B path runs through
_viewshed_cpu, so it picks up the fix. The dask+numpy test confirms it. - The Tier C out-of-core distance sweep already skips NaN in
_sweep_ring(elev != elev), so it was never affected and the fix correctly leaves it alone.
Checklist
- Algorithm matches reference (GRASS r.viewshed port; NaN cells are NODATA and excluded)
- Implemented backends consistent (numpy + dask Tier B share the CPU sweep; cupy/RTX is a separate path, out of scope)
- NaN handling correct
- Edge cases covered by tests
- Dask chunk boundaries handled (Tier B computes to one in-memory array)
- No premature materialization or unnecessary copies
- Benchmark not needed (bug fix, no perf change)
- README feature matrix N/A (no new function)
- Docstrings accurate (no API change)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2857
What changed
_init_event_listskips NaN cells so they no longer emit ENTERING/CENTER/EXITING events. This removes theValueError: node not foundthat hit when a NaN sat on the observer row to the right of the observer (its EXITING event tried to delete a node that was never inserted)._init_event_listnow returns the event count, and_viewshed_cputrims the unused trailing rows of the pre-allocated event list before sorting. Without the trim, the leftover all-zero rows sort as CENTER events at cell (0, 0) and would mark it visible.INVISIBLE(-1) fill value. That is the sentinel downstream consumers already test for (visibility.pyandexperimental/min_observable_height.pyboth treat!= INVISIBLEas visible), so a NaN cell is never counted as visible.Backends
The fix lives in the CPU sweep, which backs the numpy backend and the dask Tier B path. The cupy/RTX viewshed is a separate implementation and is out of scope here.
Test plan
xfailNaN test into passing tests.test_viewshed.py,test_visibility.py,test_min_observable_height.pyall green.No new public API, so the user-guide notebook and README feature-matrix steps are skipped. No docs change needed; the
viewshedreference entry and signature are unchanged.