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 recipe and diagnostic scripts to compute figures of D9.4 of ISENES3 #2441

Merged
merged 16 commits into from Feb 16, 2022

Conversation

sloosvel
Copy link
Contributor

@sloosvel sloosvel commented Dec 14, 2021

This pull request adds a new example recipe and diagnostic script that compares the global average of tas for DCPP data with an observational dataset. It exemplifies the use of the new timerange tag.

Description


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 recipe/diagnostic

To help with the number of pull requests:

@sloosvel sloosvel added diagnostic is-enes requires new ESMValCore release A new release of ESMValCore is needed to solve this issue/merge this pull request.. labels Dec 14, 2021
@remi-kazeroni remi-kazeroni added this to the v2.5.0 milestone Dec 15, 2021
@schlunma
Copy link
Contributor

schlunma commented Feb 9, 2022

@sloosvel Release v2.5 is approaching quickly (the feature freeze will be on 2022-02-21). Since this pull request is marked for v2.5, could you briefly comment if you further intend to include this in the new release and/or if you are facing any problems with it? Thanks!!

@remi-kazeroni
Copy link
Contributor

I will proceed with the review shortly and would like to have this in v2.5. Shall I wait for the next release candidate @schlunma?

@sloosvel, would you have the possibility to check the failing test and Codacy issues and also edit the checklist? Thanks!

@schlunma
Copy link
Contributor

schlunma commented Feb 9, 2022

Shall I wait for the next release candidate @schlunma?

Yes, that might be a good idea. I will do this as soon as ESMValGroup/ESMValCore#1486 is merged.

@sloosvel
Copy link
Contributor Author

sloosvel commented Feb 9, 2022

I'll push the codacy fixes tomorrow, the rest of the tests are failing because timerange is still not recognised as a valid time tag and the tool is expecting start_year and end_year instead.

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 creating this recipe and this diagnostic @sloosvel! I ran the recipe successfully on Mistral at the first attempt. I used the v2.5-rc2 for the Core. Everything looks fine: provenance is recorded, plots are identical to the IS-ENES3 deliverable D9.4, a filled copy of the recipe is created without the wildcards. My only question would be: do you want to add an entry to the documentation page for this recipe and maybe add some information and/or a plot to the documentation?

esmvaltool/recipes/examples/recipe_decadal.yml Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/examples/decadal_example.py Outdated Show resolved Hide resolved
esmvaltool/diag_scripts/examples/decadal_example.py Outdated Show resolved Hide resolved
@sloosvel
Copy link
Contributor Author

Thanks for the review @remi-kazeroni ! I adressed the comments but tests are failing now:

Invalid value encountered for timerange. Valid value must follow ISO 8601 standard for dates and duration periods, or be set to '*' to load available years. Got ['0000', '9999'] instead.

It looks like years 0000 and 9999 from the test dataset cannot be parsed. I can update the test to have dates that can be parsed but is this something that should be fixed in the core as well? How likely is it to run into these kind of dates in real data?

@zklaus
Copy link
Contributor

zklaus commented Feb 16, 2022

In normal calendars, there is no year zero. Additionally, CF uses year 0 to indicate climatologies in some cases (see CF Conventions 1.9, Section 4.4.1), so this really should not appear in time range selection. I think that year 9999 is probably no problem.

@sloosvel
Copy link
Contributor Author

Fixed! Is there something going on with the documentation build? It failed in another pull request as well and for the same reason that in this one.

@zklaus
Copy link
Contributor

zklaus commented Feb 16, 2022

Is there something going on with the documentation build?

This was fixed in #2531. Merging main will solve it here too.

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 making the changes @sloosvel! Everything looks good to me. This PR is all green and ready to be merged imho.

@schlunma schlunma merged commit d5b77c6 into main Feb 16, 2022
@schlunma schlunma deleted the dev_decadal_example branch February 16, 2022 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
diagnostic is-enes 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.

Add recipe with examples using decadal data
4 participants