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

incorrect identification of leap years; multiple implementations. #801

Closed
3 tasks
dlebauer opened this issue Apr 21, 2016 · 2 comments
Closed
3 tasks

incorrect identification of leap years; multiple implementations. #801

dlebauer opened this issue Apr 21, 2016 · 2 comments

Comments

@dlebauer
Copy link
Member

dlebauer commented Apr 21, 2016

The logic to test if a year is a leap year has been implemented perhaps a dozen times independently within the PEcAn code base, including once as the function is.leap.
2. there are many cases where year %% 4 i== 0 is used although it should be year %% 400 == 0 | (year %% 4 == 0 & year %% 100 != 0) (is.leap and leap_year implement this correctly).

  1. The lubridate package has a function leap_year that returns T/F. We already use this function in many places and I suggest that this is a good solution
  2. (just a comment / food for thought) testing if a year is a leap year is often followed by calculating time steps (e.g. days or seconds/year). lubridate also provides convenience functions for this that would be less error prone than declaring seconds / year or days / month constants repeatedly in the code.

TODO

  • bugfix: All of the invalid uses of year %% 4 i== 0 should be replaced by a valid function (leap_year)
  • enhancement: deprecate is.leap and replace it with lubridate::leap_year
  • documentation add some notes about existing 'helper' functions to the model template README
@robkooper
Copy link
Member

Other handy functions to add:

days_in_year <- function(year) {
  ifelse(lubridate:: leap_year(year), 366, 365)
}

hours_in_year <- function(year, every=24) {
  24/every * ifelse(lubridate:: leap_year(year), 366, 365)
}

now we can simply call `24/6 * days_in_year(1999)`` to get the value for every 6 hours.

@ashiklom
Copy link
Member

ashiklom commented Sep 7, 2017

Closed by #1623.

@ashiklom ashiklom closed this as completed Sep 7, 2017
@ashiklom ashiklom moved this from Completed, in review to Closed in PEcAn bugfix sprint: DUE Sep 15th. Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

No branches or pull requests

5 participants