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

new vertical coord decode and reordering #315

Merged
merged 22 commits into from
Mar 29, 2022

Conversation

kthyng
Copy link
Contributor

@kthyng kthyng commented Mar 25, 2022

The vertical decoding has been added for ocean_sigma_coordinate and the free surface variable has been expanded dimensionally for all the calculations so that the results preserve the standard ordering of time, vertical, lat/Y, lon/X. Three of the tests do not work anymore, though. Expanding dimensionally apparently bumps the attributes off of s_rho, for one. For another, the values are very similar but do not match anymore — not sure why. Interested to see what @dcherian thinks!

The vertical decoding has been added for `ocean_sigma_coordinate` and the free surface variable has been expanded dimensionally for all the calculations so that the results preserve the standard ordering of time, vertical, lat/Y, lon/X. Three of the tests do not work anymore, though. Expanding dimensionally apparently bumps the attributes off of s_rho, for one. For another, the values are very similar but do not match anymore — not sure why. Interested to see what @dcherian thinks!
@kthyng kthyng requested a review from dcherian March 25, 2022 14:12
@kthyng
Copy link
Contributor Author

kthyng commented Mar 25, 2022

Ah I see an immediate problem with my zname_in selection. It works for POM where there is one vertical coordinate but for FVCOM which also uses ocean_sigma_coordinate, there are two, and neither has _ in them. So, zname_in should be a list I guess? or just a prefix on the full s coordinate name?

@dcherian
Copy link
Contributor

dcherian commented Mar 26, 2022

What are the variable names for FVCOM? Can you also add an FVCOM test dataset please?

I think the clearest solution might be a new outnames kwarg or maybe decode_to

ds.cf.decode_vertical_coords(outnames={"s_rho": "z_rho", "s_w": "z_w"})

which means that we should deprecate prefix by first changing the default to None, then

if prefix is not None:
    warnings.warn(..., DeprecationWarning)
    # set outnames here

cf_xarray/accessor.py Outdated Show resolved Hide resolved
doc/whats-new.rst Outdated Show resolved Hide resolved
cf_xarray/accessor.py Outdated Show resolved Hide resolved
@dcherian dcherian closed this Mar 26, 2022
@dcherian dcherian reopened this Mar 26, 2022
pre-commit-ci bot and others added 2 commits March 26, 2022 03:41
Co-authored-by: Deepak Cherian <dcherian@users.noreply.github.com>
@kthyng
Copy link
Contributor Author

kthyng commented Mar 27, 2022

What are the variable names for FVCOM? Can you also add an FVCOM test dataset please?

FVCOM won't work currently with xarray because the vertical coordinates have the same names as the dimensions and can't be read in without dropping the coordinate information, which is what has the vertical coordinate info. But I am trying to set up to be ready to do it when it works!

I think the clearest solution might be a new outnames kwarg or maybe decode_to

ds.cf.decode_vertical_coords(outnames={"s_rho": "z_rho", "s_w": "z_w"})

which means that we should deprecate prefix by first changing the default to None, then

if prefix is not None:
    warnings.warn(..., DeprecationWarning)
    # set outnames here

I like this approach.

cf_xarray/accessor.py Outdated Show resolved Hide resolved
cf_xarray/accessor.py Outdated Show resolved Hide resolved
@kthyng
Copy link
Contributor Author

kthyng commented Mar 28, 2022

@dcherian Sorry we overlapped a bit just now. Could you take a look now? I think I have it correct but not positive.

cf_xarray/accessor.py Outdated Show resolved Hide resolved
cf_xarray/accessor.py Show resolved Hide resolved
cf_xarray/accessor.py Outdated Show resolved Hide resolved
cf_xarray/datasets.py Outdated Show resolved Hide resolved
cf_xarray/accessor.py Show resolved Hide resolved
Copy link
Contributor

@dcherian dcherian left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Just 2 v. minor comments. Thanks!

The test failures are unrelated.

cf_xarray/accessor.py Outdated Show resolved Hide resolved
cf_xarray/tests/test_accessor.py Show resolved Hide resolved
cf_xarray/accessor.py Outdated Show resolved Hide resolved
@kthyng
Copy link
Contributor Author

kthyng commented Mar 29, 2022

@dcherian Anything else I should do here?

@dcherian
Copy link
Contributor

There's another test that needs to be fixed by providing outnames

@kthyng
Copy link
Contributor Author

kthyng commented Mar 29, 2022

@dcherian Ah ok how about now?

@dcherian dcherian merged commit 228ee4a into xarray-contrib:main Mar 29, 2022
@dcherian
Copy link
Contributor

Thanks!

@kthyng
Copy link
Contributor Author

kthyng commented Mar 29, 2022

Wahoo!!! Thanks for all your help!

@kthyng kthyng deleted the add_ocean_sigma_coordinate branch March 30, 2022 14:13
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.

2 participants