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

Remove monotonicity coordinate check for unstructured grids #965

Merged
merged 6 commits into from
Feb 1, 2021

Conversation

jvegreg
Copy link
Contributor

@jvegreg jvegreg commented Jan 27, 2021

Description

We currently can not work with unstructured grids because they fail the monotonicity check. This PR fixes this by skipping the check when we detect an unstructured grid, as we do for the irregular ones when ndim of lat and lon is greather than one.

I opted for a fairly easy way to detect those grids: look for ndim == 1 and latand lon sharing the same dimension


Before you get started

Checklist

If you make backwards incompatible changes to the recipe format:

  • Update ESMValTool and link the pull request(s) in the description

To help with the number pull requests:

@jvegreg jvegreg added this to the v2.2.0 milestone Jan 27, 2021
@jvegreg jvegreg added the cmor Related to the CMOR standard label Jan 27, 2021
@jvegreg jvegreg changed the title Add support for unstructured grids Remove monotonicity coordinate check for unstructured grids Jan 27, 2021
Copy link
Contributor

@koldunovn koldunovn left a comment

Choose a reason for hiding this comment

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

Thanks a lot for doing this, @jvegasbsc ! Looks good to me. The assumption of the same sized 1d coordinate is not necessary mean unstructured mesh, one can have a setup on regular grid with same number of lons and lats (some crazy regional model), but better to be practical :)

@jvegreg
Copy link
Contributor Author

jvegreg commented Jan 28, 2021

Thanks a lot for doing this, @jvegasbsc ! Looks good to me. The assumption of the same sized 1d coordinate is not necessary mean unstructured mesh, one can have a setup on regular grid with same number of lons and lats (some crazy regional model), but better to be practical :)

Is not checking is the same size, it is checking is the same dimension in the cube (see https://scitools-iris.readthedocs.io/en/latest/generated/api/iris/cube.html?highlight=coord_dims#iris.cube.Cube.coord_dims) so it should be safer

@bouweandela bouweandela requested review from Peter9192 and removed request for bouweandela January 29, 2021 10:30
Copy link
Contributor

@Peter9192 Peter9192 left a comment

Choose a reason for hiding this comment

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

Checking in to offload Bouwe. Code make sense to me. Just one suggestion, a question, and a typo.
If you can point me to a nice example cube with unstructured data I could do a more thorough check, but maybe that's not necessary if one of the other reviewers did it.
Cheers! @jvegasbsc

esmvalcore/cmor/check.py Outdated Show resolved Hide resolved
esmvalcore/cmor/check.py Outdated Show resolved Hide resolved
esmvalcore/cmor/check.py Outdated Show resolved Hide resolved
esmvalcore/cmor/check.py Outdated Show resolved Hide resolved
Javier Vegas-Regidor and others added 2 commits January 29, 2021 16:20
Co-authored-by: Peter Kalverla <peter.kalverla@gmx.com>
@bouweandela
Copy link
Member

If you can point me to a nice example cube with unstructured data I could do a more thorough check

You can find the name of the model and variable in the linked issue, it would be great if you could try running a recipe to make sure it really works.

@jvegreg
Copy link
Contributor Author

jvegreg commented Jan 29, 2021

This set of HighRESMIP experiments, for example. Be aware that you may need some extra tags, as I run this in our local copies of the data

     - {dataset: CESM1-CAM5-SE-HR, project: CMIP6, exp: hist-1950, start_year: 1960, end_year: 2014, ensemble: r1i1p1f1, grid: gn, mip: Amon}
     - {dataset: CESM1-CAM5-SE-LR, project: CMIP6, exp: hist-1950, start_year: 1960, end_year: 2014, ensemble: r1i1p1f1, grid: gn, mip: Amon}
     - {dataset: CESM1-CAM5-SE-HR, project: CMIP6, exp: highres-future, start_year: 2015, end_year: 2050, ensemble: r1i1p1f1, grid: gn, mip: Amon}
     - {dataset: CESM1-CAM5-SE-LR, project: CMIP6, exp: highres-future, start_year: 2015, end_year: 2050, ensemble: r1i1p1f1, grid: gn, mip: Amon }

@Peter9192
Copy link
Contributor

This set of HighRESMIP experiments, for example.

Thanks, that seems to work like a charm πŸ‘

Now we just need to make sure that the checks pass. The docs build is fixed here: #964.

@jvegreg
Copy link
Contributor Author

jvegreg commented Feb 1, 2021

Checks work, codacy down.

@nielsdrost
Copy link
Member

Seems like Codacy is spitting out another warning. Can you see if this is fixable @jvegasbsc ?

@jvegreg jvegreg dismissed Peter9192’s stale review February 1, 2021 20:36

Thanks, that seems to work like a charm πŸ‘ Now we just need to make sure that the checks pass.

@jvegreg
Copy link
Contributor Author

jvegreg commented Feb 1, 2021

Sorry for dismissing your review @Peter9192 , but the check pass and the codacy issue is solved now, and we want this in 2.2

@jvegreg jvegreg merged commit 61ce6ba into master Feb 1, 2021
@jvegreg jvegreg deleted the support_unestructured branch February 1, 2021 20:37
@Peter9192
Copy link
Contributor

Sorry for dismissing your review @Peter9192 , but the check pass and the codacy issue is solved now, and we want this in 2.2

No problem! Sorry I didn't get back to it soon enough.

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

Successfully merging this pull request may close these issues.

Working with unstructured ocean AWI-CM data
5 participants