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

Modernize regrid_time and allow setting a common calendar for decadal, yearly, and monthly data #2311

Merged
merged 16 commits into from
Apr 26, 2024

Conversation

schlunma
Copy link
Contributor

@schlunma schlunma commented Jan 26, 2024

Description

This PR modernizes regrid_time (proper documentation, type hints, proper error raising, etc.) and allows to set a custom fixed calendar for decadal, yearly, and monthly data. In addition, it makes get_time_bounds more flexible (allow time units that are not days since ..., allow all n-hourly data where n is divisor of 24).

Example:

  regrid_time:
    calendar: standard
    units: days since 2000-01-01  # this is optional

Closes #2176
Related to #2106

Link to documentation: https://esmvaltool--2311.org.readthedocs.build/projects/ESMValCore/en/2311/recipe/preprocessor.html#regrid-time


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.


To help with the number pull requests:

@schlunma schlunma added the preprocessor Related to the preprocessor label Jan 26, 2024
@schlunma schlunma added this to the v2.11.0 milestone Jan 26, 2024
@schlunma schlunma requested a review from axel-lauer January 26, 2024 15:44
@schlunma schlunma self-assigned this Jan 26, 2024
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.33%. Comparing base (afde692) to head (5684f45).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2311   +/-   ##
=======================================
  Coverage   94.32%   94.33%           
=======================================
  Files         246      246           
  Lines       13599    13617   +18     
=======================================
+ Hits        12827    12845   +18     
  Misses        772      772           

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

Copy link
Contributor

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

looking very good, and a very useful addition to its functionality, cheers, Manu! Just a small docs suggestion from me, and a question about decadal bounds: isn't it better to have +/-10 years ie two straddling decades, rather than +/-5 years? the other shorter frequencies all have +/- 1 unit of frequency for bounds. just a thought - maybe the +/- 5 years is a norm I'm not aware of. Cheers, bud 🍺

@schlunma
Copy link
Contributor Author

No, the other frequencies also have +/- 1/2 of the input frequency as bounds, see here:

        if 'dec' in freq:
            min_bound = datetime(date.year - 5, 1, 1, 0, 0)
            max_bound = datetime(date.year + 5, 1, 1, 0, 0)
        elif 'yr' in freq:
            min_bound = datetime(date.year, 1, 1, 0, 0)
            max_bound = datetime(date.year + 1, 1, 1, 0, 0)
        elif 'mon' in freq or freq == 'mo':
            next_month, next_year = get_next_month(date.month, date.year)
            min_bound = datetime(date.year, date.month, 1, 0, 0)
            max_bound = datetime(next_year, next_month, 1, 0, 0)
        elif 'day' in freq:
            min_bound = date - timedelta(hours=12.0)
            max_bound = date + timedelta(hours=12.0)
        elif 'hr' in freq:
            (n_hours_str, _, _) = freq.partition('hr')
            if not n_hours_str:
                n_hours = 1
            else:
                n_hours = int(n_hours_str)
            if 24 % n_hours:
                raise NotImplementedError(
                    f"For `n`-hourly data, `n` must be a divisor of 24, got "
                    f"'{freq}'"
                )
            min_bound = date - timedelta(hours=n_hours / 2.0)
            max_bound = date + timedelta(hours=n_hours / 2.0)

E.g, for year it's 1 Jan for the current year and 1 Jan for the next year, etc. So I think decadal data should be +/-5 years as well.

@valeriupredoi
Copy link
Contributor

you're totally right, Manu! My silly overstepping on time in me head 🐴 Will merge this then, let me give it another once over

@valeriupredoi valeriupredoi added the enhancement New feature or request label Apr 26, 2024
@valeriupredoi
Copy link
Contributor

good to merge! Would you like to get a review from @axel-lauer as well? I see him listed in the reviewers' list

@schlunma
Copy link
Contributor Author

Yes, if he's fine with that! I think you already tested that, right @axel-lauer?

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.

Yes, I tested this already. It works nicely and I used this to create a plot for our benchmarking paper.

@valeriupredoi
Copy link
Contributor

great stuff, cheers, guys!

@valeriupredoi valeriupredoi merged commit e147a51 into main Apr 26, 2024
6 checks passed
@valeriupredoi valeriupredoi deleted the better_regrid_time branch April 26, 2024 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request preprocessor Related to the preprocessor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

regrid_time does not have promised functionality
3 participants