Fix ERA5 native6 fix to handle single monthly-averaged NetCDF files. #2512

rbeucher · 2024-08-14T06:46:36Z

Description

Modifies the get_frequency function in ERA5 native6 fix to handle case where monthly-averaged data are stored in single NetCDF file (one file per month) and have thus a time dimension length of 1.

Closes #2511

Link to documentation:

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

rbeucher · 2024-08-14T22:43:12Z

Hi @schlunma, @valeriupredoi,

Could someone please take a moment to review this for me? Thanks!

R

codecov · 2024-11-25T22:21:06Z

Codecov Report

❌ Patch coverage is 91.30435% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.42%. Comparing base (cbb23dc) to head (0b6af74).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
esmvalcore/cmor/_fixes/native6/era5.py	91.30%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2512      +/-   ##
==========================================
- Coverage   95.46%   95.42%   -0.04%     
==========================================
  Files         260      260              
  Lines       15526    15513      -13     
==========================================
- Hits        14822    14804      -18     
- Misses        704      709       +5

☔ 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.

for more information, see https://pre-commit.ci

valeriupredoi

looking good @rbeucher - and biy did you have to wait for a review - sorry 🍺 @schlunma @sloosvel maybe we can squeeze this one in 2.12?

schlunma

This currently breaks support for daily ERA5 data that has been added in #2178.

github-actions · 2025-09-02T02:54:09Z

In order to maintain a backlog of relevant pull requests, we automatically label them as stale after 180 days of inactivity.

If this pull request is still important to you, please comment below to remove the stale label. Otherwise, this pull request will be automatically closed in 60 days. If this pull request only suffers from a lack of reviewers, please tag the @ESMValGroup/technical-lead-development-team so they can help you find a suitable reviewer.

flicj191 · 2025-09-16T02:34:27Z

I will look into this again to fix the single monthly files

flicj191 · 2025-09-17T03:45:41Z

@schlunma @valeriupredoi @ESMValGroup/esmvaltool-developmentteam
Can we get this re-checked please?
FYI @rbeucher

bouweandela · 2025-09-17T08:06:53Z

We discussed this feature in the associated issue, and there I suggested #2511 (comment). Does that not work?

flicj191 · 2025-09-17T22:45:13Z

We discussed this feature in the associated issue, and there I suggested #2511 (comment). Does that not work?

Ah I missed that sorry, I'll test it out and found that it still runs the get_frequency each time for class AllVars here:

ESMValCore/esmvalcore/cmor/_fixes/native6/era5.py

Line 570 in 9ef036a

self._fix_monthly_time_coord(cube)

ESMValCore/esmvalcore/cmor/_fixes/native6/era5.py

Lines 585 to 587 in 9ef036a

    
           def _fix_monthly_time_coord(cube): 
        
               """Set the monthly time coordinates to the middle of the month.""" 
        
               if get_frequency(cube) == "monthly":

I can change this part in this PR

flicj191 · 2025-09-18T01:38:07Z

@bouweandela I've updated, let me know what you think. thanks!

bouweandela

Looks fine now. I wonder if we could completely get rid of the get_frequency function?

It would be nice to add a unit test, could you do that?

bouweandela · 2025-09-18T16:07:01Z

esmvalcore/cmor/_fixes/native6/era5.py

+    def _fix_monthly_time_coord(cube, frequency):
        """Set the monthly time coordinates to the middle of the month."""
-        if get_frequency(cube) == "monthly":
+        if frequency in ("monthly", "mon", "mo"):


Maybe "mo" is not needed, as it looks like monthly frequency will be either "monthly" as returned by the get_frequency function or "mon" from the CMOR table:

ESMValCore/esmvalcore/cmor/tables/cmip6/Tables/CMIP6_CV.json

Lines 2460 to 2477 in 9ef036a

"frequency":{

"1hr":"sampled hourly",

"1hrCM":"monthly-mean diurnal cycle resolving each day into 1-hour means",

"1hrPt":"sampled hourly, at specified time point within an hour",

"3hr":"sampled every 3 hours",

"3hrPt":"sampled 3 hourly, at specified time point within the time period",

"6hr":"sampled every 6 hours",

"6hrPt":"sampled 6 hourly, at specified time point within the time period",

"day":"daily mean samples",

"dec":"decadal mean samples",

"fx":"fixed (time invariant) field",

"mon":"monthly mean samples",

"monC":"monthly climatology computed from monthly mean samples",

"monPt":"sampled monthly, at specified time point within the time period",

"subhrPt":"sampled sub-hourly, at specified time point within an hour",

"yr":"annual mean samples",

"yrPt":"sampled yearly, at specified time point within the time period"

},

Thanks @bouweandela, I'm not sure I can speak to the get_frequency function.. it was added for some use cases?
I have added a test for "mon" frequency repeating one for "monthly"

I have added a test for "mon" frequency repeating one for "monthly"

Great!

Thanks @bouweandela, I'm not sure I can speak to the get_frequency function.. it was added for some use cases?

It was added before we passed all the facets and their values from the recipe to the fix functions. I've thought about it some more, and it can be removed. In rare cases where the frequency is wrongly inferred from the mip, the user can specify the right frequency value in the recipe.

ah ok, should I create a new issue/PR to discuss specifically or I can just change here?

You can just change it here

flicj191

Removing get_frequency I had to parse the frequency to fix_hourly_time_coordinate and fix_accumulated_units which I think works, similar to _fix_monthly_time_coord but the tests are failing, not sure what i'm missing? the fixes aren't applied? @bouweandela

esmvalcore/cmor/_fixes/native6/era5.py

This reverts commit 4948f38.

flicj191 · 2025-10-07T22:25:38Z

I have reverted the changes of removing the get_frequency function, more testing was showing that it wasn't working if frequency wasn't defined first in the dataset which doesn't seem to be common practise so would still be useful to have.

bouweandela · 2025-10-10T13:52:09Z

@flicj191 In #2855 I've changed the test so it is more realistic and you can see that a frequency keyword argument is now always supplied.

rbeucher added 6 commits August 14, 2024 16:43

Fix ERA5 native6 fix to handle single monthly-averaged NetCDF files.

dd26339

Fix string

e87106e

Fix cosmetics

ebd5512

happy?

a4b13a1

Formatting

ce1c78c

Fix test

5bbae26

rbeucher added 2 commits November 26, 2024 07:57

Merge branch 'main' into era5_native6_monthly

485dd11

Ruff fixing

edd9564

rbeucher added the pre-commit.ci autofix label Nov 25, 2024

pre-commit-ci bot removed the pre-commit.ci autofix label Nov 25, 2024

[pre-commit.ci] auto fixes from pre-commit.com hooks

75e8346

for more information, see https://pre-commit.ci

valeriupredoi approved these changes Feb 13, 2025

View reviewed changes

Merge branch 'main' into era5_native6_monthly

552a0e9

schlunma requested changes Mar 5, 2025

View reviewed changes

github-actions bot added the Stale label Sep 2, 2025

github-actions bot removed the Stale label Sep 16, 2025

flicj191 and others added 5 commits September 16, 2025 14:36

update get time frequency

7b92ede

Merge branch 'main' into era5_native6_monthly

210dc60

remove blank line

ece7ca2

log warning

84d6994

Merge branch 'main' into era5_native6_monthly

be63ec9

flicj191 added 2 commits September 18, 2025 11:16

revert get frequency, use when freq not specified

389ea2b

revert test for fx

ac6eef6

ruff check branches

5dd56cc

bouweandela reviewed Sep 18, 2025

View reviewed changes

flicj191 added 4 commits September 23, 2025 14:09

remove mo, test mon freq

3228c72

ruff

dc00c12

remove comment

030e5e5

remove get_frequency, tests freq for some vars

4948f38

flicj191 reviewed Sep 29, 2025

View reviewed changes

esmvalcore/cmor/_fixes/native6/era5.py Show resolved Hide resolved

Revert "remove get_frequency, tests freq for some vars"

5de098e

This reverts commit 4948f38.

add to error message

9dbf26c

Fix ERA5 without get_frequency (#2855)

0b6af74

	"frequency":{
	"1hr":"sampled hourly",
	"1hrCM":"monthly-mean diurnal cycle resolving each day into 1-hour means",
	"1hrPt":"sampled hourly, at specified time point within an hour",
	"3hr":"sampled every 3 hours",
	"3hrPt":"sampled 3 hourly, at specified time point within the time period",
	"6hr":"sampled every 6 hours",
	"6hrPt":"sampled 6 hourly, at specified time point within the time period",
	"day":"daily mean samples",
	"dec":"decadal mean samples",
	"fx":"fixed (time invariant) field",
	"mon":"monthly mean samples",
	"monC":"monthly climatology computed from monthly mean samples",
	"monPt":"sampled monthly, at specified time point within the time period",
	"subhrPt":"sampled sub-hourly, at specified time point within an hour",
	"yr":"annual mean samples",
	"yrPt":"sampled yearly, at specified time point within the time period"
	},

Fix ERA5 native6 fix to handle single monthly-averaged NetCDF files. #2512

Are you sure you want to change the base?

Fix ERA5 native6 fix to handle single monthly-averaged NetCDF files. #2512

Uh oh!

Conversation

rbeucher commented Aug 14, 2024

Description

Before you get started

Checklist

Uh oh!

rbeucher commented Aug 14, 2024

Uh oh!

codecov bot commented Nov 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

valeriupredoi left a comment

Choose a reason for hiding this comment

Uh oh!

schlunma left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 2, 2025

Uh oh!

flicj191 commented Sep 16, 2025

Uh oh!

flicj191 commented Sep 17, 2025

Uh oh!

bouweandela commented Sep 17, 2025

Uh oh!

flicj191 commented Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flicj191 commented Sep 18, 2025

Uh oh!

bouweandela left a comment

Choose a reason for hiding this comment

Uh oh!

bouweandela Sep 18, 2025

Choose a reason for hiding this comment

Uh oh!

flicj191 Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

bouweandela Sep 24, 2025

Choose a reason for hiding this comment

Uh oh!

flicj191 Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

bouweandela Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

flicj191 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

flicj191 commented Oct 7, 2025

Uh oh!

bouweandela commented Oct 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov bot commented Nov 25, 2024 •

edited

Loading

flicj191 commented Sep 17, 2025 •

edited

Loading