Add API extending xgcm vertical regridding #388

jasonb5 · 2022-11-11T00:57:15Z

Description

Closes [Feature]: Add vertical regridding #45

Adds vertical regridding via xgcm.

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
My changes generate no new warnings
Any dependent changes have been merged and published in downstream modules

If applicable:

I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass with my changes (locally and CI/CD build)
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
I have noted that this is a breaking change for a major release (fix or feature that would cause existing functionality to not work as expected)

jasonb5 · 2022-11-11T01:18:33Z

Tasks left:

Add documentation
Add tests
Add example notebook
Determine method to derive grid coord, do we have a reliable way to tell position of data point e.g. left, center, right, inner, outer, maybe via cf_xarray. If not we'll need to have the user pass this information.
Add method to derive target_data if formula_terms is present in vertical coordinate and variables are present in dataset otherwise rely on user to pass variable.

The current plan for handling target_data is to provide an argument that can be one of the following.

xr.DataArray is used directly
str is used to reference variable in dataset
None will trigger auto derive

If auto derive is selected the vertical coordinate attributes will be checked for formula_terms. This will be used to collect all variables required. These variables will be processed using these formulas to derive the target_data. If auto derive fails the user will be presented with an error alerting them that they'll need to manually provide the target_data.

codecov-commenter · 2022-11-29T18:39:51Z

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (c70827a) 100.00% compared to head (50f01d9) 100.00%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff            @@
##              main      #388   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           14        15    +1     
  Lines         1428      1517   +89     
=========================================
+ Hits          1428      1517   +89

Impacted Files	Coverage Δ
xcdat/axis.py	`100.00% <100.00%> (ø)`
xcdat/regridder/__init__.py	`100.00% <100.00%> (ø)`
xcdat/regridder/accessor.py	`100.00% <100.00%> (ø)`
xcdat/regridder/base.py	`100.00% <100.00%> (ø)`
xcdat/regridder/grid.py	`100.00% <100.00%> (ø)`
xcdat/regridder/regrid2.py	`100.00% <100.00%> (ø)`
xcdat/regridder/xesmf.py	`100.00% <100.00%> (ø)`
xcdat/regridder/xgcm.py	`100.00% <100.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

tomvothecoder · 2023-02-01T17:19:26Z

Hey @jasonb5, I threw the notes from the past few meetings here since they are action items. Feel free to check-box or update as needed.

Action Items from Meetings (11/2022 - 1/2023)

Detecting target variable in the metadata
- Example regrid depth variable -> pass target levels to transform
- Pass name (derived and added to dataset), or pass DataArray itself of variable
  - Same process of deriving variables as our other methods
  - Seems like a common workflow, based on
  - Variable like temp on grid you don’t like, then regrid
Parametric - detect CF formula attribute to generate target output (add as a separate PR if we want this feature)
- CF metadata should be present too
- If no formulas are provided, parse internal formula via CF convention - formula_terms, maps to variables in the dataset
- If we can’t parse the user supplied formula or find the formula_terms metadata, throw error
Parse info from pressure var metadata (cf_array to interpret metadata, pull out formulas) – cdat requires explicit passing of arrays (not intuitive)
Look into detecting coordinate positions internally
- e.g,. outer, left, right
- If can't be determined, error is raised and user has to pass info depending on vertical regridding method (conservative requires multiple points)
deal with spatially varying vertical elements
- Look how xgcm handles missing values (use nans for outputs, not sure how they affect calculations right now)
- depending on how missing values are handled, might need to manipulate data before passing into library
Find test datasets for ocean (try https://github.com/pochedls/xsearch/)
- ta/cfmon
- dpaths = xs.findPaths('historical', 'ta', 'mon', cmipTable='cfMon')
- thetao = dpaths = xs.findPaths('historical', 'thetao', 'mon')
- Amon: cl, cli, clw on model levels
- Amon: ta, hur, hurs… on pressure levels

tomvothecoder · 2023-05-24T21:55:12Z

xcdat/regridder/grid.py

+def create_grid(**kwargs: CoordOptionalBnds) -> xr.Dataset:
+    """Creates a grid from coordinate mapping.

    Parameters
    ----------
-    lat : Union[np.ndarray, xr.DataArray]
-        Array of latitude values.
-    lon : Union[np.ndarray, xr.DataArray]
-        Array of longitude values.
-    lat_bnds : Optional[Union[np.ndarray, xr.DataArray]]
-        Array of bounds for latitude values.
-    lon_bnds : Optional[Union[np.ndarray, xr.DataArray]]
-        Array of bounds for longitude values.
+    **kwargs : CoordOptionalBnds
+        Mapping of coordinate name and data with optional bounds. See
+        :py:data:`xcdat.axis.VAR_NAME_MAP` for valid coordinate names.


Hey Jason, my suggestion here was to make the parameters for create_grid more explicit.

In my opinion, using **kwargs over an explicit args/kwargs can create confusion and reduce readability since there isn't set number of clearly defined parameters for the function. Here is a post a read about **kwargs: https://softwareengineering.stackexchange.com/a/294486

I would suggest keeping the args and kwargs, then extending as needed.

@pochedls @chengzhuzhang @lee1043 @tomvothecoder
How do we feel about the following function to replace the current create_grid.

def create_grid(x: GridCoord, y: GridCoord, z: GridCoord) -> xr.Dataset:

Where GridCoord could be any of the following forms.

(name, data) (name, data, attrs) (name, data, bnds) (name, data, bnds, attrs)

From what I understand, this is a tuple where:

name = string

data = array of floats

attrs = dictionary mapping str to str

bnds = 2d array of floats

Is this correct?

@tomvothecoder Yes that's correct. The data and bnds values can be a list, ndarray or DataArray (1d/2d respectively).

Thanks for confirming. I am curious about the benefits you see in using a tuple of sub-arguments over an xr.DataArray? I'm pretty sure this was discussed with the team during our meetings, but I didn't really weigh in because there's a lot to track!

Yeah – I have instances where I have a numpy array and want to create a DataArray (with correct axis information). I think this is kind of common. @mzelinka – were you saying that you have cases where it would be helpful to generate a new axis (with correct metadata/attributes)?

What if we include a create_axis() function (similar to the one at bottom)?

I can see a function like create_axis making the process of creating xr.DataArray axes faster and less prone to error. Maybe this can be scoped out in another GitHub issue since I don't think this PR is dependent on this possible feature?

I had a few questions (for myself too):

When and how often might users want/need to create axes instead of using the ones in the file?

How do we decide what is considered the most sensible default CF attributes for each axis? Xarray sets some like calendar="standard" for the time axis, and we set a few others like units for latitude/longitude.

If we decide this makes sense to implement, we should consider calling it create_dim since we have get_dim_keys() and get_dim_coords().

I also like the idea of having a static dictionary to store default CF attributes. I think cf-xarray and/or xarray might already have this implemented. We have default attrs set in various parts of the code-base (e.g., calendar=standard for the time axis). It'll be cleaner to have a single reference point for default attrs.

# axis.py CF_DEFAULT_ATTRS = { "lat": '{'units': 'degrees_north', 'axis': 'Y', 'long_name': 'latitude', 'standard_name': 'latitude' }, .... }

@pochedls @tomvothecoder Lets move this discussion to the new issue.

This update (#496) will go in a new PR after this PR is merged right?

That's correct, this will be resolved in a separate PR.

jasonb5 · 2023-06-02T00:22:48Z

@jasonb5 – I was working on opening a ticket about this issue and I realize that the bounds are not dropped on the v0.5.0 release – it is happening on this branch. Here's a MCVE (fixed from above):

import xcdat as xc
import numpy as np
# define target grid
nlat = np.arange(-88.75, 90, 2.5)
nlon = np.arange(1.25, 360, 2.5)
ngrid = xc.create_grid(lat=nlat, lon=nlon)
# define / open dataset
dspath = '/p/css03/esgf_publish/CMIP6/CMIP/CNRM-CERFACS/CNRM-CM6-1/historical/r3i1p1f2/Omon/thetao/gn/v20190125/thetao_Omon_CNRM-CM6-1_historical_r3i1p1f2_gn_200001-201412.nc'
# another example path (for the atmospheric grid): 
# /p/css03/esgf_publish/CMIP6/CMIP/NCAR/CESM2/amip/r1i1p1f1/Amon/ta/gn/v20190218/ta_Amon_CESM2_amip_r1i1p1f1_gn_195001-201412.nc
ds = xc.open_mfdataset(dspath)
# subset for minimal example
ds = ds.isel(time=[0])
# horizontal regridding
dsg = ds.regridder.horizontal('thetao', ngrid, method='conservative_normed')
dsg.lev_bnds

I see what's going on here, when horizontal regridding was first implemented the method would call ds.bounds.add_missing_bounds(axes=["X", "Y"]), with vertical regridding it's calling ds.bounds.add_missing_bounds(axes=["X", "Y", "Z", "T"]).

Should we even be adding bounds outside of what was generated during the regridding process?

If we do want the methods to fill out the bounds should it be for all dimensions or only the dimensions involved in the process e.g. horizontal generates only "X", "Y"?

One other potential issue only associated with this branch. When I exit iPython I get this warning:

[WARNING] yaksa: 10 leaked handle pool objects

I haven't been able to reproduce this yet, I'm not sure what is using the yaksa package.

pochedls · 2023-06-02T16:09:51Z

I see what's going on here, when horizontal regridding was first implemented the method would call ds.bounds.add_missing_bounds(axes=["X", "Y"]), with vertical regridding it's calling ds.bounds.add_missing_bounds(axes=["X", "Y", "Z", "T"]).

Should we even be adding bounds outside of what was generated during the regridding process?

If we do want the methods to fill out the bounds should it be for all dimensions or only the dimensions involved in the process e.g. horizontal generates only "X", "Y"?

One other potential issue only associated with this branch. When I exit iPython I get this warning:

[WARNING] yaksa: 10 leaked handle pool objects

I haven't been able to reproduce this yet, I'm not sure what is using the yaksa package.

The original dataset includes lev_bounds. These should be copied over after horizontal regridding (since the vertical bounds don't change during horizontal regridding). These bounds are still included as an attribute after horizontal regridding (i.e., dsg.lev.attrs includes "bounds": "lev_bounds"), even though bounds themselves are not in dsg.

So – we shouldn't generate the bounds, but I do think we should carry over the original bounds (that are unaffected by horizontal regridding).

I'm not sure how much we need to worry about yaksa, I get the warning on main branch, too (even after re-creating the dev environment).

jasonb5 · 2023-06-06T19:22:49Z

@pochedls This logic sounds good, I'll update accordingly.

tomvothecoder

Hi Jason, here is my final review. The suggestions are mostly renaming things.

docs/examples/regridding-vertical.ipynb

tomvothecoder · 2023-06-15T18:02:21Z

tests/test_regrid.py

+        mock_regridder = mock.MagicMock()
+        mock_regridder.return_value.vertical.return_value = "output data"
+
+        mock_data = mock.MagicMock()
+
+        with mock.patch.dict(accessor.VERTICAL_REGRID_TOOLS, {"xgcm": mock_regridder}):
+            output = self.ac.vertical("ts", mock_data, tool="xgcm", target_data=None)
+
+        assert output == "output data"


The use of MagicMock is neat here. For those interested (probably just me), the xgcm vertical regridder object is being mocked (mock_regridder) and set as the xgcm dictionary value in accessor.VERTICAL_REGRID_TOOLS. The return value of mock_regridder is set to "output_data", which is being tested in the assertion on line 1088. This test is to check if the correct vertical regridding tool is being referenced.

The actual result of the regridder object is not being tested (hence "output_data") because it is already tested through class TestXGCMRegridder.

tomvothecoder · 2023-06-15T18:06:26Z

xcdat/regridder/accessor.py

            )

        regridder = regrid_tool(self._ds, output_grid, **options)
        output_ds = regridder.horizontal(data_var, self._ds)

        return output_ds
+
+    def vertical(


Based on #501, are we also considering calling this vertical_xgcm and not having a general wrapper with tool="xgcm"?

That's correct, I'll address it in the next PR.

tomvothecoder · 2023-06-15T18:09:54Z

xcdat/regridder/grid.py

+def create_grid(**kwargs: CoordOptionalBnds) -> xr.Dataset:
+    """Creates a grid from coordinate mapping.

    Parameters
    ----------
-    lat : Union[np.ndarray, xr.DataArray]
-        Array of latitude values.
-    lon : Union[np.ndarray, xr.DataArray]
-        Array of longitude values.
-    lat_bnds : Optional[Union[np.ndarray, xr.DataArray]]
-        Array of bounds for latitude values.
-    lon_bnds : Optional[Union[np.ndarray, xr.DataArray]]
-        Array of bounds for longitude values.
+    **kwargs : CoordOptionalBnds
+        Mapping of coordinate name and data with optional bounds. See
+        :py:data:`xcdat.axis.VAR_NAME_MAP` for valid coordinate names.


This update (#496) will go in a new PR after this PR is merged right?

tomvothecoder · 2023-06-15T18:15:41Z

xcdat/regridder/base.py

+Coord = Union[np.ndarray, xr.DataArray]
+
+CoordOptionalBnds = Union[Coord, Tuple[Coord, Coord]]
+

 def preserve_bounds(


Do you think this function will be used publicly, or should it be private (_preserve_bounds())?

I'm guessing it won't be used publicly, I'll rename it .

tomvothecoder · 2023-06-15T18:21:45Z

xcdat/regridder/base.py

+    output_ds: xr.Dataset,
+    output_grid: xr.Dataset,
+    input_ds: xr.Dataset,
+    ignore_dims: List[CFAxisKey],


Suggested change

output_ds: xr.Dataset,

output_grid: xr.Dataset,

input_ds: xr.Dataset,

ignore_dims: List[CFAxisKey],

input_ds: xr.Dataset,

output_grid: xr.Dataset,

output_ds: xr.Dataset,

ignore_dims: List[CFAxisKey],

It's more intuitive to me to have the input params input_ds and output_grid first since they contain the bounds info, which are copied/preserved to the output_ds.

I'm thinking input -> output basically. Not a big deal though, especially if this should be a private function (my previous comment).

xcdat/regridder/xgcm.py

Co-authored-by: Tom Vo <tomvothecoder@gmail.com>

tomvothecoder · 2023-06-15T20:59:36Z

🎉🎊

Adds initial vertical regridding via xgcm

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

fd34905

jasonb5 changed the title ~~Adds initial vertical regridding via xgcm~~ Adds vertical regridding Nov 11, 2022

Merge remote-tracking branch 'upstream/main' into adds_vertical_regrid

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

e997d76

jasonb5 added 22 commits December 1, 2022 10:24

Refactors create_grid to be more flexible

da38684

Fixes typings

5795a45

Adds some initial documentation

724c889

Fixes grid method to handle vertical grids

abcba85

Removes unused test

3bbe0b7

Resolves type issue

5edcb27

Temporary pin esmpy

3b30e64

Updates testing

cc83984

Fixes type errors

6e112c2

Fixes using input grid's Z coordinate

b001c9e

Adds method to derive coord positions for regridding

246c094

Fixes failing pre-commit

7988f9d

Updates tests

e242e7a

Fixes typing for method

be862df

Adds default method

d1ed882

Fixes target data argument

1324cea

Fixes black and type errors

d32cd74

Exposes extra options

ae5c3aa

Adds initial example notebook

00569a6

Adds initial vertical regridding notebook

1c31300

Fixes loaded kernel

9820113

Reworks description of XGCMRegridder

f106f67

jasonb5 added 2 commits February 15, 2023 13:17

Updates error message

d73b763

Updates vertical regridding example

a482dea

tomvothecoder reviewed May 24, 2023

View reviewed changes

Merge branch 'main' into adds_vertical_regrid

ddf423f

jasonb5 mentioned this pull request Jun 6, 2023

[Feature]: Improve create_grid function #496

Closed

pochedls mentioned this pull request Jun 6, 2023

[Bug]: Regridding error from differing time dimensions #497

Closed

tomvothecoder changed the title ~~Adds vertical regridding~~ Add extension of xgcm vertical regridding Jun 7, 2023

Removes generating bounds from horizontal/vertical regrid methods

e78f8c8

tomvothecoder changed the title ~~Add extension of xgcm vertical regridding~~ Add API extending xgcm vertical regridding Jun 8, 2023

jasonb5 and others added 7 commits June 8, 2023 15:07

Fixes preserve_bounds function

5f94530

Adds XCGMRegridder to module

de08dee

Adds missing argument to docstring

85c33e7

Cleans up to documentation

ec84a03

Fixes setting the attributes on the output DataArray

753bded

Merge branch 'main' into adds_vertical_regrid

48e4253

Merge branch 'main' into adds_vertical_regrid

e75f1bc

tomvothecoder reviewed Jun 15, 2023

View reviewed changes

jasonb5 and others added 7 commits June 15, 2023 12:31

Apply suggestions from code review

f32db3d

Co-authored-by: Tom Vo <tomvothecoder@gmail.com>

Update docs/examples/regridding-vertical.ipynb

fadd95b

Co-authored-by: Tom Vo <tomvothecoder@gmail.com>

Fixes missed _grid_coords renamed to _grid_positions

e0d6fca

Changes preserve_bounds visibility

111232d

Rearranges _preserve_bounds arguments

c406642

Updates preserve_bounds test

06b17b3

Fixes black formatting

50f01d9

tomvothecoder approved these changes Jun 15, 2023

View reviewed changes

jasonb5 merged commit be3244e into xCDAT:main Jun 15, 2023

jasonb5 deleted the adds_vertical_regrid branch June 15, 2023 20:58

pochedls mentioned this pull request Jun 22, 2023

[DevOps]: xgcm warnings polluting test suite and yaksa leaking handlers #513

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add API extending xgcm vertical regridding #388

Add API extending xgcm vertical regridding #388

jasonb5 commented Nov 11, 2022

jasonb5 commented Nov 11, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading

tomvothecoder commented Feb 1, 2023 •

edited

Loading

tomvothecoder May 24, 2023

jasonb5 May 25, 2023

tomvothecoder May 26, 2023

jasonb5 May 30, 2023

tomvothecoder May 30, 2023

pochedls Jun 2, 2023

tomvothecoder Jun 6, 2023 •

edited

Loading

jasonb5 Jun 6, 2023

tomvothecoder Jun 15, 2023

jasonb5 Jun 15, 2023

jasonb5 commented Jun 2, 2023

pochedls commented Jun 2, 2023

jasonb5 commented Jun 6, 2023

tomvothecoder left a comment

tomvothecoder Jun 15, 2023

tomvothecoder Jun 15, 2023

jasonb5 Jun 15, 2023

tomvothecoder Jun 15, 2023

tomvothecoder Jun 15, 2023

jasonb5 Jun 15, 2023

tomvothecoder Jun 15, 2023

tomvothecoder commented Jun 15, 2023

Add API extending xgcm vertical regridding #388

Add API extending xgcm vertical regridding #388

Conversation

jasonb5 commented Nov 11, 2022

Description

Checklist

jasonb5 commented Nov 11, 2022 • edited Loading

codecov-commenter commented Nov 29, 2022 • edited Loading

Codecov Report

tomvothecoder commented Feb 1, 2023 • edited Loading

Action Items from Meetings (11/2022 - 1/2023)

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

tomvothecoder Jun 6, 2023 • edited Loading

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

jasonb5 commented Jun 2, 2023

pochedls commented Jun 2, 2023

jasonb5 commented Jun 6, 2023

tomvothecoder 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

tomvothecoder commented Jun 15, 2023

jasonb5 commented Nov 11, 2022 •

edited

Loading

codecov-commenter commented Nov 29, 2022 •

edited

Loading

tomvothecoder commented Feb 1, 2023 •

edited

Loading

tomvothecoder Jun 6, 2023 •

edited

Loading