Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Lubridate error due to leapday + suggestion of using multiple en dates #72

Closed
gforge opened this issue Sep 6, 2015 · 3 comments
Closed

Comments

@gforge
Copy link
Contributor

gforge commented Sep 6, 2015

Leap day issue

It seems that leap days are causing issues. Although this should be fixed in the lubridate package this is not entirely the case, the following throws an error:

int <- new("Interval"
    , .Data = 2121342802
    , start = structure(-1446595200, class = c("POSIXct", "POSIXt"), tzone = "UTC")
    , tzone = "UTC"
)

tspan <- new("Period"
             , .Data = 0
             , year = 1
             , month = 0
             , day = 0
             , hour = 0
             , minute = 0
)

int%/%tspan
Error in while (any(start + est * per < end)) est[start + est * per <  : 
  missing value where TRUE/FALSE needed

Multiple dates

I suggest that you add the option of having multiple end dates, this is useful when calculating the time to event data.

Suggested solution

The following change the pin_age use the lubridate:::as.period() together with an option of having end dates specified:

pin_age <- function (pin, date = Sys.Date(), timespan = "years") 
{
  if (length(date) == 1) {
    message("The age has been calculated at ", as.character(date), 
            ".")
  } 
  else if (length(date) == length(pin)){
    message("The age is calculated relative to the '", deparse(substitute(date)), "' date")
  }
  else {
    stop("Multiple dates used.")
  }
  date <- as.Date(date)
  if (!is.pin(pin)) 
    pin <- as.pin(pin)
  all_pins <- pin
  pin <- all_pins[!is.na(all_pins)]
  diff <- lubridate::interval(pin_to_date(pin), lubridate::ymd(date))
  timespan_lubridate <- switch(timespan, years = lubridate::years(1), 
                               months = lubridate::new_period(month = 1), weeks = lubridate::weeks(1), 
                               days = lubridate::days(1))
  age <- as.integer(lubridate:::as.period(diff)%/%timespan_lubridate)
  if (any(age < 0)) 
    warning("Negative age(es).")
  all_age <- rep(as.integer(NA), length(all_pins))
  all_age[!is.na(all_pins)] <- age
  all_age
}
@MansMeg
Copy link
Contributor

MansMeg commented Sep 6, 2015

Hi Max!

Thanks for the suggestions! Regarding leapyear this is a known bug (see #26 ). The problem is fixed in lubridate 1.4 (the test cases of sweidnumbr passes) but lubridate 1.4 has not (yet) been published on CRAN. So as soon lubridate is published on CRAN this will problem will go away. If you download the latest version of lubridate this will work for you.

Regarding multiple time periods I think it is a good idea to implement it in a vectorized form. Is it possible to for the repo and add it using a merge request? The all testsuites will be run and we can see if it will make any test cases fail?

/Måns

@gforge gforge mentioned this issue Sep 6, 2015
MansMeg added a commit that referenced this issue Sep 7, 2015
@MansMeg
Copy link
Contributor

MansMeg commented Sep 7, 2015

Great job!

@MansMeg MansMeg closed this as completed Sep 7, 2015
@eribul
Copy link
Contributor

eribul commented Sep 7, 2015

Hi there @gforge!
Nice to see your name showing up here!
You probably don't know me but I am actually reading your phd thesis right now :-)
I work in the same building as Göran G and am a close friend to Szilard!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants