Unify handling of group coordinates in temporal statistics preprocessors

[schlunma]

Description

This PR adds the keyword argument keep_group_coordinates (by default keep_group_coordinates=False) to temporal statistics preprocessors which controls how group coordinates (like year, day_of_year) are handled. Previously, each preprocessor handled this differently. Affected preprocessors:

• hourly_statistics (previous default: keep_group_coordinates=False)
• daily_statistics (previous default: keep_group_coordinates=False)
• monthly_statistics (previous default: keep_group_coordinates=True)
• seasonal_statistics (previous default: keep_group_coordinates=True)
• annual_statistics (previous default: keep_group_coordinates=True)
• decadal_statistics (previous default: keep_group_coordinates=True)

Backwards-incompatible change #

This PR changes the default behavior of the preprocessor functions

• monthly_statistics (group coordinates month_number and year)
• seasonal_statistics (group coordinates clim_season and season_year)
• annual_statistics (group coordinate year)
• decadal_statistics (group coordinate decade)

Previously, the returned cubes of these preprocessor functions contained the corresponding group coordinates. After this PR, those coordinates are not present in the returned cubes anymore.

To restore the old behavior, set keep_group_coordinates to True, e.g. in the recipe

monthly_statistics:
  operator: mean
  keep_group_coordinates: true

or Python script

cube = monthly_statistics(cube, "mean", keep_group_coordinates=True)

See ESMValGroup/ESMValTool#4129.

---

Before you get started

• ☝ Create an issue to discuss what you are going to do

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.

• 🧪 The new functionality is relevant and scientifically sound
• 🛠 This pull request has a descriptive title and labels
• 🛠 Code is written according to the code quality guidelines
• 🧪 and 🛠 Documentation is available
• 🛠 Unit tests have been added
• 🛠 Changes are backward compatible
• 🛠 Any changed dependencies have been added or removed correctly
• 🛠 The list of authors is up to date
• 🛠 All checks below this pull request were successful

---

To help with the number pull requests:

• 🙏 We kindly ask you to review two other open pull requests in this repository

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.62%. Comparing base (82b282f) to head (5c7655f).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2787      +/-   ##
==========================================
+ Coverage   95.59%   95.62%   +0.02%     
==========================================
  Files         266      266              
  Lines       15573    15581       +8     
==========================================
+ Hits        14887    14899      +12     
+ Misses        686      682       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[schlunma]

To properly unify this, it would be nice to use the same default value for keep_group_coordinates in all preprocessors. However, this would not be fully backwards-compatible:

• keep_group_coordinates=False: even though I would prefer this option, this would probably lead to many problems with subsequent code (e.g., diagnostics) that expect certain coordinates to be present. This would also mean that the default behavior of 4 of 6 preprocessors will change.
• keep_group_coordinates=True: this should be relatively safe since the presence of coordinates is usually a much smaller problem than their absence. In this case, the default behavior of only 2 of 6 preprocessors would change.

@ESMValGroup/technical-lead-development-team opinions?

[jlenh]

To properly unify this, it would be nice to use the same default value for keep_group_coordinates in all preprocessors. However, this would not be fully backwards-compatible:

• keep_group_coordinates=False: even though I would prefer this option, this would probably lead to many problems with subsequent code (e.g., diagnostics) that expect certain coordinates to be present. This would also mean that the default behavior of 4 of 6 preprocessors will change.
• keep_group_coordinates=True: this should be relatively safe since the presence of coordinates is usually a much smaller problem than their absence. In this case, the default behavior of only 2 of 6 preprocessors would change.

@ESMValGroup/technical-lead-development-team opinions?

Not sure where we stand on this. Maybe it would require some further discussions at the next @ESMValGroup/technical-lead-development-team meeting before considering merging?

[bouweandela]

even though I would prefer this option, this would probably lead to many problems with subsequent code (e.g., diagnostics) that expect certain coordinates to be present.

Could you do a grep through the recipes in ESMValTool so we can get an estimate of how many recipes would be affected?

[jlenh]

even though I would prefer this option, this would probably lead to many problems with subsequent code (e.g., diagnostics) that expect certain coordinates to be present.

Could you do a grep through the recipes in ESMValTool so we can get an estimate of how many recipes would be affected?

Long story short, following Manuel's initial comment, switching by default to use keep_group_coordinates=True for all preprocessors for minimal changes would impact three cmorizers (WFDE5, ESA-CCI Cloud, ERA-Interim) and a recipe (ERA5 daily).

(Hopefully did not miss anything while copying)

• hourly_statistics (default: keep_group_coordinates=False)
  • Did not come up in cmorizers and/or diagnostic scripts and/or recipes
• daily_statistics (default: keep_group_coordinates=False)
  • Cmorizer WFDE5 script
  • Cmorizer ESA-CCI Cloud script
  • Cmorizer ERA-interim script
  • Recipe daily ERA5 script
• monthly_statistics (default: keep_group_coordinates=True)
  • Cmorizer WFDE5 script
  • Cmorizer REGEN script
  • Cmorizer ESA-CCI Sea-ice script
  • Cmorizer APHRO-MA script
  • Cmorizer OSI script
  • Cmorizer E-OBS script
  • Cmorizer ESA-CCI Soil moisture script
  • Cmorizer ESDC script
  • Cmorizer ERA-Interim script
  • Cmorizer ESA-CCI Cloud script
  • Recipe MPQB XCH4 script
  • Recipe climate patterns script
  • Recipe hydro forcing script
• seasonal_statistics (default: keep_group_coordinates=True)
  • Diagnostic eddy growth rate script
  • Recipe monitor script
  • Recipe climate change hotspots script
• annual_statistics (default: keep_group_coordinates=True)
  • Diagnostic COS22ESSD script
  • Diagnostic eddy growth rate script
  • Recipe TCRE script
  • Recipe Python for CI script
  • Recipe model evaluation basics script
  • Recipe ZEC script
  • Recipe sea ice sensitivity script
  • Recipe REF ozone script
  • Recipe calculate GWL exceedance script
  • Recipe KCS script
  • Recipe MPQB XCH4 script
  • Recipe Schlund 2020 script 1 script 2 script 3
  • Recipe Lauer 2022 script
  • Recipe ocean biogeochemistry script
  • Recipe Wenzel 2016 script
  • Recipe monitor with refs script
  • Recipe Climwip Brunner 2020 script
  • Recipe Easy IPCC script
  • Recipe Python script
  • Recipe Climwip Brunner 2019 script
  • Recipe Climwip test basic script
  • Recipe Climwip test performance sigma script
• decadal_statistics (default: keep_group_coordinates=True)
  • Recipe ocean biogeochemistry script
  • Recipe Tebaldi 2021 ESD script

[bouweandela]

Thank you for the summary, and I apologize that my question was not clear. What I meant was that I agree that setting keep_group_coordinates=False by default would be the cleanest solution. To estimate the impact of that, I would like to know the number of recipes with a diagnostic script that actually uses the group coordinates.

CMORizers should always discard the group coordinates, as these are not part of the variable definition in the CMOR tables.

[jlenh]

Thank you for the summary, and I apologize that my question was not clear. What I meant was that I agree that setting keep_group_coordinates=False by default would be the cleanest solution. To estimate the impact of that, I would like to know the number of recipes with a diagnostic script that actually uses the group coordinates.

CMORizers should always discard the group coordinates, as these are not part of the variable definition in the CMOR tables.

Aaaah I understand now! The number of recipes is greatly reduced it seems (hopefully I haven't missed any).
Here is the summary then for the group coordinates with by default keep_group_coordinates=True:

• monthly_statistics (default: keep_group_coordinates=True)
  • Recipe monitor script
  • Recipe ENSO Basic climatology script
  • Recipe climate patterns script
• seasonal_statistics (default: keep_group_coordinates=True)
• annual_statistics (default: keep_group_coordinates=True)
  • Recipe calculate GWL exceedance script
• decadal_statistics (default: keep_group_coordinates=True)

[bouweandela]

In that case, it looks like only four recipes would be affected and we could change the default to keep_group_coordinates=False without creating a lot of work on updating existing recipes.

[schlunma]

Changed this here because cube_div was potentially undefined

[schlunma]

Set keep_group_coordinates=False for all preprocessors in 5c7655f. Broken recipes and CMORizers need to be updated in subsequent PRs. Due to the number of potentially affected CMORizers/recipe, I have NOT tested this.