robjhyndman / forecast

Forecasting Functions for Time Series and Linear Models
http://pkg.robjhyndman.com/forecast
1.1k stars 340 forks source link

Failing test for 'bizdays()' due to bug fix in 'timeDate' #913

Closed GeoBosh closed 1 year ago

GeoBosh commented 1 year ago

Recently I took over maintenance of package timeDate and fixed some missing holidays for financial centre "TSX" (Toronto stock exchange) where the Labour day was missing from the holidays. This fails the reverse dependency check on CRAN for package "forecast", in that in file "test-Calendar.R" the comparison below fails.

   ...
   b1 <- bizdays(gas, FinCenter = "NERC")
    b2 <- bizdays(gas, FinCenter = "Tokyo")
    expect_equal(sum(abs(b1 - b2)), 244L)

Looking in the body of forecast::bizdays I see the following:

    ...
    else if (FinCenter == "NERC") {
        holidays <- timeDate::holidayNERC(years)
    }
    else if (FinCenter == "Tokyo") {
        holidays <- timeDate::holidayTSX(years)
    }

The above mentioned fix in holidayTSX changes the number of business days, so the failure of the test is expected. As a side note, I wonder if in the 'if' clause above 'Tokyo' should be 'Toronto'.

I wonder if you could consider disabling this test temporarily or changing it with some (shorter)) time span with verified business days? timeDate has not been really updated since around 2011 and it is likely that further fix(es) in timeDate would be needed to improve its historical accuracy. There are some new holidays, as well. I just added the US juneteenth holiday which was made national holiday last year and became a holiday for NYSE this year.

Thanks, Georgi Boshnakov

robjhyndman commented 1 year ago

Thanks. Fixed in https://github.com/robjhyndman/forecast/commit/c04e47b5f248d87e0ad623ecb22ec86fc0069d7c and https://github.com/robjhyndman/forecast/commit/9438a54deb0c979267dad397ee8c80675211bf91.

When do you need this on CRAN?

GeoBosh commented 1 year ago

Thanks, CRAN accepted my explanation about the failing test in forecast and published timeDate. I hadn't noticed the failure when checking the reverse dependencies since revdepcheck had skipped 'forecast' for some reason.

robjhyndman commented 1 year ago

OK. I'll update on CRAN in the next week.