Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Error in docstring of vertices_to_bounds #326

Closed
rcaneill opened this issue Apr 5, 2022 · 1 comment · Fixed by #329
Closed

Error in docstring of vertices_to_bounds #326

rcaneill opened this issue Apr 5, 2022 · 1 comment · Fixed by #329
Assignees
Labels
help wanted Extra attention is needed

Comments

@rcaneill
Copy link
Contributor

rcaneill commented Apr 5, 2022

There is an error in the docstring of vertices_to_bounds for the parameter vertices. The docstring says that it must be of shape (N, 2) or (N, M, 4), while in reality it must be (N+1,) (for the 1-D case, I guess (N+1, M+1) for the 2D case?).
Below is the line in the docstring (line 126).

def vertices_to_bounds(
vertices: DataArray, out_dims: Sequence[str] = ("bounds", "x", "y")
) -> DataArray:
"""
Convert vertices to CF-compliant bounds. There are 2 covered cases:
- 1D coordinates, with vertices of shape (N+1,),
converted to bounds of shape (N, 2)
- 2D coordinates, with vertices of shape (N+1, M+1).
converted to bounds of shape (N, M, 4).
Parameters
----------
vertices : DataArray
The bounds to convert. Must be of shape (N, 2) or (N, M, 4).

Minimal example

import numpy as np
import xarray as xr
import cf_xarray
print(cf_xarray.vertices_to_bounds(xr.DataArray(np.arange(5)), ('bounds', 'dim_0')))

prints

<xarray.DataArray (bounds: 2, dim_0: 4)>
array([[0, 1, 2, 3],
       [1, 2, 3, 4]])
Dimensions without coordinates: bounds, dim_0

which is what we expect.

Proposed solution

I can correct the docstring in a PR. I however don't understand the 2D case, so if anyone can guide me on this I'll be happy.

@aulemahal
Copy link
Contributor

Indeed you are right I think. The first part of the docstring is correct, but the "Parameters" section is wrong. I guess it could simply read: "The vertices to convert." or, for completeness, " The vertices to convert. Must be of shape (N+1,) or (N + 1, M + 1). ".

This distinction between "bounds" and "vertices" is not universal. I've seen "bounds" used for what we call here "vertices", but our definition of bounds is closest to CF, which is what this is all about.

Let's say we have a rectangular grid. We might have lat and lon giving the coordinates of the centers. Then, "Vertices" are two new coordinates giving the corners. If the grid is regular, lat, lon and the corresponding vertices are 1D, but with a rotated grid (for example) they are 2D.

@dcherian dcherian added the help wanted Extra attention is needed label Apr 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants