Hi all,
I stumbled upon a weirdness caused by loading of package S4Vectors. which seems to break the ifelse function. Here's the example code (maybe not the simplest, but hopefully simple enough):
################ Code begin
testFnc = function(exprData, colors, universalColors = NULL,
grey = ifelse(is.null(universalColors), colors, "grey"))
{
print(is.null(universalColors));
print( grey);
}
expr1 = 1;
labels1 = 1
MEs1 = testFnc(expr1, universalColors = labels1)
library(S4Vectors)
MEs1 = testFnc(expr1, universalColors = labels1)
################# Code end
The first call to testFnc produces the expected result:
[1] FALSE
[1] "grey"
The second call to testFnc, after loading S4Vectors, produces the following output:
[1] FALSE
Error in ifelse(is.null(universalColors), colors, "grey") :
error in evaluating the argument 'yes' in selecting a method for function 'ifelse': Error: argument "colors" is missing, with no default
This means that even though the 'test' in the ifelse is FALSE, the yes argument is evaluated. This breaks one my WGCNA functions... thanks in advance for looking into it!
Peter
> sessionInfo()
R version 3.2.2 beta (2015-08-05 r68859)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Fedora 20 (Heisenbug)
locale:
[1] LC_CTYPE=en_US.UTF-8 LC_NUMERIC=C
[3] LC_TIME=en_US.UTF-8 LC_COLLATE=en_US.UTF-8
[5] LC_MONETARY=en_US.UTF-8 LC_MESSAGES=en_US.UTF-8
[7] LC_PAPER=en_US.UTF-8 LC_NAME=C
[9] LC_ADDRESS=C LC_TELEPHONE=C
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C
attached base packages:
[1] parallel stats4 stats graphics grDevices utils datasets
[8] methods base
other attached packages:
[1] S4Vectors_0.6.3 BiocGenerics_0.14.0
loaded via a namespace (and not attached):
[1] tools_3.2.2
It seems the horse has left the barn, so there's no point closing the barn doors. I would include a help file for ifelse in S4Vectors that clearly states that unlike the base ifelse, the S4Vectors ifelse (or, if it's more accurate, the dispatcher?) evaluates all 3 arguments irrespective of the actual values of 'test'. At least it would save people the headache of chasing down what looks like an obscure bug.
One thing that probably changed between Bioconductor 2.14 (where I did not have this problem) and 3.1 (where it bit me) is that one of the packages I load (not sure which one) changed from importing IRanges to depending on S4Vectors. This looks like a innocuous change, but it was all it took.
Frustrating...
Herve, are you saying this package changes the semantics of the base ifelse when used for objects that are not S4Vectors (or Rles)? If so, I would recommend that this be undone ASAP. Changing the base language like this seems like a "very bad idea". Imagine, you load a package that does some specific magic, but, as a side effect it also redefines `&&` to no longer be "short-circuited". The world could blow up!
Hi Malcolm,
Yes defining
ifelse
methods for Rle objects changes the semantic of the function when called on non-Rle objects. More generally speaking, turning a base function into an S4 generic and defining methods for it can potentially modify its semantic since the method dispatch mechanism can decide to evaluate some of the arguments that would have otherwise remained non-evaluated. Most of the case, this won't have any practical consequences but forifelse()
it does have consequences. Note however that code that relies on one of theyes
orno
argument to remain non-evaluated is VERY LIKELY to be code that should have usedif else
instead ofifelse()
. Said otherwise, when thetest
argument has a length > 1 (the legitimate case for usingifelse
), it doesn't seem like a good idea to rely on one of theyes
orno
argument to remain non-evaluated.Anyway, I agree that no package should modify the semantic of the base stuff. Furthermore I'm not particularly attached to these
ifelse
methods for Rle objects (they provide very little value) so I'm fine with removing them. Unless someone objects?H.
Hi Herve,
You can not possibly anticipate the number of hours such an interaction can introduce into somebody's day trying to debug this, or worse, possibly failing to debug it and leaving with a scowl. Its side effects like this that can make debugging the side effect of introducing a package into an environment feel like a bad game of whack-a-mole.
By my sights, you're on VERY THIN ICE with your appeal to what you think is the "VERY LIKELY" intention of the programmer who depends upon the clearly advertised semantics of ifelse in an unstated application.
I really think you should remove them ASAP; even if someone objects; they would be wrong.
~Malcolm
FWIW:
PS.
survivor$shareOfEstate <- ifelse(survivor$agreesToEqualDivision,1/nrow(survivor),lengthyCourtBattle(survivor))
Hi Malcolm,
Nobody has objected so far so let's get rid of the
ifelse()
generic.Full disclaimer: I didn't introduce it.
Cheers,
H.
Hello Herve,
I just bumped into this thread after an hour of debugging my code, which wasn't broken. ifelse is broken.
Not any more in S4vectors, but the code turning base function ifelse to a S4 function dispatched by all 3 arguments still exists in IRanges. Moreover, when I load IRanges there is no warning message of "ifelse" being masked.
Breaking base R functionality by loading a library I consider a bug.
PS.
Error in a + 5 : non-numeric argument to binary operator