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

Automatically fix small deviations in vertical levels #1177

Merged
merged 7 commits into from
Oct 5, 2021

Conversation

bouweandela
Copy link
Member

@bouweandela bouweandela commented Jun 14, 2021

Description

This pull request aims to make working with vertical levels that have tiny errors in the coordinate values easier. It does this by:

  1. trying to automatically fix the problem
  2. setting the levels equal to those requested in extract_levels when cube slicing is used instead of interpolation

This will make it easier to compute multi-model statististics, which now need exactly matching coordinates.

See #956 (comment)


Before you get started

Checklist

It is the responsibility of the author to make sure the pull request is ready to review. The icons indicate whether the item will be subject to the 🛠 Technical or 🧪 Scientific review.


To help with the number pull requests:

@bouweandela bouweandela requested a review from jvegreg June 14, 2021 09:23
@bouweandela bouweandela added the cmor Related to the CMOR standard label Jun 14, 2021
@valeriupredoi
Copy link
Contributor

this is cool! maybe we should make the tolerances a user tunable parameter? I can see uses for in a configuration file

@bouweandela
Copy link
Member Author

I'm not sure. Do you think the values that I've chosen now make sense?

@zklaus zklaus added this to the v2.4.0 milestone Jun 15, 2021
@zklaus
Copy link

zklaus commented Jun 15, 2021

I think in this case it's better if we control the tolerances. Note also that it might be tempting to do a similar thing for horizontal coordinates, where e.g. in CORDEX very small deviations can occur. The difference is that there is no "right" value in CORDEX, this is decided by the model, whereas here we have an authoritative value from the CMIP tables. This is also what allows us to decide on tolerances, imho.

@bouweandela
Copy link
Member Author

Note also that it might be tempting to do a similar thing for horizontal coordinates, where e.g. in CORDEX very small deviations can occur.

Indeed, this was implemented in #507 with the default tolerances. The default relative tolerance might be a bit lax (rtol 1e-5, atol 1e-8).

@bouweandela
Copy link
Member Author

I think in this case it's better if we control the tolerances.

I could add that for extract_levels. What would you propose as the default? The current values?

@zklaus
Copy link

zklaus commented Jun 23, 2021

I think the current values look fine to me, but let's have another look for the next release.

@codecov
Copy link

codecov bot commented Oct 1, 2021

Codecov Report

Merging #1177 (076243a) into main (43e6b16) will increase coverage by 0.27%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1177      +/-   ##
==========================================
+ Coverage   87.47%   87.74%   +0.27%     
==========================================
  Files         193      193              
  Lines        9649     9683      +34     
==========================================
+ Hits         8440     8496      +56     
+ Misses       1209     1187      -22     
Impacted Files Coverage Δ
esmvalcore/_recipe.py 93.19% <100.00%> (+0.97%) ⬆️
esmvalcore/cmor/check.py 97.71% <100.00%> (+<0.01%) ⬆️
esmvalcore/experimental/recipe_info.py 79.59% <100.00%> (+14.37%) ⬆️
esmvalcore/experimental/recipe_output.py 77.51% <100.00%> (+3.36%) ⬆️
esmvalcore/preprocessor/_regrid.py 96.34% <100.00%> (+0.88%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c77474d...076243a. Read the comment docs.

@bouweandela
Copy link
Member Author

The remaining Codacy issue is about too many arguments for the extract_levels preprocessor function, so unfortunately it cannot be solved.

@bouweandela
Copy link
Member Author

@zklaus I added the option to tweak the parameters as requested. Could you have another look, please?

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

nice add-on @bouweandela - do you want to add a couple words about it in the documentation (other than the API documentation)?

@bouweandela
Copy link
Member Author

Thank you for reviewing @valeriupredoi! @zklaus Do you have any further comments on this?

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.

3 participants