search

gsucarrat / gets

R Package for General-to-Specific (GETS) modelling and Indicator Saturation (ISAT) methods
8 stars 5 forks source link

standardise normality test argument behaviour #12

Closed jkurle closed 4 years ago

jkurle commented 4 years ago

In most functions, the argument normality.JarqueB has a default of NULL (in which case no normality test is done) or the user can instead specify a significance level for the test by giving a numeric argument between 0 and 1. This is for example the case for getsm(), getsv(), isat(), and internally getsFun(). However, the behaviour of the arx() function differs, where the argument can only be a logical value and the default is FALSE. In the arx() function itself, FALSE is then converted to NULL to conform to the inputs of the diagnostics() function. TRUE is converted to a numeric, i.e. 1, which is then used by diagnostics(). It is used as the significance level, i.e. the probability of a type I error is 1.

As far as I have seen, this is not an issue in practice because the arx() function calls diagnostics() with argument verbose = TRUE, so it returns the full info on the test result (test statistic and p-value). I think it makes the code fragile though because if this is changed to verbose = FALSE, then only the binary result of the normality test (passed or not) is returned and in this case, a significance level of 1 is problematic.

I therefore suggest changing the code in arx() accordingly to conform to the usual default = NULL and significance level otherwise setting. I am happy to implement these changes if the others also think that it makes sense.

gsucarrat commented 4 years ago

You definitely have a point! But in arx() the purpose of the normality.JarqueB argument is to decide on whether to include a normality test of the residuals. The natural pair of possible values is therefore TRUE/FALSE. Whenever the natural range of possible values is not TRUE/FALSE, however, as for example in the normality.JarqueB argument in getsm(), then the default is NULL. This is the general rule that is followed, not only in the 'gets' package, but also in other packages. If we change the default of normality.JarqueB to NULL, then we would break this general rule. Having said this, there is indeed an inconsistency across function.