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

Add CMIP6 amoc derivation case and add a test #1577

Merged
merged 21 commits into from
May 20, 2022
Merged

Conversation

valeriupredoi
Copy link
Contributor

@valeriupredoi valeriupredoi commented May 11, 2022

Description

Improving a bit the Amoc derivation script amoc.py:

  • add optionality for CMIP6 variable that changed name
  • add some Value- and iris.Constraint- Errors
  • add a test to it although I am not 100% happy with the test since it doesn't actually check the computation
    but for that I need to assemble a fairly complex cube and know what that Atlantic constraint is - no time now - @ledm can do that
    scratch that it's a boss test for 100% coverage 😁

Closes ESMValGroup/ESMValTool#2645


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:

@valeriupredoi valeriupredoi added enhancement New feature or request preprocessor Related to the preprocessor testing labels May 11, 2022
@codecov
Copy link

codecov bot commented May 11, 2022

Codecov Report

Merging #1577 (6ee5e69) into main (b902e58) will increase coverage by 0.15%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1577      +/-   ##
==========================================
+ Coverage   90.83%   90.98%   +0.15%     
==========================================
  Files         200      200              
  Lines       10648    10662      +14     
==========================================
+ Hits         9672     9701      +29     
+ Misses        976      961      -15     
Impacted Files Coverage Ξ”
esmvalcore/preprocessor/_derive/amoc.py 100.00% <100.00%> (+65.21%) ⬆️

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 b902e58...6ee5e69. Read the comment docs.

@dhohn
Copy link
Contributor

dhohn commented May 11, 2022

Thanks a lot @valeriupredoi .
You're just missing the msftmz also for CMIP6 where one of the two msftyz/msftmz may be present. Theres also different latitude coords to deal with. See my adaptation here: https://github.com/dhohn/ESMValCore/blob/amoc_cmip6/esmvalcore/preprocessor/_derive/amoc.py
Very much inspired by the olf https://github.com/ESMValGroup/ESMValCore/tree/development_amoc_cmip6 branch I was pointed to in Tool#2645.

@valeriupredoi
Copy link
Contributor Author

@dhohn cheers! Ah that's why an engineer should not code up scientific stuff 😁 That's good to know, I didn't know about the new CMIP6-induced wiggles, the only problem is that we don't change the CMOR tables, any way you can go around the issue without changing the CMOR table?

@dhohn
Copy link
Contributor

dhohn commented May 12, 2022

Indeed the change to the cmor table was me being lazy (all the models I happened to look at had the same naming...). We can do dataset fixes for all the models instead. But that can be separate from the actual amoc strength computation in this PR.

@valeriupredoi
Copy link
Contributor Author

cheers @dhohn - been busy with other stuffs today, will have a look tomorrow and integrate your implementation for CMIP6 Amoc, and see what I can do about that var name change - probably open another PR πŸ‘

@schlunma
Copy link
Contributor

Let me know if/when you need a review from me on this, sounds like it does not make sense to do that right now! πŸ‘

@valeriupredoi valeriupredoi marked this pull request as draft May 13, 2022 11:19
@valeriupredoi
Copy link
Contributor Author

cheers, Manu! I forgot to convert it to a draft right after I got the (very useful) pointer from @dhohn that the CMIP6 amoc computation is not quite correct - did that now, and I'll try address that today, will ping you πŸ‘

@valeriupredoi
Copy link
Contributor Author

I have now fixed the amoc computation for CMIP6 but am very puzzled by the change for basin's out_name from basin to sector, as I pointed out here dhohn#1 (comment) - is that a genuine issue in the CMOR table (that we may need to raise with the PCMDI folk) or is it something else?

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented May 13, 2022

BTW @dhohn - many thanks for the input so far - most useful - would you like to be a collaborator to both ESMValCore and ESMValTool? I can add you to the repos, and to our collabs list, if that's OK with you - so that you can open PRs straight in w/o using forks 🍺

@valeriupredoi
Copy link
Contributor Author

@dhohn I have now finalized the implementation of the Amoc derivation - based on your PR, and am happy with the test as well, it's a proper one for 100% coverage, would you have time for a review please? Once you are happy, we'll ask @schlunma for a technical review too. Have a good weekend, guys! 🍺

@dhohn
Copy link
Contributor

dhohn commented May 16, 2022

Thanks a lot @valeriupredoi.
Looks great. Were just missing the optional varnames for cmip6:

 required = [ {'short_name': 'msftyz', 'optional': True, },
              {'short_name': 'msftmz', 'optional': True, } ]

The long name for msftmz is also 'ocean_meridional_overturning_mass_streamfunction' like for msftmyz in cmip5.

@dhohn
Copy link
Contributor

dhohn commented May 16, 2022

BTW @dhohn - many thanks for the input so far - most useful - would you like to be a collaborator to both ESMValCore and ESMValTool? I can add you to the repos, and to our collabs list, if that's OK with you - so that you can open PRs straight in w/o using forks 🍺

Sure. Id be very happy to :) What do you need from me for it?

@valeriupredoi
Copy link
Contributor Author

BTW @dhohn - many thanks for the input so far - most useful - would you like to be a collaborator to both ESMValCore and ESMValTool? I can add you to the repos, and to our collabs list, if that's OK with you - so that you can open PRs straight in w/o using forks beer

Sure. Id be very happy to :) What do you need from me for it?

awesome, cheers! I'll add you to the contributors list so if that's okay with you could I please have your full name, affiliation, orcid number (if any), and bank account and sort code - just kidding about the bank info 😁 Have a look at our Contributing Guidelines and pls let me know if that's all good with you 🍺 πŸ…

@valeriupredoi
Copy link
Contributor Author

valeriupredoi commented May 16, 2022

Thanks a lot @valeriupredoi. Looks great. Were just missing the optional varnames for cmip6:

 required = [ {'short_name': 'msftyz', 'optional': True, },
              {'short_name': 'msftmz', 'optional': True, } ]

The long name for msftmz is also 'ocean_meridional_overturning_mass_streamfunction' like for msftmyz in cmip5.

Is {'short_name': 'msftmz' } an alternative to msftyz? After I add you as contributor, would you mind adding that case in there please? So you are an author of the PR as you should be (I rushed things and went ahead with this PR, sorry); then I'll add a test case for that, and we're ready to launch this boat into the Atlantic β›΅

@dhohn
Copy link
Contributor

dhohn commented May 16, 2022

BTW @dhohn - many thanks for the input so far - most useful - would you like to be a collaborator to both ESMValCore and ESMValTool? I can add you to the repos, and to our collabs list, if that's OK with you - so that you can open PRs straight in w/o using forks beer

Sure. Id be very happy to :) What do you need from me for it?

awesome, cheers! I'll add you to the contributors list so if that's okay with you could I please have your full name, affiliation, orcid number (if any), and bank account and sort code - just kidding about the bank info 😁 Have a look at our Contributing Guidelines and pls let me know if that's all good with you 🍺 πŸ…

OK cool. So I add my info myself in my next PR for msftmz? Or you add it separately?

@valeriupredoi
Copy link
Contributor Author

BTW @dhohn - many thanks for the input so far - most useful - would you like to be a collaborator to both ESMValCore and ESMValTool? I can add you to the repos, and to our collabs list, if that's OK with you - so that you can open PRs straight in w/o using forks beer

Sure. Id be very happy to :) What do you need from me for it?

awesome, cheers! I'll add you to the contributors list so if that's okay with you could I please have your full name, affiliation, orcid number (if any), and bank account and sort code - just kidding about the bank info grin Have a look at our Contributing Guidelines and pls let me know if that's all good with you beer medal_sports

OK cool. So I add my info myself in my next PR for msftmz? Or you add it separately?

I'll add you via a direct PR (now) then you'd be ready to edit this PR and co-author it, how's that sound?

@dhohn
Copy link
Contributor

dhohn commented May 16, 2022

BTW @dhohn - many thanks for the input so far - most useful - would you like to be a collaborator to both ESMValCore and ESMValTool? I can add you to the repos, and to our collabs list, if that's OK with you - so that you can open PRs straight in w/o using forks beer

Sure. Id be very happy to :) What do you need from me for it?

awesome, cheers! I'll add you to the contributors list so if that's okay with you could I please have your full name, affiliation, orcid number (if any), and bank account and sort code - just kidding about the bank info grin Have a look at our Contributing Guidelines and pls let me know if that's all good with you beer medal_sports

OK cool. So I add my info myself in my next PR for msftmz? Or you add it separately?

I'll add you via a direct PR (now) then you'd be ready to edit this PR and co-author it, how's that sound?

perfect πŸ‘

@dhohn
Copy link
Contributor

dhohn commented May 18, 2022

@valeriupredoi Now that Im member and contributor I would add my suggested changes myself to this PR. So Im also on the record (and on the hook;) for the changes. Sound good?

@valeriupredoi
Copy link
Contributor Author

@dhohn absolutely, fire up the engines 🏍️

@dhohn dhohn marked this pull request as ready for review May 19, 2022 10:21
Copy link
Contributor Author

@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.

awesome, cheers muchly @dhohn 🍺 @schlunma a review would be much appreciated, my man 🍺

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Code looks good to me, just two minor comments.

I'm not qualified to review this PR from the scientific point of you, so it would be great if someone else with more expertise could have a look at that. Maybe @ledm? πŸ‘

esmvalcore/preprocessor/_derive/amoc.py Show resolved Hide resolved
tests/unit/preprocessor/_derive/test_amoc.py Outdated Show resolved Hide resolved
@valeriupredoi valeriupredoi changed the title Improve a bit amoc derivation and add a test Add CMIP6 amoc derivation case and add a test May 19, 2022
Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Code approved, but as already mentioned someone with the necessary scientific experience needs to have a look at this πŸ‘

@ledm
Copy link
Contributor

ledm commented May 20, 2022

The changes to the AMOC derivation code look good to me and are in-line with what I've done elsewhere for the AR6 CMIP6 AMOC calculation - looks good to me! I can't comment on whether the new test code is appropriate though as that's far my comfort zone.

@valeriupredoi
Copy link
Contributor Author

cheers @ledm - I think the sci approval between you and @dhohn together with the tech approval from @schlunma (and myself) makes this PR bullet-proof - an excellent dev and review process 😁 Cheers, guys! 🍺

@valeriupredoi valeriupredoi merged commit cac0ff5 into main May 20, 2022
@valeriupredoi valeriupredoi deleted the fix_amoc_derivation branch May 20, 2022 12:37
@sloosvel sloosvel added this to the v2.6.0 milestone Jun 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request preprocessor Related to the preprocessor testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

recipe_ocean_amoc.yml using CMIP6 data
5 participants