Does package S4Vectors break ifelse?
2
2
Entering edit mode
@peter-langfelder-4469
Last seen 12 months ago
United States

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
[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

S4Vectors ifelse • 1.4k views
2
Entering edit mode
@herve-pages-1542
Last seen 8 hours ago
Seattle, WA, United States

Hi Peter,

All S4Vectors is doing is turning ifelse() into a generic function with dispatch on its 3 arguments. As a result, the arguments passed to it get evaluated before dispatch can happen, like for any generic function. I understand this changes a little bit the contract of the ifelse() function. According to its man page:

‘yes’ will be evaluated if and only if any element of ‘test’ is true, and analogously for ‘no’.

One workaround is for you to use if else instead. See this recommendation from Martin Maechler:

If this is too much of an annoyance we could also get rid of the ifelse methods for Rle objects defined in S4Vectors:

> showMethods("ifelse")
Function: ifelse (package base)
test="ANY", yes="ANY", no="ANY"
test="ANY", yes="ANY", no="Rle"
test="ANY", yes="Rle", no="ANY"
test="ANY", yes="Rle", no="Rle"

These methods were added a long time ago when Rle objects were implemented in IRanges. I moved them from IRanges to S4Vectors when I did the IRanges/S4Vectors split. I don't know how much use they get.

This is a little bit extreme though. What other alternatives are there?

Cheers,

H.

0
Entering edit mode

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...

0
Entering edit mode

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!

0
Entering edit mode

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 for ifelse() it does have consequences. Note however that code that relies on one of the yes or no argument to remain non-evaluated is VERY LIKELY to be code that should have used if else instead of ifelse(). Said otherwise, when the test argument has a length > 1 (the legitimate case for using ifelse), it doesn't seem like a good idea to rely on one of the yes or no 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.

0
Entering edit mode

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))

0
Entering edit mode

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.

1
Entering edit mode

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.

a <- "foo"
ifelse(is.numeric(a), yes=a+5, no=a)

Error in a + 5 : non-numeric argument to binary operator

2
Entering edit mode
balwierz ▴ 40
@balwierz-8583
Last seen 2.2 years ago
United Kingdom

After 2 years it is still broken. This time IRanges introduce faulty code.

a <- "foo"
ifelse(is.numeric(a), yes=a+5, no=a)

[1] "foo"

library(IRanges)
ifelse(is.numeric(a), yes=a+5, no=a)

Error in a + 5 : non-numeric argument to binary operator

3
Entering edit mode

Hi,

Michael replaced the ifelse generic and methods with a new ifelse2 generic and methods in IRanges 2.11.8.  So AFAICT no Bioconductor package defines any ifelse method anymore.

Cheers,

H.

0
Entering edit mode

Merci