Skip to content

Name the calling function in the unit-mismatch warning (#2782)#3036

Merged
brendancol merged 1 commit into
mainfrom
issue-2782
Jun 8, 2026
Merged

Name the calling function in the unit-mismatch warning (#2782)#3036
brendancol merged 1 commit into
mainfrom
issue-2782

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #2782.

The decision the issue asked for (whether aspect() should call warn_if_unit_mismatch on its planar branch) was already made in #2839: aspect's planar path calls the helper. The leftover footgun is that the shared warning text was hard-coded to say "before calling slope", so an aspect() caller was told to fix a slope call they never made.

  • Add an optional func_name argument to warn_if_unit_mismatch so the message names the operation the user actually called. Default stays slope, so slope's wording is unchanged.
  • Have aspect() pass func_name='aspect'.
  • Extend the message to mention the anisotropy effect called out in the issue: away from the equator the degree-to-meter ratio differs between the x and y axes, which skews the computed aspect direction.

This is a warning-message change in a private helper. No public API, no new function, and no backend support change, so it applies to all backends equally (the warning fires in the shared planar dispatch before backend selection).

Test plan:

  • warn_if_unit_mismatch names slope by default and aspect when passed func_name='aspect' (test_utils.py)
  • aspect() planar path emits a warning that names aspect (test_aspect.py)
  • Existing slope/aspect/utils warning tests still pass (message stem unchanged)

aspect() already calls warn_if_unit_mismatch on its planar branch (added
in #2839), but the shared warning text was hard-coded to say "before
calling `slope`". An aspect() caller was told to fix a slope call they
never made.

Add an optional func_name argument to warn_if_unit_mismatch so the
message names the operation the user actually invoked, and have aspect
pass func_name='aspect'. The default stays 'slope', so slope's message
is unchanged. The text now also notes the anisotropy effect from the
issue: away from the equator the degree-to-meter ratio differs between
the x and y axes, which skews aspect.

Tests check the message names slope by default and aspect when called
from aspect, at both the helper level and through aspect().
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 8, 2026

@brendancol brendancol left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR Review: Name the calling function in the unit-mismatch warning (#2782)

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • utils.py:1005-1006: the message still opens with "Slope/aspect operations expect...", and with func_name='aspect' it then closes with "before calling aspect", so the reader sees both "Slope/aspect" and "aspect" in one warning. It reads fine and is arguably more informative, so leaving it is reasonable. Dropping the "/aspect" and leaning on func_name would tighten it, but that is cosmetic.

What looks good

  • Default func_name='slope' keeps slope's existing message byte-for-byte, so no current behavior changes.
  • The added anisotropy sentence matches the issue rationale: the x/y degree-to-meter ratio diverges away from the equator.
  • Tests cover the helper default, the helper override, and the aspect() integration path. The "appears to have coordinates in degrees" stem is unchanged, so pre-existing match-based tests keep passing.
  • The warning fires in the shared planar dispatch before backend selection, so all four backends behave identically. No dispatch change needed.

Checklist

  • Algorithm matches reference: n/a (warning-text change)
  • Backends consistent: yes, warning is pre-dispatch
  • NaN handling: unchanged
  • Edge cases covered by tests: yes for the warning path
  • Dask chunk boundaries: unchanged
  • No premature materialization: n/a
  • Benchmark: not needed
  • README feature matrix: not needed (private helper, no API change)
  • Docstrings present and accurate: yes, Parameters added for func_name

@brendancol brendancol merged commit 52e11c9 into main Jun 8, 2026
8 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consider unit-mismatch warning for aspect() planar path

1 participant