gsucarrat / gets

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

'uis' argument in isat() #18

Closed gsucarrat closed 4 years ago

gsucarrat commented 4 years ago

As of version 0.25, the following should - but does not - work:

set.seed(123); y <- rnorm(30);  x <- sim(y)
isat(y, sis=FALSE, uis=x)

Interestingly, the following does:

z <- matrix(rnorm(30*29), 30, 29)
isat(y, sis=FALSE, uis=z)
moritzpschwarz commented 4 years ago

Ah, this is a really annoying bug.

The reason for this is that in the isat function there is now a check to ensure that any saturation method is selected.

if (sis == FALSE && iis == FALSE && tis == FALSE && uis == FALSE) {
    stop("No Indicator Selection Method was selected. Either set iis, sis or tis as TRUE or specify uis.")
  }

The isat standard argument for uis is FALSE. The initial bug comes from me not realising that all iis, sis and tis will only ever take the values TRUE or FALSE, while uis takes either FALSE or a matrix/zoo/etc. with values that are not logical.

In your example, x is a matrix of only 0 and 1 - which if I test it for uis == FALSE changes all 0 values to FALSE and all 1 to TRUE. This allows the statement in the if function to return TRUE because some elements of that matrix now actually do equal FALSE == FALSE. So this returns TRUE (even though it only does so for certain rows, but that's beside the point).

This is not the case for z, as the matrix is not automatically converted to logical values because the values are not only 0 and 1. Hence the condition z == FALSE gives FALSE and the if statement is skipped.

While there might be more beautiful ways to fix this, this will deal with this problem (wrapping uis == FALSE in all())

if (sis == FALSE && iis == FALSE && tis == FALSE && all(uis == FALSE)) {
    stop("No Indicator Selection Method was selected. Either set iis, sis or tis as TRUE or specify uis.")
}
gsucarrat commented 4 years ago

Your proposed solution looks good, but will it handle situations where uis is a list? The following yields an error:

x <- matrix(0,3,2); z <- matrix(1,3,4); mylist <- list(x,z)
all( mylist==FALSE )

One possibility you could try instead is identical(uis, FALSE).

moritzpschwarz commented 4 years ago

Your proposed solution looks good, but will it handle situations where uis is a list? The following yields an error:

x <- matrix(0,3,2); z <- matrix(1,3,4); mylist <- list(x,z)
all( mylist==FALSE )

One possibility you could try instead is identical(uis, FALSE).

Yes identical is definitely better than all! Thanks!

This could work as well, but is more tedious (and I prefer your identical solution...): if(is.logical(uis)){uis==FALSE} else {FALSE}

gsucarrat commented 4 years ago

Thumbs up;)