Validate n_levels contract in contours()#2899
Merged
Merged
Conversation
n_levels is only consumed on the auto-level branch (levels is None). Add a fail-fast check there: reject non-integers (including bool) with TypeError and values < 1 with ValueError. Explicit-levels callers are unaffected since n_levels is unused for them. Previously n_levels=0/-1 silently returned no contours and n_levels=2.5 leaked a raw numpy TypeError.
brendancol
commented
Jun 3, 2026
brendancol
left a comment
Contributor
Author
There was a problem hiding this comment.
PR Review: Validate n_levels contract in contours()
Blockers (must fix before merge)
None.
Suggestions (should fix, not blocking)
None.
Nits (optional improvements)
- Pre-existing, not from this PR:
xrspatial/tests/test_contour.py:5imports_contours_numpy, which flake8 flags as F401 (unused). It is already unused onmain, so it is out of scope for this change. Flagging it so it does not get attributed to this PR.
What looks good
- Validation lives inside the
if levels is None:block (contour.py:627-636), so it only fires when n_levels is actually consumed. Passing an explicitlevelswith a junkn_levelsis not rejected, which matches the documented contract and is covered bytest_explicit_levels_skip_n_levels_validation. - bool is rejected explicitly even though it is an int subclass (contour.py:632).
np.bool_also lands in the TypeError branch since it is neitherboolnornp.integer. - The type check runs before the
< 1comparison, so the range check never sees a non-int. - TypeError for wrong type and ValueError for out-of-range matches the convention in
terrain_metrics.landformsandbalanced_allocation. - Tests cover 0, -1, 2.5, True, the valid n_levels=1 single-level case, and the explicit-levels skip path.
- Added line is 85 chars, under the repo's 100-char flake8 limit, so no wrap needed.
Checklist
- Algorithm matches reference/paper (n/a, validation only)
- All implemented backends produce consistent results (shared entry point, no backend code touched)
- NaN handling is correct (unchanged)
- Edge cases are covered by tests
- Dask chunk boundaries handled correctly (n/a)
- No premature materialization or unnecessary copies (scalar checks only)
- Benchmark exists or is not needed (not needed)
- README feature matrix updated (contours already listed; no change needed)
- Docstrings present and accurate
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 #2895
What changed
contours()now validatesn_levelson the auto-level branch (whenlevelsis not passed). Non-integers (includingbool) raiseTypeError; values< 1raiseValueError. Both messages name the parameter.if levels is None:block, so callers passing explicitlevelsare unaffected.n_levelsis unused for them and is not validated.n_levelsdocstring documents the contract.Before this,
n_levels=0or-1silently returned no contours, andn_levels=2.5leaked a raw numpyTypeError("'float' object cannot be interpreted as an integer").Backends
Scalar argument validation in the shared
contours()entry point. No backend-specific code paths touched, so numpy / cupy / dask+numpy / dask+cupy all behave the same.Test plan
n_levels=0andn_levels=-1raiseValueErrorn_levels=2.5andn_levels=TrueraiseTypeErrornaming the parametern_levels=1produces exactly one levellevelswithn_levels=0still works (validation skipped)test_contour.pysuite passes (52 tests)