Suharto Anggono Suharto Anggono
2019-Jul-04 15:13 UTC
[Rd] methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error
In 'conformMethod', there is another instance of omittedSig & <something about signature[omittedSig]> . It just affects error message. Original: ? ? if(any(is.na(match(signature[omittedSig], c("ANY", "missing"))))) { ? ? ? ? bad <- omittedSig & is.na(match(signature[omittedSig], c("ANY", "missing"))) After r76756: ? ? if(any(iiN <- is.na(match(signature[omittedSig], c("ANY", "missing"))))) { ? ? ? ? bad <- omittedSig & iiN ---------------------------------------------->>>>> Martin Maechler >>>>>? ? on Sat, 29 Jun 2019 12:05:49 +0200 writes:>>>>> Martin Maechler >>>>>? ? on Sat, 29 Jun 2019 10:33:10 +0200 writes:>>>>> peter dalgaard >>>>>? ? on Fri, 28 Jun 2019 16:20:03 +0200 writes:? ? >>> > On 28 Jun 2019, at 16:03 , Martin Maechler <maechler using stat.math.ethz.ch> wrote: ? ? >>> > ? ? >>> >>>>>> Henrik Bengtsson ? ? >>> >>>>>>? ? on Thu, 27 Jun 2019 16:00:39 -0700 writes: ? ? >>> > ? ? >>> >> Using: ? ? >>> >> ? ? >>> >> untrace(methods::conformMethod) ? ? >>> >> at <- c(12,4,3,2) ? ? >>> >> str(body(methods::conformMethod)[[at]]) ? ? >>> >> ## language omittedSig <- omittedSig && (signature[omittedSig] != "missing") ? ? >>> >> cc <- 0L ? ? >>> >> trace(methods::conformMethod, tracer = quote({ ? ? >>> >>? cc <<- cc + 1L ? ? >>> >>? print(cc) ? ? >>> >>? if (cc == 31) {? ## manually identified ? ? >>> >>? ? untrace(methods::conformMethod) ? ? >>> >>? ? trace(methods::conformMethod, at = list(at), tracer = quote({ ? ? >>> >>? ? ? str(list(signature = signature, mnames = mnames, fnames = fnames)) ? ? >>> >>? ? ? print(ls()) ? ? >>> >>? ? ? try(str(list(omittedSig = omittedSig, signature = signature))) ? ? >>> >>? ? })) ? ? >>> >>? } ? ? >>> >> })) ? ? >>> >> loadNamespace("oligo") ? ? >>> >> ? ? >>> >> gives: ? ? >>> >> ? ? >>> >> Untracing function "conformMethod" in package "methods" ? ? >>> >> Tracing function "conformMethod" in package "methods" ? ? >>> >> Tracing conformMethod(signature, mnames, fnames, f, fdef, definition) ? ? >>> >> step 12,4,3,2 ? ? >>> >> List of 3 ? ? >>> >> $ signature: Named chr [1:4] "TilingFeatureSet" "ANY" "ANY" "array" ? ? >>> >>? ..- attr(*, "names")= chr [1:4] "object" "subset" "target" "value" ? ? >>> >>? ..- attr(*, "package")= chr [1:4] "oligoClasses" "methods" "methods" "methods" ? ? >>> >> $ mnames? : chr [1:2] "object" "value" ? ? >>> >> $ fnames? : chr [1:4] "object" "subset" "target" "value" ? ? >>> >> [1] "f"? ? ? ? ? "fdef"? ? ? "fnames"? ? "fsig"? ? ? "imf" ? ? >>> >> [6] "method"? ? "mnames"? ? "omitted"? ? "omittedSig" "sig0" ? ? >>> >> [11] "sigNames"? "signature" ? ? >>> >> List of 2 ? ? >>> >> $ omittedSig: logi [1:4] FALSE TRUE TRUE FALSE ? ? >>> >> $ signature : Named chr [1:4] "TilingFeatureSet" "ANY" "ANY" "array" ? ? >>> >>? ..- attr(*, "names")= chr [1:4] "object" "subset" "target" "value" ? ? >>> >>? ..- attr(*, "package")= chr [1:4] "oligoClasses" "methods" "methods" "methods" ? ? >>> >> 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, nice piece of using trace() .. and the above ? ? >>> > is useful for solving the issue --? I can work with that. ? ? >>> > ? ? >>> > I'm? already pretty sure the wrong code starts with ? ? >>> > ? ? >>> >? ? omittedSig <- sigNames %in% fnames[omitted] # .... ? ? >> my? "pretty sure"? statement above has proven to be wrong .. ? ? >>> > ------------- ? ? >>> > ? ? >>> ? ? >>> I think the intention must have been that the two "ANY" signatures should change to "missing". ? ? >> Definitely. ? ? >>> However, with the current logic that will not happen, because ? ? >>> ? ? >>> > c(F,T,T,F) &&? c(T,T) ? ? >>> [1] FALSE ? ? >>> ? ? >>> Henrik's non-fix would have resulted in ? ? >>> ? ? >>> > c(F,T,T,F) &? c(T,T) ? ? >>> [1] FALSE? TRUE? TRUE FALSE ? ? >>> ? ? >>> which is actually right, but only coincidentally due to recycling of c(T,T). Had it been c(F,T) then it would have been expanded to c(F,T,F,T) which would be the opposite of what was wanted. ? ? >>> ? ? >>> Barring NA issues, I still think ? ? >>> ? ? >>> omittedSig[omittedSig] <- (signature[omittedSig] != "missing") ? ? >>> ? ? >>> should do the trick. ? ? >> yes, (most probably).? I've found a version of that which should ? ? >> be even easier to "read and understand", in? svn commit 76753 : ? ? >> svn diff -c 76753 src/library/methods/R/RMethodUtils.R ? ? >> --- src/library/methods/R/RMethodUtils.R??? (Revision 76752) ? ? >> +++ src/library/methods/R/RMethodUtils.R??? (Revision 76753) ? ? >> @@ -342,8 +342,7 @@ ? ? >> gettextf("formal arguments (%s) omitted in the method definition cannot be in the signature", bad2), ? ? >> call. = TRUE, domain = NA) ? ? >> } ? ? >> -? ? else if(!all(signature[omittedSig] == "missing")) { ? ? >> -? ? ? ? omittedSig <- omittedSig && (signature[omittedSig] != "missing") ? ? >> +? ? else if(any(omittedSig <- omittedSig & signature != "missing")) { ? ? >> BTW:? I've marked this --- and the? runmed() seg.fault + na.action ? ? >> change ---? as something to be added to R 3.6.1 patched,? as I ? ? >> deemed I should obey the "code freeze" rule in both cases. ? ? >> Martin ? ? > Hmm... I think we got a 'Neverending Story' here -- because it ? ? > seems, both Peter and I were wrong in thinking that it's a good ? ? > idea to change "missing" to "ANY" here ... ? ? > ((or if that *was* correct, that needs to entail more changes ? ? > happening during setMethod(.) {conformMethod() is only called in ? ? > one place in our code base, namely from setMethod()} : as a matter of fact, I've been brave for now, left the change to conformMethod()? and started to fix the constructed .local() calls which are created in? conformMethod()'s sibling, which is rematchDefinition(). It seems that this builds e.g. Matrix {with its gazillion setMethod()s} and that continues to run its own tests.? OTOH, Matrix may not trigger the situations that are dealt with here at all, as the signature() are rarely longer than three, and at some point in time I had made long passes through the package in order to "minimize" the .local() calls. --> svn commit 76756? (in addition to 76753, mentioned earlier) ? ? now has rematchDefinition() changes I would be positively surprised if (but can imagine that) this had no affect on CRAN / Bioconductor packages. Still, these two changes seem to achieve what both the comments and the documentation of? conformMethod() and rematchDefinition() suggest should happen. Of course, I'd really be happy if people with S4 packages would check them with an R-devel version with svn rev >=? 76756 and report problems they see. I do imagine effects, and would expect that bugs in current code become visible where they had not done so previously. Martin
Martin Maechler
2019-Jul-06 13:21 UTC
[Rd] methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error
>>>>> Suharto Anggono Suharto Anggono via R-devel >>>>> on Thu, 4 Jul 2019 15:13:55 +0000 writes:> In 'conformMethod', there is another instance of > omittedSig & <something about signature[omittedSig]> . > It just affects error message. > Original: > ? ? if(any(is.na(match(signature[omittedSig], c("ANY", "missing"))))) { > ? ? ? ? bad <- omittedSig & is.na(match(signature[omittedSig], c("ANY", "missing"))) > After r76756: > ? ? if(any(iiN <- is.na(match(signature[omittedSig], c("ANY", "missing"))))) { > ? ? ? ? bad <- omittedSig & iiN Yes, thank you, that's same "internal logic" error as later. One difference is that I think the above case cannot be triggered by setMethod() .. one needs an explicit "artificial" conformMethod() call. Still, of course, it's wrong and should be corrected. I've started to test changes .. maye not get to commit (with enough confidence) before taking the train to useR! 2019 @ Toulouse (France). Martin
Possibly Parallel Threads
- methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true
- methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error
- methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error
- methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error
- methods package: A _R_CHECK_LENGTH_1_LOGIC2_=true error