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

Update WOA cmorizer for WOA18 and WOA13v2 #1812

Merged
merged 44 commits into from
Oct 11, 2021
Merged

Update WOA cmorizer for WOA18 and WOA13v2 #1812

merged 44 commits into from
Oct 11, 2021

Conversation

LisaBock
Copy link
Contributor

@LisaBock LisaBock commented Sep 11, 2020

Tasks

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • Give this pull request a descriptive title that can be used as a one line summary in a changelog
  • Make sure your code is composed of functions of no more than 50 lines and uses meaningful names for variables
  • Circle/CI tests pass. Status can be seen below your pull request. If the tests are failing, click the link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. Status can be seen below your pull request. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.

Modified data reformatting script

  • Test the CMORized data using recipes/example/recipe_check_obs.yml, to make sure the CMOR checks pass without errors
  • Tag @remi-kazeroni in this pull request, so that the updated dataset can be added to the OBS data pool at DKRZ and synchronized with CEDA-Jasmin

Closes #1811, #1959

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 one @LisaBock - a couple minor style issues, I am CC-ing @malininae since she is also interested in the topic. Also, would it make sense to add a few lines about WOA13 in the description in the cmorizer (I noticed you left the source for WOA13 so that's nice, but maybe a couple words in human-readable format what's the difference between the two versions or something like that

esmvaltool/cmorizers/obs/cmorize_obs_woa.py Outdated Show resolved Hide resolved
esmvaltool/recipes/examples/recipe_check_obs.yml Outdated Show resolved Hide resolved
@LisaBock
Copy link
Contributor Author

@valeriupredoi The test fails. So do I have to upload the rawobs dataset somewhere?

@valeriupredoi
Copy link
Contributor

@valeriupredoi The test fails. So do I have to upload the rawobs dataset somewhere?

no no worries, I fixed the test, it needed the new names to build dummy files. So just to be clear: only 4 variables (so, sos, tos and thetao) are produced now - what about the o2 from file: woa13_all_o and all the other ones commented out - what's the deal with those?

@LisaBock
Copy link
Contributor Author

@valeriupredoi The test fails. So do I have to upload the rawobs dataset somewhere?

no no worries, I fixed the test, it needed the new names to build dummy files. So just to be clear: only 4 variables (so, sos, tos and thetao) are produced now - what about the o2 from file: woa13_all_o and all the other ones commented out - what's the deal with those?

Thanks!
I am not sure. I did not change anything for the o2 or other variables... just (so, sos, tos and thetao)

@valeriupredoi
Copy link
Contributor

OK so now the code is fixed and formatted and tests (will) pass, so I am ready to greenlight this from a technical point. I am, however, a bit puzzled about the variables we removed and that were present for woa13 (like o2, po, si etc) - are those variables not produced anymore by woa18 and they should be used from woa13 or they are not produced anymore by woa18 and the ones from woa13 are obsolete? @remi-kazeroni could you give this a scientific review please, mate, and maybe link with @LisaBock and @malininae see about those variables and availability of data? Cheers guys 🍺

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks again for all your work on this PR @tomaslovato! I have just tested everything again and all looks good to me now. Both versions of the data are well downloaded and cmorized. The recipe_ocean_bgc.yml runs fine as well. I left minor comments related to the source of both dataset versions and the recipe_checkobs.yml. Could you please have a look at those? Could you also merge the main branch into this one? That may fix the currently failing test. After that, this is good to be merged imho.

@remi-kazeroni remi-kazeroni changed the title Update WOA cmorizer for WOA18 Update WOA cmorizer and recipe_ocean_bgc for WOA18 Sep 30, 2021
@tomaslovato
Copy link
Contributor

@remi-kazeroni @valeriupredoi CircleCI is producing an error while testing recipe_ocean_bgc.yml related to volcello definition in OBS6 ... do you have any hint about?

Copy link
Contributor

@remi-kazeroni remi-kazeroni left a comment

Choose a reason for hiding this comment

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

Thanks for the latest changes @tomaslovato! Everything looks good to me and this can be merged as soon as the failing test is fixed. @valeriupredoi any idea what went wrong with the test?

@valeriupredoi
Copy link
Contributor

valeriupredoi commented Oct 6, 2021

ouch! it looks like the first failed test is easily fixable (why are you expecting 18 files and not 8?), the second one is a bit more hairy, I need to have a closer look see what's happening - we're not producing and WOA fx variables no?

@remi-kazeroni
Copy link
Contributor

ouch! it looks like the first failed test is easily fixable (why are you expecting 18 files and not 8?)

My bad, it did not scroll to the end of the log file and missed that failed test... Sure this is no longer up to date after the latest changes to the cmorizer. Could you please check that @tomaslovato?

@valeriupredoi
Copy link
Contributor

the problem is with esmvaltool/recipes/recipe_ocean_bgc.yml - the additional dataset is WOA as OBS6 following CMIP6 cmor standards, but everything in it apart from that is CMIP5 - the loader fails when finding a correct mip for volcello needed in volume stats (well, there is only one model needed, and that's CMIP5) - that recipe needs a hefty clean up - I don't know what's the best way to set it up but surely asking for a CMIP6-style dataset with CMIP5 data specs ain't gonna work - @ledm could you please have a look?

@tomaslovato
Copy link
Contributor

ouch! it looks like the first failed test is easily fixable (why are you expecting 18 files and not 8?)

My bad, it did not scroll to the end of the log file and missed that failed test... Sure this is no longer up to date after the latest changes to the cmorizer. Could you please check that @tomaslovato?

I missed it as well! I changed the number of expected files in test_cmorize_obs.py ... thanks V!

@remi-kazeroni
Copy link
Contributor

the problem is with esmvaltool/recipes/recipe_ocean_bgc.yml - the additional dataset is WOA as OBS6 following CMIP6 cmor standards, but everything in it apart from that is CMIP5 - the loader fails when finding a correct mip for volcello needed in volume stats (well, there is only one model needed, and that's CMIP5) - that recipe needs a hefty clean up - I don't know what's the best way to set it up but surely asking for a CMIP6-style dataset with CMIP5 data specs ain't gonna work - @ledm could you please have a look?

Thanks @valeriupredoi! I'm just wondering whether this should be done here or go to a separate PR. Now sure what is the best to get at least the updated cmorizer into the upcoming release. Since we support the cmorization of both versions of WOA, updating the esmvaltool/recipes/recipe_ocean_bgc.yml with WOA18 and CMIP6 data could be done afterwards.

@tomaslovato
Copy link
Contributor

in the meanwhile I updated recipe_ocean_bgc.yml to use the new write options from #2005, as it was not working with the latest version of ESMValCore.
@remi-kazeroni actually updating the script to use CMIP6 data won't be that long, since it only needs to move the reference dataset from HadGEM2-ES to UKESM1-0-LL.

@valeriupredoi this issue with the loader would appear also when a single recipe is using both CMIP5 and CMIP6 models?

@tomaslovato tomaslovato changed the title Update WOA cmorizer and recipe_ocean_bgc for WOA18 Update WOA cmorizer for WOA18 and WOA13v2 Oct 8, 2021
@tomaslovato
Copy link
Contributor

@remi-kazeroni I think this should be good to go now ...

Copy link
Contributor

@remi-kazeroni remi-kazeroni 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 your efforts @tomaslovato! And sorry for the complications related to the recipe_ocean_bgc.yml... This PR is a nice example of how several versions of an obs dataset can be supported in the ESMValTool without the need to have one cmorizer (and config file) per dataset version.

I have added 2 vars to the test for the sake of completness and retrieved the deleted datasets which were commented out in the recipe. @tomaslovato, please let me know if that is fine for you and I will merge the PR.

@tomaslovato
Copy link
Contributor

@remi-kazeroni thanks for your revision! for me it is ok!

@remi-kazeroni remi-kazeroni merged commit f3962eb into main Oct 11, 2021
@remi-kazeroni remi-kazeroni deleted the woa18_cmorizer branch October 11, 2021 12:20
@remi-kazeroni remi-kazeroni mentioned this pull request Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend WOA cmorizer for WOA18 data
6 participants