sillasgonzaga / mafs

Multiple Automatic Forecast Selection
GNU General Public License v2.0
19 stars 5 forks source link

Bad class testing #2

Closed Deleetdk closed 7 years ago

Deleetdk commented 7 years ago

Running the example command produces warnings:

> mafs::select_forecast(AirPassengers, test_size = 6, horizon = 12, error = "MAPE")
[SKIPPED OUTPUT]
Warning messages:
1: In if (class(fit) != "try-error") models[[i]] <- fit :
  the condition has length > 1 and only the first element will be used
2: In if (class(fit) != "try-error") models[[i]] <- fit :
  the condition has length > 1 and only the first element will be used
3: In if (class(fit) != "try-error") models[[i]] <- fit :
  the condition has length > 1 and only the first element will be used

These are due to a bad test for whether the fit produced an error. Don't use class(x) == "str". Use inherits(x, "str"). Example:

> fit = try({fit = lm("Species ~ wrong_name", data = iris)}, silent = T)
> fit
[1] "Error in eval(expr, envir, enclos) : object 'wrong_name' not found\n"
attr(,"class")
[1] "try-error"
attr(,"condition")
<simpleError in eval(expr, envir, enclos): object 'wrong_name' not found>
> class(fit)
[1] "try-error"
> inherits(fit, "try-error")
[1] TRUE

In this particular case, there is no trouble with the class approach because the error only has 1 class. However, in your code, the errors have >1 class and that's why you get the warning. Only the first is used by the if statement.

sillasgonzaga commented 7 years ago

There's a lot to be improved regarding tests in my package. I do confirm that the code produces warnings, but I didn't really care about them because they are harmless.

Also, great tip about inherits.

sillasgonzaga commented 7 years ago

I now used inherits() and select_forecast() does not return warnings anymore. Thank you very much, I learned something new today!