xgcm icon indicating copy to clipboard operation
xgcm copied to clipboard

[BUG] Diff in non periodic axis seems to be periodic

Open robin-cls opened this issue 3 years ago • 4 comments
trafficstars

What happened:

When applying a diff operation on a simple example (modified from the doc https://xgcm.readthedocs.io/en/latest/grids.html), it seems that the X axis is considered as periodic even if stated otherwise.

What you expected to happen:

In our minimal example, we apply the diff operation on a small array [2, 4, 5, 8, 10, 12, 14, 16, 18]. We expected to have [np.nan, 2, 2, 2...] or an array with one less element, but we got [-16, 2, 2, 2...] instead. It seems that the last element is taken to extend the boundary at the left, whereas it has no relation with the first element whatsoever.

Minimal Complete Verifiable Example:

import xarray as xr
import xgcm
import numpy as np

ds = xr.Dataset(
	coords={
		"x_c": (
			["x_c"],
			np.arange(1, 10),
		),
		"x_g": (
			["x_g"],
			np.arange(0.5, 9),
		),
	}
)

da = ds.x_c*2
grid = xgcm.Grid(ds, coords={"X": {"center": "x_c", "left": "x_g"}}, periodic=False)

grid
#>> <xgcm.Grid>
#>> X Axis (not periodic, boundary=None):
#>>  * center   x_c --> left
#>>  * left     x_g --> center

grid.diff(da, "X")
#>> <xarray.DataArray 'x_c' (x_g: 9)>
#>> array([-16,   2,   2,   2,   2,   2,   2,   2,   2])
#>> Coordinates:
#>>   * x_g      (x_g) float64 0.5 1.5 2.5 3.5 4.5 5.5 6.5 7.5 8.5

xgcm version and environment:

xgcm 0.7.0

robin-cls avatar Jun 10 '22 14:06 robin-cls

Thanks for raising this issue @robin-cls. This is clearly a bug! In fact this

(not periodic, boundary=None)

does not really make sense. I believe this might be a bug caused by our plan to fully deprecate the periodic keyword argument. I think a quick solution to your problem here is to specify a value for boundary (if you wanted to have a periodic boundary you could do boundary='periodic'.

Ill try look more into this next week. Let me know if the fix above works for you

jbusecke avatar Jun 10 '22 15:06 jbusecke

Thanks @jbusecke , using boundary works properly :

image

nans are propagated on the inverse operation cumsum, so I guess fill_value=np.nan is not a very good idea for handling the boundary.

robin-cls avatar Jun 10 '22 15:06 robin-cls

Just commenting that I also ran into this same issue today when I upgraded from 0.6.0 to 0.7.0.

jkrasting avatar Jun 15 '22 17:06 jkrasting

Thanks for reporting @jkrasting. Ill try to work on a fix next week!

jbusecke avatar Jun 16 '22 20:06 jbusecke