Add support for custom coordinate names #94

huard · 2020-04-23T22:01:59Z

As proposed in #74, this PR uses a dictionary of variable names to indicate which variables hold latitude and longitude coordinates and bounds. If this dictionary is not given, the code uses very basic heuristics based on the CF-Convention to find the coordinates and their bounds. The bounds are then converted into the format required by ESMF.

Note that the conversion from vertices (nx, ny, 4) is probably wrong and needs additional work and testing.

jthielen · 2020-04-23T22:13:57Z

Glad to see this moving forward before a full cf-xarray package is ready to be integrated!

I have one early suggestion: instead of having the basic CF-based check being against the long_name attribute (which is not standardized in the current conventions), have it check for the required units attribute

for latitude, this is one of degrees_north, degree_north, degree_N, degrees_N, degreeN, or degreesN (see http://cfconventions.org/Data/cf-conventions/cf-conventions-1.8/cf-conventions.html#latitude-coordinate).
for longitude, this is one of degrees_east, degree_east, degree_E, degrees_E, degreeE, or degreesE (see http://cfconventions.org/Data/cf-conventions/cf-conventions-1.8/cf-conventions.html#longitude-coordinate).

codecov-io · 2020-04-24T12:43:28Z

Codecov Report

Merging #94 into master will decrease coverage by 0.71%.
The diff coverage is 93.42%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
- Coverage   96.55%   95.83%   -0.72%     
==========================================
  Files           6        6              
  Lines         319      384      +65     
==========================================
+ Hits          308      368      +60     
- Misses         11       16       +5

Impacted Files	Coverage Δ
xesmf/util.py	`94.18% <92.53%> (-5.82%)`	⬇️
xesmf/frontend.py	`95.55% <100.00%> (-0.03%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d2e365...d9b1062. Read the comment docs.

huard · 2020-04-24T12:45:03Z

Done. Just wondering whether this is a one-to-one relationship. Lats and lons have to have those units, but is any variable with those units guaranteed to be --the-- grid coordinate ?

huard · 2020-04-24T13:02:05Z

Another question: there is a potential for confusion between two types of "bounds":

the grid corners required by xESMF (n+1, m+1),
the grid vertices defined in the CF convention (n, 2) or (n, m, 4)

Suggestions about the terminology to use to make this distinction clear ? The code currently uses bounds and boundaries interchangeably.

jthielen · 2020-04-28T20:46:08Z

Just wondering whether this is a one-to-one relationship. Lats and lons have to have those units, but is any variable with those units guaranteed to be --the-- grid coordinate ?

From my reading of the conventions document, while it is not explicitly stated to be one-to-one/uniquely identifying, it is implied since this is how the conventions instruct applications to interpret those coordinate types (and such an instruction wouldn't be very useful if it wasn't uniquely identifying). For example:

Coordinates of latitude with respect to a rotated pole should be given units of degrees, not degrees_north or equivalents, because applications which use the units to identify axes would have no means of distinguishing such an axis from real latitude, and might draw incorrect coastlines, for instance.

stefraynaud

This PR will help users to use xesmf with their own data formats in most cases.
Minor changes are proposed, at least to clarify a few things.

stefraynaud · 2020-05-20T06:44:28Z

xesmf/util.py

+def is_longitude(var):
+    """Return True if variable is a longitude.
+
+    A variable is considered a longitude if its `units` are 'degrees_east' or a synonym.


Yes. That's part of the Cf conventions.
Other criteria from cf paragraph: standard_name and axis attributes, despite the checking with axis attribute may lead to errors with 2D coordinates.

Now filters on

has_units_of_longitude(var) or has_standard_name_of_longitude(var)

stefraynaud · 2020-05-20T06:45:05Z

xesmf/util.py

+    return has_units_of_latitude(var)
+
+
+def has_name_of_longitude(var):


name -> standard_name?

stefraynaud · 2020-05-20T06:54:51Z

xesmf/util.py

+
+def has_name_of_longitude(var):
+    key = "standard_name"
+    return var.attrs.get(key) == "longitude"


Be careful, models with staggered grids use standard_name like "longitude_at_u_location".

Should then this be

var.attrs.get(key).startswith("longitude")

?

Without regexp, I would suggest an equality comparison to "longitude" and, in addition, that the string starts with "longitude_at_".

stefraynaud · 2020-05-20T07:31:47Z

xesmf/util.py

+    np.ndarray, np.ndarray
+      Longitude and latitude arrays.
+    """
+    if var_names is None:


Should it support the case of an incomplete dictionary, for instance with missing lat or missing names for bounds.
If the dict is provided without name for bounds only, lon and lat are found explicitly and bounds are found from CF checking.

Something like

if var_names is None or "lon" not in var_names or "lat" not in var_names:

?

xesmf/frontend.py

stefraynaud · 2020-05-26T12:14:19Z

xesmf/util.py

+
+# TODO: This is probably not true in general, and depends on the convention used, starting point, clockwise, etc.
+def cf_to_esm_bnds_2d(vertices):
+    """Convert 3D CF-compliant bounds to 2D ESM boundaries."""


I'm pretty sure this implementation is not robust. It assumes coordinates are given starting from the upper left corner and going anti-clockwise.

stefraynaud · 2020-05-26T12:22:24Z

xesmf/util.py

+    return var.attrs.get(key) == "longitude"
+
+
+def has_name_of_latitude(var):


A minimal doc? Well, the name is explicit. Perhaps just to say that the input is a datarray.

stefraynaud · 2020-05-26T12:26:29Z

xesmf/util.py

+def cf_to_esm_bnds_1d(vertices):
+    """Convert 2D CF-compliant bounds to 1D ESM boundaries."""
+    v = vertices
+    return np.concatenate((v[:, 0], v[-1:, 1]))


Perhaps add in the docstring that the cell bounds are contiguous.

Done, please check that this is what you meant.

stefraynaud · 2020-05-26T12:29:48Z

xesmf/util.py

+
+
+def cf_lon_lat_bnds(ds):
+    """Return longitude and latitude boundaries.


Maybe add a reminder about the array shapes.

stefraynaud · 2020-05-26T12:30:59Z

xesmf/util.py

+
+    Return
+    ------
+    np.ndarray, np.ndarray


With the shapes?

codecov-commenter · 2020-05-26T14:04:19Z

Codecov Report

Merging #94 into master will increase coverage by 0.59%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master      #94      +/-   ##
==========================================
+ Coverage   96.55%   97.15%   +0.59%     
==========================================
  Files           6        6              
  Lines         319      386      +67     
==========================================
+ Hits          308      375      +67     
  Misses         11       11

Impacted Files	Coverage Δ
xesmf/frontend.py	`95.55% <100.00%> (-0.03%)`	⬇️
xesmf/util.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d2e365...6b67e6c. Read the comment docs.

huard · 2020-05-26T14:40:10Z

Thanks @stefraynaud for the review. The one thing that still bothers me with the PR is the naming convention. There are various terms used that I think we should use more consistently:

bounds
boundaries
vertices
corners

I don't have any actual suggestion to make to clear this up.

stefraynaud · 2020-05-26T14:59:07Z

As for the terms, maybe it can be clarified in two generic steps:

Make sure that the docstrings mentions the shape of arrays.
Clarify the user guide about the possible terms.

huard · 2020-06-05T11:57:37Z

The code to convert nxmx4 bounds to 2D grid corners should make respect this convention. I don't think it does at the moment: https://docs.google.com/document/d/1ueYsYrPob8Gd3HxQVHC0vKfpj_Jjh0iO4Pe70_QLpDk/edit

rabernat · 2020-07-24T13:08:04Z

If this PR is ready to merge, perhaps it can be moved to https://github.com/pangeo-data/xESMF? Unfortunately this has to be done manually.

jthielen · 2020-07-24T13:28:22Z

Also, given that cf-xarray has been released, would it be worth just switching over to depending on that at this point?

rabernat · 2020-07-24T13:30:30Z

We discussed that at the last dev meeting (which you're welcome to join any time; see #98). @huard concluded it might not be worth it in this specific case.

jthielen · 2020-07-24T13:36:11Z

We discussed that at the last dev meeting (which you're welcome to join any time; see #98). @huard concluded it might not be worth it in this specific case.

Sounds good! That's definitely reasonable for it to not be worth it in this case, and I'm glad to hear it was discussed. I'll be using xESMF and cf-xarray in combination for an upcoming project or two, so if there are any sticking points, I'll be sure to put in a PR to help.

dcherian · 2020-07-24T14:53:00Z

it might not be worth it in this specific case.

So this PR was a main reason I wrote it up, so that logic to figure lat/lon didn't have to be written 5-10x over.

The logic is pretty self contained so cf-xarray's utility functions should be easily copy/pastable.

rabernat · 2020-07-24T15:02:35Z

FWIW, I also favor outsourcing this to cf-xarray. However, I am not the author of the PR and so must defer to his judgement. Perhaps we could merge this as is, with good test coverage, and then refactor to use cf-xarray in a future PR without touching the tests?

stefraynaud · 2020-07-27T07:46:04Z

Historically, this PR came after PR #73.
This latter was proposed to make the job of cf-xarray, and it has been chosen to keep only #94 because it simpler implementation.
Now that cf-xarray is here, a new PR must be proposed to merge to two capabilities:

Let the user specify the lon/lat names.
Find the lon/lat names according to CF conventions.

This means: if the names are not specified, then search for them according to the default names, and if not found, use cf-xarray,

huard · 2020-08-03T16:41:53Z

One concern was weighing the maintenance costs of adding a dependency versus maintaining code. I thought the code was so basic, it was not worth an additional dependency. As xesmf and cf-xarray both evolve, I suspect the tradeoff will lean in favor of relying on cf-xarray.

I think this PR still has an issue with the conversion from 2D vertices to grid corners however, and I'm short on time to investigate this. Is this in-scope for cf-xarray ?

dcherian · 2020-08-03T18:13:00Z

One concern was weighing the maintenance costs of adding a dependency versus maintaining code. I thought the code was so basic, it was not worth an additional dependency. As xesmf and cf-xarray both evolve, I suspect the tradeoff will lean in favor of relying on cf-xarray.

another approach is to make the code in cf-xarray easily copy-pastable which it may be already.

I think this PR still has an issue with the conversion from 2D vertices to grid corners however, and I'm short on time to investigate this. Is this in-scope for cf-xarray ?

Sure. I added a function to guess 1D bounds (edges) already: https://cf-xarray.readthedocs.io/en/latest/generated/xarray.Dataset.cf.add_bounds.html#xarray.Dataset.cf.add_bounds. We can add a 2D version too.

huard · 2020-08-03T18:17:28Z

Great ! If xesmf can rely on cf-xarray for the variable identification and corner grid calculations, I think it would make sense to ditch this PR in favor of a new one using cf-xarray as a dependency.

…eights Set add_nans_to_weights as default as discussed in JiaweiZhuang#56

huard added 5 commits April 23, 2020 01:03

building support for detection of lat and lon from cf attributes

25f615f

fix typos

3f45859

all tests pass

68ca197

all tests pass

c8cb812

raised test code coverage

a7c0337

identify lat, lon using units

d9b1062

stefraynaud mentioned this pull request May 20, 2020

Handle different grid coordinate formats and naming #74

Open

huard mentioned this pull request May 20, 2020

Add some CF support for lon and lat #73

Closed

stefraynaud approved these changes May 26, 2020

View reviewed changes

implemented suggested changes from review

d2d5887

added test for get_lon_lat_bnds

e3fe0ec

accept longitude_at_... standard name

6b67e6c

huard mentioned this pull request Sep 2, 2020

Use cf-xarray to parse variable names and compute grid corners from bounds pangeo-data/xESMF#16

Closed

huard closed this May 7, 2021

raphaeldussin added a commit to raphaeldussin/xESMF that referenced this pull request Jul 21, 2021

Merge pull request JiaweiZhuang#94 from sol1105/Default_add_nans_to_w…

abec0d3

…eights Set add_nans_to_weights as default as discussed in JiaweiZhuang#56

		return has_units_of_latitude(var)


		def has_name_of_longitude(var):

		return var.attrs.get(key) == "longitude"


		def has_name_of_latitude(var):



		def cf_lon_lat_bnds(ds):
		"""Return longitude and latitude boundaries.

Add support for custom coordinate names #94

Add support for custom coordinate names #94

Conversation

huard commented Apr 23, 2020

jthielen commented Apr 23, 2020

codecov-io commented Apr 24, 2020 • edited Loading

Codecov Report

huard commented Apr 24, 2020

huard commented Apr 24, 2020

jthielen commented Apr 28, 2020

stefraynaud left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented May 26, 2020 • edited Loading

Codecov Report

huard commented May 26, 2020

stefraynaud commented May 26, 2020

huard commented Jun 5, 2020

rabernat commented Jul 24, 2020

jthielen commented Jul 24, 2020

rabernat commented Jul 24, 2020

jthielen commented Jul 24, 2020 • edited Loading

dcherian commented Jul 24, 2020

rabernat commented Jul 24, 2020

stefraynaud commented Jul 27, 2020

huard commented Aug 3, 2020

dcherian commented Aug 3, 2020

huard commented Aug 3, 2020

codecov-io commented Apr 24, 2020 •

edited

Loading

codecov-commenter commented May 26, 2020 •

edited

Loading

jthielen commented Jul 24, 2020 •

edited

Loading