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

pin xgcm < 0.7 #243

Closed
Mikejmnez opened this issue Jul 22, 2022 · 13 comments · Fixed by #305
Closed

pin xgcm < 0.7 #243

Mikejmnez opened this issue Jul 22, 2022 · 13 comments · Fixed by #305
Assignees
Labels
dependencies Pull requests that update a dependency file

Comments

@Mikejmnez
Copy link
Collaborator

Description

There have been major updates to xgcm , regarding generalized ufuncs, resulting in some changes to the way grid interpolates and calculates derivatives. These shouldn't affect oceanspy at the fundamental level, but have been causing some trouble.

For a better description on the changes to xgcm see:
xgcm/xgcm#344

These new changes involve refactoring, and are somewhat related to #224.

Changes to xgcm have been implemented in versions >= 0.7. You can see the description of new features here: https://xgcm.readthedocs.io/en/latest/whats-new.html

As a result of the changes, the following code does not work after performing the cutout (the cutout works well, as you can make plots which trigger the calculation):

grid.interp(cut_od._ds['Depth'], axis='X', boundary='extrapolate')

which yields the error

NotImplementedError: Cannot chunk along a core dimension for a grid ufunc which has a signature which includes one of the axis positions ['inner', 'outer']

Also, beginning with version 0.7 the grid.interp argument

boundary='extrapolate'

no longer appears as an option on the description/documentation and there was some discussion to remove extrapolate as an option completely, although for now (as of version 0.8), it is still possible to continue using it.

Quick fix

Pin xgcm to version 0.6 while I work on figuring out the best fix to this. There is strong possibility that the error only affects the grid post-cutout, and so the fix will be local to the code where I update the grid object with the new (subsampled) dataset. I just need to some to figure it out.

@Mikejmnez Mikejmnez self-assigned this Jul 22, 2022
@Mikejmnez Mikejmnez changed the title pin gxcm = 0.6 pin xgcm = 0.6 Jul 25, 2022
@Mikejmnez Mikejmnez changed the title pin xgcm = 0.6 pin xgcm < 0.7 Jul 25, 2022
@Mikejmnez
Copy link
Collaborator Author

Will keep this open, as pining xgcm is only a quick fix.

@Mikejmnez Mikejmnez changed the title pin xgcm < 0.7 pin xgcm < 0.7 and xarray < 2022.6 Jul 25, 2022
@Mikejmnez
Copy link
Collaborator Author

The new version of xarray also contains some internal refactoring changes that changes some aspects of indexing. This new version of xarray was released last week, and so it didn't cause any issue on the test image that me and Tom ran in Sciserver.

More on xarray changes released last week are here

https://docs.xarray.dev/en/stable/whats-new.html#v2022-06-0-july-21-2022

Using the new release of xarray (version 2022.6) did cause an error in Sciserver in

dmaskT = maskT.where(maskT, drop=True)

before the cutout. I need some time to fully understand and fix these incompatibility issues (should be small, but subtle). This all is related to Ryan's refactoring proposals in #224. Thus, a fix to this current issue should also fix #224.

@ThomasHaine
Copy link
Collaborator

ThomasHaine commented Oct 18, 2022

Explore this issue when #224 is addressed....

@ThomasHaine ThomasHaine added the dependencies Pull requests that update a dependency file label Oct 18, 2022
@MaceKuailv MaceKuailv self-assigned this Oct 20, 2022
@MaceKuailv
Copy link
Collaborator

The survey station is also broken by changes in xgcm, with a similar message.

@Mikejmnez
Copy link
Collaborator Author

Makes sense, as there is some interpolation happening there.

@MaceKuailv
Copy link
Collaborator

I don't think we should change anything here, although it apparently breaks a lot of things. Oceanspy is only trying to do the most basic things xgcm promises, interpolate velocity to the center point. If the velocity dataset is chunked, then you would have this issue.

This is a known bug they have mentioned many many times.

xgcm/xgcm#522
xgcm/xgcm#533
xgcm/xgcm#518

I have adapted their tutorial just a little bit to reproduce this bug.

from xgcm import Grid
ds = xr.Dataset(
        coords={
            "x_c": (
                ["x_c"],
                np.arange(1, 10),
            ),
            "x_g": (
                ["x_g"],
                np.arange(1.5, 9),
            ),
        }
    )
grid = Grid(ds, coords={"X": {"center": "x_c", "inner": "x_g"}})
da = np.sin(ds.x_c * 2 * np.pi / 9)
da = da.chunk((9)) # if it is not chunked, then no error

da_interp = grid.diff(da, axis="X") # you get the same error here

@Mikejmnez
Copy link
Collaborator Author

Thanks for looking into this. It is re assuring that we don't need to make changes regarding xgcm. It wasn't clear to me whether that was true or not back in July when the release happened just before updating the image.

So it seems that keep pinning xgcm is the right way to do this, and wait until it gets fixed on their end. Have you had time to check on the issue regarding xarray? An error during a cutout cause some trouble, after the following line:

dmaskT = maskT.where(maskT, drop=True)

I don't remember the actual example where this happened, but I am guessing in one of the example notebooks (not LLC specific).

@MaceKuailv
Copy link
Collaborator

I am having a bit of a hard time finding this issue. I am using newer version of xarray to cutout LLC datasets now. It seems fine to me

@MaceKuailv
Copy link
Collaborator

I tried a similar thing here:
image
It seems fine to me...

@ThomasHaine
Copy link
Collaborator

So, can we unpin xarray? Or @Mikejmnez can you reproduce the error?

@Mikejmnez
Copy link
Collaborator Author

Mikejmnez commented Oct 22, 2022 via email

@Mikejmnez Mikejmnez changed the title pin xgcm < 0.7 and xarray < 2022.6 pin xgcm < 0.7 ~~and xarray < 2022.6~~ Oct 25, 2022
@Mikejmnez Mikejmnez changed the title pin xgcm < 0.7 ~~and xarray < 2022.6~~ pin xgcm < 0.7 <s>and xarray < 2022.6<s> Oct 25, 2022
@Mikejmnez Mikejmnez changed the title pin xgcm < 0.7 <s>and xarray < 2022.6<s> pin xgcm < 0.7 <s>and xarray < 2022.6</s> Oct 25, 2022
@Mikejmnez Mikejmnez changed the title pin xgcm < 0.7 <s>and xarray < 2022.6</s> pin xgcm < 0.7 Oct 25, 2022
@Mikejmnez
Copy link
Collaborator Author

PR #270 unpins xarray and all tests pass

@Mikejmnez
Copy link
Collaborator Author

Mikejmnez commented Dec 30, 2022

It looks like they have mostly solve this issue in the latest xgcm version. I ran the example above by @MaceKuailv

from xgcm import Grid
ds = xr.Dataset(
        coords={
            "x_c": (
                ["x_c"],
                np.arange(1, 10),
            ),
            "x_g": (
                ["x_g"],
                np.arange(1.5, 9),
            ),
        }
    )
grid = Grid(ds, coords={"X": {"center": "x_c", "inner": "x_g"}})
da = np.sin(ds.x_c * 2 * np.pi / 9)
da = da.chunk((9)) # if it is not chunked, then no error

da_interp = grid.diff(da, axis="X") 

and runs fine. This issue xgcm/xgcm#522 explains their approach, which is basically that single chunking along a core dimension works (it had stopped working). Multiple or arbitrary chunks along a core dimension yields the same error. They have updated the error to suggest rechunking to a single chunk along the dimension if possible.

I will again update the binder shortly.

@Mikejmnez Mikejmnez mentioned this issue Dec 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants