On 25 Jun 2019, at 09:44 , peter dalgaard <pdalgd at gmail.com> wrote:
Argh! Yes you are right, the "fix" doesn't. And I fell into the same "hey it's a vector so && has to be wrong"-trap. So this has to be reverted to something that has at least failed unconspicuously for a decade.... Will do. Thanks to Martin for remaining suspicious!
[This code was originally from 2009, by John Chambers. It is not too likely that he'll remember what the intention actually was... The logic does escape me: An omitted signature is only an omitted signature if the signature of the omitted signature is not "missing"? In that case, I think
omittedSig[omittedSig] <- (signature[omittedSig] != "missing")
might work (omittedSig[omittedSig] == TRUE, so we don't need to &. That is, unless there are NA issues.), but I am not at all sure that is what is wanted!
]
-pd
On 25 Jun 2019, at 07:16 , Henrik Bengtsson <henrik.bengtsson at gmail.com> wrote:
**Maybe this bug needs to be understood further before applying the
patch because patch is most likely also wrong**
Because, from just looking at the expressions, I think neither the R
3.6.0 version:
omittedSig <- omittedSig && (signature[omittedSig] != "missing")
nor the patched version (I proposed):
omittedSig <- omittedSig & (signature[omittedSig] != "missing")
can be correct. For a starter, 'omittedSig' is a logical vector. We
see that 'omittedSig' is used to subset 'signature'. In other words,
the length of 'signature[omittedSig]' and hence the length of
'(signature[omittedSig] != "missing")' will equal sum(omittedSig),
i.e. the length will be in {0,1,...,length(omittedSig)}.
The R 3.6.0 version with '&&' is not correct because '&&' requires
length(omittedSig) == 1L and sum(omittedSig) == 1L, which is unlikely
to be the original intention.
The patched version with '&' is most likely not correct either because
'LHS & RHS' assume length(LHS) == length(RHS), unless it relies on the
auto-expansion of either vector. So, for the '&' version to be
correct, it basically requires that length(omittedSig) = length(LHS) =
length(RHS) = sum(omittedSig), which also sounds unlikely to be the
original intention.
Disclaimer: Please note that I have not at all studied the rest of the
function, so the above is just based on that single line plus
debugging that 'omittedSig' is a logical vector.
Martin, I don't have the time to dive into this further. Though I did
try to see if it happened when one of oligo's dependencies were
loaded, but that was not the case. It kicks in when oligo is loaded.
FYI, I can also replicate your non-replicatation on another R 3.6.0
version. I'll see if I can narrow down what's different, e.g.
comparing sessionInfo():s, etc. However, I want to reply with
findings above asap due to the R 3.6.1 release and you might roll back
the patch (since it might introduce other bugs as Peter mentioned).
/Henrik
On Mon, Jun 24, 2019 at 3:05 AM Martin Maechler
<maechler at stat.math.ethz.ch> wrote:
Henrik Bengtsson via R-core
on Sun, 23 Jun 2019 11:29:58 -0700 writes:
Thank you.
To correct myself, I can indeed reproduce this with R --vanilla too.
A reproducible example is:
$ R --vanilla
R version 3.6.0 Patched (2019-05-31 r76629) -- "Planting of a Tree"
...
Sys.setenv("_R_CHECK_LENGTH_1_LOGIC2_" = "true")
loadNamespace("oligo")
Error in omittedSig && (signature[omittedSig] != "missing") :
'length(x) = 4 > 1' in coercion to 'logical(1)'
Error: unable to load R code in package ?oligo?
Thank you Henrik, for the report, etc, but
hmm... after loading the oligo package, almost 40 (non
base+Recommended) packages have been loaded as well, which hence
need to have been installed before, too ..
which is not quite a "vanilla repr.ex." in my view
Worse, I cannot reproduce :
Sys.setenv("_R_CHECK_LENGTH_1_LOGIC2_" = "true")
Sys.getenv("_R_CHECK_LENGTH_1_LOGIC2_")
debugonce(conformMethod)
loadNamespace("oligo")
<environment: namespace:oligo>
Warning messages:
1: multiple methods tables found for ?rowSums?
2: multiple methods tables found for ?colSums?
3: multiple methods tables found for ?rowMeans?
4: multiple methods tables found for ?colMeans?
R Under development (unstable) (2019-06-20 r76729)
(similarly with other versions of R >= 3.6.0).
So, even though R core has fixed this now in the sources, it
would be nice to have an "as simple as possible" repr.ex. for this.
Martin
On Sun, Jun 23, 2019 at 1:54 AM peter dalgaard <pdalgd at gmail.com> wrote:
This looks obvious enough, so I just committed your fix to R-devel and R-patched.
I'm at the wrong machine for thorough testing, but at least it seems to build OK. However, I sense some risk that this could uncover sleeping bugs elsewhere, so watch out.
-pd
On 22 Jun 2019, at 18:49 , Henrik Bengtsson <henrik.bengtsson at gmail.com> wrote:
DISCLAIMER: I can not get this error with R --vanilla, so it only
occurs when some other package is also loaded. I don't have time to
find to narrow that down for a reproducible example, but I believe the
following error in R 3.6.0:
Sys.setenv("_R_CHECK_LENGTH_1_LOGIC2_" = "true")
library(oligo)
Error in omittedSig && (signature[omittedSig] != "missing") :
'length(x) = 4 > 1' in coercion to 'logical(1)'
Error: unable to load R code in package 'oligo'
is because of a '_R_CHECK_LENGTH_1_LOGIC2_=true' mistake in the
'methods' package. Here's the patch:
$ svn diff src/library/methods/R/RMethodUtils.R &
[1] 1062
Index: src/library/methods/R/RMethodUtils.R
===================================================================
--- src/library/methods/R/RMethodUtils.R (revision 76731)
+++ src/library/methods/R/RMethodUtils.R (working copy)
@@ -343,7 +343,7 @@
call. = TRUE, domain = NA)
}
else if(!all(signature[omittedSig] == "missing")) {
- omittedSig <- omittedSig && (signature[omittedSig] != "missing")
+ omittedSig <- omittedSig & (signature[omittedSig] != "missing")
.message("Note: ", .renderSignature(f, sig0),
gettextf("expanding the signature to include omitted
arguments in definition: %s",
paste(sigNames[omittedSig], "=
\"missing\"",collapse = ", ")))
[1]+ Done svn diff src/library/methods/R/RMethodUtils.R
Maybe still in time for R 3.6.1?
/Henrik