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

Masking with conservative_normed #82

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Masking with conservative_normed #82

wants to merge 7 commits into from

Conversation

jhamman
Copy link

@jhamman jhamman commented Feb 20, 2020

@JiaweiZhuang - I'm reviving this conversation from #22. Where do you fall on this issue now?

@@ -70,6 +70,10 @@ def esmf_grid(lon, lat, periodic=False):
Periodic in longitude? Default to False.
Only useful for source grid.

mask : 2D numpy array, optional
Grid mask. Follows SCRIP convention where 1 is unmasked and 0 is
masked.
Copy link

Choose a reason for hiding this comment

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

Mention what dimensions should mask be. E.g. (Nlon, Nlat).

Copy link

Choose a reason for hiding this comment

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

If that is the ESMF convention as well, I'd replace SCRIP by ESMF to avoid confusion.

@@ -52,7 +52,7 @@ def warn_lat_range(lat):
warnings.warn("Latitude is outside of [-90, 90]")


def esmf_grid(lon, lat, periodic=False):
def esmf_grid(lon, lat, periodic=False, mask=None):
'''
Create an ESMF.Grid object, for contrusting ESMF.Field and ESMF.Regrid
Copy link

Choose a reason for hiding this comment

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

Suggested change
Create an ESMF.Grid object, for contrusting ESMF.Field and ESMF.Regrid
Create an ESMF.Grid object, for constructing ESMF.Field and ESMF.Regrid.

# See https://github.com/NCPP/ocgis/blob/61d88c60e9070215f28c1317221c2e074f8fb145/src/ocgis/regrid/base.py#L391-L404
if mask is not None:
grid_mask = np.swapaxes(mask.astype(np.int32), 0, 1)
grid_mask = np.where(grid_mask == 0, 0, 1)
Copy link

Choose a reason for hiding this comment

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

Is this to support unmasked values other than 1 ? If so, maybe worth mentioning in the docstring.

Choose a reason for hiding this comment

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

Maybe a conversion to boolean then to integer.

@@ -77,17 +83,21 @@ def __init__(self, ds_in, ds_out, method, periodic=False,
ds_in, ds_out : xarray DataSet, or dictionary
Contain input and output grid coordinates. Look for variables
``lon``, ``lat``, and optionally ``lon_b``, ``lat_b`` for
conservative method.
add method.
Copy link

Choose a reason for hiding this comment

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

?

@@ -175,6 +193,7 @@ def esmf_regrid_build(sourcegrid, destgrid, method,

- 'bilinear'
- 'conservative', **need grid corner information**
- 'conservative_normed', **need grid corner information**
Copy link

Choose a reason for hiding this comment

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

I'm wondering if mapping ESMFpy method options to new xESMF methods will scale well.

ESMFPy has these clear and well documented IntEnum classes in api.constants for options. To make it more pythonic, maybe xESMF could convert those to string options, e.g.

norm_type : {'dstarea', 'fracarea'}
  The type of normalization used when producing the weights.

and then

ESMF.Regrid(..., 
norm_type=getattr(NormType, norm_type.upper()),
...)

@@ -54,8 +54,14 @@ def ds_to_ESMFgrid(ds, need_bounds=False, periodic=None, append=None):
lat = np.asarray(ds['lat'])
lon, lat = as_2d_mesh(lon, lat)

if 'mask' in ds:
mask = np.asarray(ds['mask'])
print(mask.shape)
Copy link

Choose a reason for hiding this comment

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

Remove print statement.

# tranpose the arrays so they become Fortran-ordered
grid = esmf_grid(lon.T, lat.T, periodic=periodic)
grid = esmf_grid(lon.T, lat.T, periodic=periodic, mask=mask)
Copy link

Choose a reason for hiding this comment

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

Would it be clearer to have mask.T as well insted of transposing it inside esgf_grid ?

dims=('y', 'x'))
print(ds_in)
# 'patch' is too slow to test
for method in ['bilinear', 'conservative', 'nearest_s2d', 'nearest_d2s']:
Copy link

Choose a reason for hiding this comment

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

Shouldn't conservative_normed be exercised ?

Choose a reason for hiding this comment

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

Yep I agree. In fact, all conservative methods must be tested numerically, especially with masking.

@raphaeldussin
Copy link
Contributor

I am able to get the results I want with the masking, however a big issue is that the mask is restricted to 2d.
I think we should be able to handle at least 3d, maybe even 4d for wetting/drying

@rabernat
Copy link

When we last talked it seems like @raphaeldussin was willing to take this over.

Raf, starting from this branch, cloud you make a new PR to https://github.com/pangeo-data/xESMF?

@raphaeldussin
Copy link
Contributor

@rabernat sure I'll take care of this one. It probably just needs a quick fix for conflicts since I was able to get the computation done.

@yuvipanda
Copy link

I think pangeo-data/xESMF#1 overrides this, right?

@huard
Copy link

huard commented Nov 10, 2020

Yes, development for xESMF now happens in pangeo-data/xESMF

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants