-
Notifications
You must be signed in to change notification settings - Fork 128
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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 inK
(fields also look like data are inK
) - The variable attributes
host
,reference
,source
,tier
,user
andversion
should rather be global attributes - Not sure about the variable attributes
mip
,modeling_realm
andproject_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 variabletos
as this only applies totosStderr
, which is written to separate files - the attribute
long_name
of variabletime
should be simplytime
(instead ofreference time of sst field
) - similarly, the attribute
long_name
of variableslon
andlat
should belongitude
andlatitude
instead oflongitude coordinate
andlatitude coordinate
Co-authored-by: Axel Lauer <[email protected]>
… into update_esacci_sst
Co-authored-by: Axel Lauer <[email protected]>
Co-authored-by: Axel Lauer <[email protected]>
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:
Cmor units are degC, the fields is now also converted.
This happened due to a change in Iris. I updated the function
They are also global attributes now.
The comment is now only attached to 'tosStderr'.
Thanks. Done.
Thanks. Done. |
There was a problem hiding this 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.
Thanks again @axel-lauer ! |
Description
Update ESACCI-SST cmorizer to v3.0 (daily and monthly data).
I changed the ESACCI-SST data to the variable
tos
instead ofts
because the observation field is only defined over ocean and not the whole globe likets
.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