Skip to content

reproject: rename vertical CRS kwargs to source_/target_vertical_crs (#2613)#2626

Merged
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-reproject-2026-05-29
May 29, 2026
Merged

reproject: rename vertical CRS kwargs to source_/target_vertical_crs (#2613)#2626
brendancol merged 2 commits into
mainfrom
deep-sweep-api-consistency-reproject-2026-05-29

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2613

  • Renames src_vertical_crs / tgt_vertical_crs to source_vertical_crs / target_vertical_crs so the vertical-datum kwargs match the source_crs / target_crs spelling already used in the same signature.
  • Keeps the old names working through a deprecation shim: passing one emits a DeprecationWarning, and passing both the old and new spelling for the same side raises TypeError.
  • Updates the docstring and migrates the existing vertical-shift tests to the new names.

Backend coverage: the signature is shared across backends (numpy, cupy, dask+numpy, dask+cupy); verified the new and deprecated names on numpy and cupy entry points.

Test plan:

  • pytest xrspatial/tests/test_reproject.py::TestVerticalShift (14 passed)
  • cupy smoke test: new names and deprecated-alias warning both produce identical output

…2613)

Align the vertical-datum kwargs with the source_crs/target_crs spelling
used elsewhere in the signature. Old names keep working through a
deprecation shim that warns and rejects passing both spellings at once.

- Add source_vertical_crs / target_vertical_crs params and docstring
- Keep src_vertical_crs / tgt_vertical_crs as deprecated aliases
- Migrate existing tests to the new names; add back-compat tests
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 29, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: reproject vertical CRS kwarg rename

Blockers (must fix before merge)

None.

Suggestions (should fix, not blocking)

None.

Nits (optional improvements)

  • Passing the deprecated name with an explicit None (e.g. src_vertical_crs=None) still emits the DeprecationWarning, since the shim treats any non-sentinel value as "the old name was used" (_resolve_deprecated_vertical_kwarg in reproject/__init__.py). That is defensible: the caller did type the old name.

What looks good

  • The sentinel-based shim tells "not passed" apart from an explicit None, so the old and new defaults stay independent.
  • Passing both spellings for one side raises TypeError, and the new test covers it.
  • stacklevel=3 points the warning at the user's call site rather than the internal helper.
  • The new and old names were verified on both the numpy and cupy entry points and produce identical output.
  • No other call sites in xrspatial, docs, or examples use the old names, so the rename is self-contained.

Checklist

  • Algorithm unchanged (signature/shim only)
  • Implemented backends consistent (numpy + cupy verified)
  • NaN handling unchanged
  • Edge cases covered: deprecation warning, both-names conflict
  • No premature materialization
  • Benchmark not needed (no compute change)
  • README feature matrix not applicable
  • Docstrings updated for new and deprecated params

@brendancol brendancol merged commit 6bbf597 into main May 29, 2026
7 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.

reproject(): vertical CRS params use src_/tgt_ while horizontal use source_/target_

1 participant