>>>>> Duncan Murdoch >>>>> on Thu, 2 Jan 2025 11:28:45 -0500 writes:> On 2025-01-02 11:20 a.m., Duncan Murdoch wrote: >> On 2025-01-02 9:04 a.m., Norbert Kuder wrote: >>> Hello all, >>> >>> I am running R version 4.4.2 (2024-10-31 ucrt) on Windows 10 x64, and >>> noticed something that might be a minor bug (or at least inconsistent code) >>> in the stats/arima.R package. >>> I have found: >>> 1. A missing stop() call at line 69: >>> if (length(order) == 3) seasonal <- list(order = seasonal) else >>> ("\'seasonal\' is of the wrong length") >>> it should be rather: >>> if (length(order) == 3) seasonal <- list(order = seasonal) else >>> stop("\'seasonal\' is of the wrong length") >> >> I think you're right about this one. >> >>> >>> 2. An unused 'mod' variable assignment at line 190: >>> >>> mod <- makeARIMA(trarma[[1]], trarma[[2]], Delta, kappa, SSinit) >>> >>> I am trying to confirm whether this is intended behavior or possibly an >>> overlooked detail. Could someone please clarify if the current logic is >>> correct? >>> >> >> In the R-devel source I see mod being used in the next statement: >> >> mod <- makeARIMA(trarma[[1L]], trarma[[2L]], Delta, kappa, SSinit) >> val <- if(ncxreg > 0L) >> arimaSS(x - xreg %*% coef[narma + (1L:ncxreg)], mod) >> else arimaSS(x, mod) >> >> It appears in both alternatives of the if statement. >> >> This one is strange though: the code in the github mirror >> (https://github.com/wch/r-source/blob/4a1ed749271c52e60a85e794e6f34b0831efb1ae/src/library/stats/R/arima.R#L256-L258) >> is different: >> >> mod <- makeARIMA(trarma[[1L]], trarma[[2L]], Delta, kappa, SSinit) >> if(ncxreg > 0) x <- x - xreg %*% coef[narma + (1L:ncxreg)] >> arimaSS(x, mod) >> >> yet the log shows no recent changes. I'm not sure what's going on. > Mystery solved: code like this appears several times in that file. In > the occurrence here: > https://github.com/wch/r-source/blob/4a1ed749271c52e60a85e794e6f34b0831efb1ae/src/library/stats/R/arima.R#L293 > it does appear that the mod value isn't being used. > Duncan Murdoch Thank you, Norbert and Duncan. A little bit (unfinished) aRcheology showed that both parts have been in the arima code since Dec 11 2003 (when the code, i.e., the whole package 'ts') was moved / merged into package 'stats'. I'll fix and quickly test the change, and then commit it. Congratulations indeed to Norbert Kuder for finding such (small) blemishes of such an age! ... and Happy New yeaR! to all readers. Martin
>>>>> Martin Maechler on Thu, 2 Jan 2025 20:42:58 +0100 writes:>>>>> Duncan Murdoch on Thu, 2 Jan 2025 11:28:45 -0500 writes: >> On 2025-01-02 11:20 a.m., Duncan Murdoch wrote: >>> On 2025-01-02 9:04 a.m., Norbert Kuder wrote: >>>> Hello all, >>>> >>>> I am running R version 4.4.2 (2024-10-31 ucrt) on Windows 10 x64, and >>>> noticed something that might be a minor bug (or at least inconsistent code) >>>> in the stats/arima.R package. >>>> I have found: >>>> 1. A missing stop() call at line 69: >>>> if (length(order) == 3) seasonal <- list(order = seasonal) else >>>> ("\'seasonal\' is of the wrong length") >>>> it should be rather: >>>> if (length(order) == 3) seasonal <- list(order = seasonal) else >>>> stop("\'seasonal\' is of the wrong length") >>> >>> I think you're right about this one. well, actually, the mishap is larger: Reading the help page for arima, 'seasonal' is documented as seasonal: A specification of the seasonal part of the ARIMA model, plus the period (which defaults to ?frequency(x)?). This may be a list with components ?order? and ?period?, or just a numeric vector of length 3 which specifies the seasonal ?order?. In the latter case the default period is used. Note the or just a numeric vector of length 3 ... the seasonal 'order' part. If you look at the larger context of the else ("'seasonal... part, it becomes clear that -- in order to fulfill the above documented behavior, it's not length(order), but length(seasonal) which should be 3 which leads to the following change : @@ -124,10 +124,11 @@ if(!is.numeric(seasonal$order) || length(seasonal$order) != 3L || any(seasonal$order < 0L)) stop("'seasonal$order' must be a non-negative numeric vector of length 3") - } else if(is.numeric(order)) { - if(length(order) == 3L) seasonal <- list(order=seasonal) - else ("'seasonal' is of the wrong length") - } else stop("'seasonal' must be a list with component 'order'") + } else if(is.numeric(seasonal)) { # meant to be seasonal$order + if(length(seasonal) != 3L || any(seasonal < 0)) + stop("if not a list, 'seasonal' must be a non-negative numeric vector of length 3") + seasonal <- list(order=seasonal) + } else stop("'seasonal' must be a list with component 'order' or length-3 vector") ... I still plan to commit this, but it may well be that this change will wake up arima() use that was buggy and never detected till now. Martin
>>>>> Martin Maechler on Thu, 2 Jan 2025 20:42:58 +0100 writes:>>>>> Duncan Murdoch on Thu, 2 Jan 2025 11:28:45 -0500 writes: >> On 2025-01-02 11:20 a.m., Duncan Murdoch wrote: >>> On 2025-01-02 9:04 a.m., Norbert Kuder wrote: >>>> Hello all, >>>> >>>> I am running R version 4.4.2 (2024-10-31 ucrt) on Windows 10 x64, and >>>> noticed something that might be a minor bug (or at least inconsistent code) >>>> in the stats/arima.R package. .................... >>>> 2. An unused 'mod' variable assignment at line 190: >>>> >>>> mod <- makeARIMA(trarma[[1]], trarma[[2]], Delta, kappa, SSinit) >>>> >>>> I am trying to confirm whether this is intended behavior or possibly an >>>> overlooked detail. Could someone please clarify if the current logic is >>>> correct? >> Mystery solved: code like this appears several times in that file. In >> the occurrence here: >> https://github.com/wch/r-source/blob/4a1ed749271c52e60a85e794e6f34b0831efb1ae/src/library/stats/R/arima.R#L293 >> it does appear that the mod value isn't being used. >> Duncan Murdoch > Thank you, Norbert and Duncan. > A little bit (unfinished) aRcheology showed that both parts > have been in the arima code since Dec 11 2003 (when the code, i.e., the > whole package 'ts') was moved / merged into package 'stats'. > I'll fix and quickly test the change, and then commit it. and testing quickly revealed what is obvious in hindsight: When minimizing the negative log likelihood, in optim(<p>, armafn, ...) the armafn() is called of course and it *does* need the model, i.e., `mod`. ==> the 2nd "inconsistency was *not* a mistake at all: The mod <- makeARIMA(....) line has clearly been necessary all along. Martin