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

Fix time weights support for different in NCL function time_operations #1956

Merged
merged 3 commits into from Dec 15, 2020

Conversation

axel-lauer
Copy link
Contributor

@axel-lauer axel-lauer commented Dec 14, 2020

The NCL function time_operations (statistics.ncl) optionally calculates weights based on the number of days per month for time averaging. In the current implementation, a possibly present calendar attribute is not taken into account when calculating the number of days per month potentially giving the wrong number of days in case of a leap year. This has now been fixed by preserving the calendar attribute (if present).

Closes #1946


Checklist for technical review

  • Create an issue to discuss what you are going to do, if you haven't done so already (and add the link at the bottom)
  • The pull request has a descriptive title that can be used as a one line summary in the changelog
  • The code is composed of functions of no more than 50 lines and uses meaningful names for variables and follows the style guide

Automated checks pass, status can be seen below the pull request:

  • Circle/CI tests pass. If the tests are failing, click the Details link to find out why.
  • Preferably Codacy code quality checks pass, however a few remaining hard to solve Codacy issues are still acceptable. If there is an error, click the link to find out why. If you suspect Codacy may be wrong, please ask by commenting.
  • The documentation is building successfully on readthedocs and looks well formatted, click the Details link to see it.

Checklist for scientific review

New or updated recipe/diagnostic:

  • The recipe runs successfully on your own machine/cluster or with the @esmvalbot without any modifications to the recipe and with all data specified in the recipe

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Tested successfully for most calendars, but for for proleptic_gregorian I get the warning

days_in_month: illegal calendar = 'proleptic_gregorian'

resulting in missing values for the weights and thus a missing value for the time average. Maybe we should check if the calendar is supported by the function days_in_month?

esmvaltool/diag_scripts/shared/statistics.ncl Outdated Show resolved Hide resolved
Co-authored-by: Manuel Schlund <32543114+schlunma@users.noreply.github.com>
@axel-lauer
Copy link
Contributor Author

Tested successfully for most calendars, but for for proleptic_gregorian I get the warning

days_in_month: illegal calendar = 'proleptic_gregorian'

resulting in missing values for the weights and thus a missing value for the time average. Maybe we should check if the calendar is supported by the function days_in_month?

Thanks for spotting this. I guess checking for a valid calendar would then be a must. Since development of NCL has been discontinued, I would presume it is safe to assume that the list of supported calendars will not change any more. I'll propose some code for this...

Copy link
Contributor

@schlunma schlunma left a comment

Choose a reason for hiding this comment

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

Works as expected! 🎉

@axel-lauer axel-lauer merged commit 40afc4a into master Dec 15, 2020
@axel-lauer axel-lauer deleted the bugfix_time_operations branch December 15, 2020 11:49
@jvegreg jvegreg changed the title bugfix: time weights in time_operations Fix time weights support for different in NCL function time_operations Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ncl: time_operations uses wrong weights
2 participants