Skip to content

Document actual return types of vertical datum functions (#3097)#3134

Open
brendancol wants to merge 3 commits into
mainfrom
deep-sweep-api-consistency-reproject-2026-06-09-02
Open

Document actual return types of vertical datum functions (#3097)#3134
brendancol wants to merge 3 commits into
mainfrom
deep-sweep-api-consistency-reproject-2026-06-09-02

Conversation

@brendancol

Copy link
Copy Markdown
Contributor

Closes #3097

The five vertical functions in xrspatial/reproject/_vertical.py described their return value as "same type as input", which is wrong: DataArray input returns a plain ndarray, and the four conversion wrappers return numpy scalars for scalar input (only geoid_height converts back to Python float).

  • Returns sections now state what actually comes back: geoid_height gives a Python float for scalar input, otherwise an ndarray shaped like the inputs; the wrappers pass height/depth through np.asarray, so scalar in gives a numpy scalar, array-like or DataArray in gives an ndarray.
  • New TestVerticalReturnTypes pins those types so a future behaviour change has to update the docs too.
  • One commit also carries the api-consistency sweep state CSV update for the reproject module.

Docs-only change at runtime; no behaviour modified. Making these functions round-trip DataArrays would be a separate feature decision, noted in the issue.

Test plan:

  • New return-type pin tests pass.
  • flake8 clean on the touched file.

@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: Document actual return types of vertical datum functions (#3097)

Blockers

None.

Suggestions

None.

Nits

  • The same three-line np.asarray explanation is repeated in four Returns sections (_vertical.py). Numpy-style docs favour explicit over DRY, so this is fine as-is; flagging only in case a shared "Notes" section is preferred later.

What looks good

  • Each documented claim matches the code: geoid_height does float(out[0]) on the scalar path and out.reshape(np.shape(lon)) otherwise, and the wrappers return np.asarray(height) +/- N, so the new wording ("numpy scalar for scalar input, plain ndarray otherwise, DataArray coords not carried through") is verifiably accurate.
  • TestVerticalReturnTypes pins the types with type(...) is checks rather than isinstance, so a silent change from float to np.float64 (or ndarray to DataArray) fails loudly and forces a doc update.
  • No runtime behaviour touched; the diff is Returns sections plus tests.

Checklist

  • Docstrings present and accurate (verified against the implementation)
  • Edge cases: scalar, 1-D array, and DataArray inputs all covered by the pin tests
  • No behaviour change, so backend parity and NaN handling are unaffected
  • Benchmark not applicable
  • README feature matrix not applicable

@github-actions github-actions Bot added the performance PR touches performance-sensitive code label Jun 10, 2026
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 functions: Returns docstrings claim 'same type as input' but ndarray comes back

1 participant