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 ESACCI-SST cmorizer to v3.0 #3697

Open
wants to merge 45 commits into
base: main
Choose a base branch
from
Open

Update ESACCI-SST cmorizer to v3.0 #3697

wants to merge 45 commits into from

Conversation

LisaBock
Copy link
Contributor

@LisaBock LisaBock commented Jun 28, 2024

Description

Update ESACCI-SST cmorizer to v3.0 (daily and monthly data).
I changed the ESACCI-SST data to the variable tos instead of ts because the observation field is only defined over ocean and not the whole globe like ts.

Registration at CEDA is needed for downloading the data.

Also a new custom variable tosStderr is needed (see #2470). It is already merge in the main branch of ESMValCore but not included in the last release.


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.

New or updated data reformatting script

@LisaBock LisaBock marked this pull request as ready for review July 1, 2024 14:23
@LisaBock LisaBock requested a review from a team as a code owner July 1, 2024 14:23
Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

This looks already very good. I addition to my comments on the code, I would recommend the following changes:

  • Units are reported as degC but should be in K (fields also look like data are in K)
  • The variable attributes host, reference, source, tier, user and version should rather be global attributes
  • Not sure about the variable attributes mip, modeling_realm and project_id. Other CMORrized datasets do not have these attributes. I would suggest to either change these attributes to global attributes or to remove them.
  • I would remove the variable attribute comment: Note that the variable tsStderr is an uncertainty not a standard error. from variable tos as this only applies to tosStderr, which is written to separate files
  • the attribute long_name of variable time should be simply time (instead of reference time of sst field)
  • similarly, the attribute long_name of variables lon and lat should be longitude and latitude instead of longitude coordinate and latitude coordinate

doc/sphinx/source/input.rst Outdated Show resolved Hide resolved
esmvaltool/cmorizers/data/cmor_config/ESACCI-SST.yml Outdated Show resolved Hide resolved
esmvaltool/cmorizers/data/cmor_config/ESACCI-SST.yml Outdated Show resolved Hide resolved
esmvaltool/recipes/examples/recipe_check_obs.yml Outdated Show resolved Hide resolved
@LisaBock
Copy link
Contributor Author

Thanks @axel-lauer for the review! The memory issue should be solved now and also the years could be also set now in the command line. Below you find my answers to your comments:

* Units are reported as `degC` but should be in `K` (fields also look like data are in `K`)

Cmor units are degC, the fields is now also converted.

* The variable attributes `host`, `reference`, `source`, `tier`, `user` and `version` should rather be global attributes

This happened due to a change in Iris. I updated the function set_global_atts in PR #3717

* Not sure about the variable attributes `mip`, `modeling_realm` and `project_id`. Other CMORrized datasets do not have these attributes. I would suggest to either change these attributes to global attributes or to remove them.

They are also global attributes now.

* I would remove the variable attribute `comment: Note that the variable tsStderr is an uncertainty not a standard error.` from variable `tos` as this only applies to `tosStderr`, which is written to separate files

The comment is now only attached to 'tosStderr'.

* the attribute `long_name` of variable `time` should be simply `time` (instead of `reference time of sst field`)

Thanks. Done.

* similarly, the attribute `long_name` of variables `lon` and `lat` should be `longitude` and `latitude` instead of `longitude coordinate` and `latitude coordinate`

Thanks. Done.

Copy link
Contributor

@axel-lauer axel-lauer left a comment

Choose a reason for hiding this comment

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

Thanks @LisaBock for addressing all my comments. Everything looks fine to me now. The only question mark left is whether the uncertainty data tsStderr should really be regridded. Strictly speaking, we would need to know the correlation lengths in order to be able to regrid the data, which we do not have at the moment. One option could be to simply not regrid tsStderr so we can look into this at a later stage.

@LisaBock LisaBock added requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. in scientific review Data labels Jul 17, 2024
@LisaBock
Copy link
Contributor Author

Thanks @LisaBock for addressing all my comments. Everything looks fine to me now. The only question mark left is whether the uncertainty data tsStderr should really be regridded. Strictly speaking, we would need to know the correlation lengths in order to be able to regrid the data, which we do not have at the moment. One option could be to simply not regrid tsStderr so we can look into this at a later stage.

Thanks again @axel-lauer !
I removed the regridding of the uncertainty field. As the files are now much huger we might not to cmorize the whole time period for tosStderr...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Data in scientific review requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request..
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update ESACCI-SST cmorizer
2 participants