[Bioc-devel] condition tests on vectors with length greater than 1

Kevin RUE kevinrue67 at gmail.com
Fri Mar 31 19:30:12 CEST 2017


Hi Martin,

My apologies to Tomas Kalibera. I read too fast and missed the part where
you gave him credit for the report.

As I wrote in my previous email, I have started a significant clean-up of
the GOexpress package to improve both the coding standards and the user
experience. I haven't reached the overlap_GO function yet (that uses the
VennDiagram package); however, this is not a core function (I used it for
diagnostic mostly during package development), and I will most likely
drop it altogether before the next release.

Have a great weekend.

Kind regards,
Kevin


On Fri, Mar 31, 2017 at 5:00 PM, Martin Morgan <
martin.morgan at roswellpark.org> wrote:

> On 03/31/2017 08:21 AM, Kevin RUE wrote:
>
>> Hi Martin,
>>
>> Thanks a lot for systematically identifying the issue.
>>
>> Looking at the issue for GOexpress now, I realise that the issue is not
>> in GOexpress itself, but actually within the VennDiagram package that
>> GOexpress imports. You actually captured that in your report:
>>
>> "GOexpress","_VennDiagram_","GOexpress.Rcheck/GOexpress-Ex.R
>> out","VennDiagram::adjust.venn","VennDiagram/R/adjust.venn.R#42 "
>>
>> In your report, GOexpress is the only Bioconductor in this situation.
>> I can (and will) attempt to contact the maintainer of VennDiagram, but
>> it just makes me wonder if you have another suggestion to deal with this
>> kind of issue (/i.e./, imported packages not conforming to BioC
>> guidelines). Is this going to raise Warnings or Errors in BiocCheck ? If
>> so, should we ignore such packages altogether, as long as those packages
>> raise check issues?
>>
>
> Actually, the report was generated by an R-core member, Tomas Kalibera and
> the more extensive version includes all CRAN and Bioc packages. It reflects
> a general effort to address these and then make them errors, rather than
> warnings -- R CMD check would then fail.
>
> Package dependencies are interesting. They are obviously hugely important
> to reduce redundant code, and to fix bugs once in a single place. But they
> also carry with them an additional maintenance cost. If VennDiagram is
> central to your package, especially if it implements complicated
> functionality, then definitely the balance is in favor of depending on it,
> and working with the maintainer to address problems that you discover.
>
> The Bioc requirements are really about your package, especially enforcing
> adequate documentation.
>
> Martin
>
>
>> As a side note, I have actually already been going through GOexpress to
>> prepare a new major version of GOexpress (2.x.x) that will introduce S4
>> where relevant, and break down large functions into more sensibly-sized
>> steps. However, most likely that won't be ready for the next BioC release.
>>
>> Best,
>> Kevin
>>
>>
>> On Wed, Mar 29, 2017 at 10:44 PM, Martin Morgan
>> <martin.morgan at roswellpark.org <mailto:martin.morgan at roswellpark.org>>
>>
>> wrote:
>>
>>     On 03/29/2017 05:37 PM, Martin Morgan wrote:
>>
>>         Bioc developers,
>>
>>         R emits a warning when a condition test has length > 1
>>
>>         $ R --vanilla
>>
>>             if (letters == "a") TRUE
>>
>>         [1] TRUE
>>         Warning message:
>>         In if (letters == "a") TRUE :
>>           the condition has length > 1 and only the first element will
>>         be used
>>
>>         These are almost always programming errors.
>>
>>         R-3-4-branch and R-devel can be configured to report such errors,
>> as
>>         described on the help page for, e.g., `?"if"`
>>
>>         $ _R_CHECK_LENGTH_1_CONDITION_=TRUE R --vanilla
>>
>>             if (letters=="a") TRUE
>>
>>         Error in if (letters == "a") TRUE : the condition has length > 1
>>
>>         The attached file (thanks to Tomas Kalibera) summarizes
>> Bioconductor
>>         code that triggers this type of error.
>>
>>
>>     Second try on the attachment
>>
>>     r = read.csv("long-conditional-bioc.txt")
>>
>>
>>
>>         If you maintain one of these packages (appearing in either
>> column),
>>         please address the error. And of course as a developer, please
>> avoid
>>         making the error in the future!
>>
>>             r = read.csv("long-conditional-bioc.csv")
>>             r[, c("FailedPackage", "Srcref")]
>>
>>            FailedPackage                                          Srcref
>>         1     biovizBase               biovizBase/R/crunch-method.R#295
>>         2  branchpointer               branchpointer/R/makeRegions.R#41
>>         3       Cardinal                        Cardinal/R/spatial.R#57
>>         4      debrowser                       debrowser/R/heatmap.R#38
>>         5        DMRcate                        DMRcate/R/DMR.plot.R#13
>>         6      exomePeak                          exomePeak/R/RMT.R#119
>>         7          fabia                           fabia/R/fabia.R#1504
>>         8   GenomeInfoDb             GenomicFeatures/R/TxDb-class.R#377
>>         9         Glimma                           Glimma/R/hexcol.R#32
>>         10     GOexpress                 VennDiagram/R/adjust.venn.R#42
>>         11      GUIDEseq GUIDEseq/R/offTargetAnalysisOfPeakRegions.R#95
>>         12      hapFabia  hapFabia/R/methods-IBDsegmentList-class.R#110
>>         13     MassArray                   MassArray/R/convControl.R#26
>>         14    methylPipe                methylPipe/R/Allfunctions.R#635
>>         15        NOISeq               NOISeq/R/biodetection.plot.R#157
>>         16      pathview                  pathview/R/geneannot.map.R#31
>>         17      phyloseq              phyloseq/R/multtest-wrapper.R#101
>>         18         rHVDM              rHVDM/R/measurementerrorHVDM.R#23
>>         19          SEPA                  SEPA/R/truetimevisualize.R#28
>>         20      SPLINTER                 SPLINTER/R/main_splinter.R#817
>>
>>         As an example the GenomeInfoDb package (row 8) has this complete
>>         record
>>
>>         FailedPackage "GenomeInfoDb"
>>         IfPackage     "GenomicFeatures"
>>         File          "GenomeInfoDb.Rcheck/GenomeInfoDb-Ex.Rout"
>>         Function      "S4 Method seqlevels<-:GenomeInfoDb defined in
>>         namespace
>>                        GenomicFeatures with signature TxDb has this body."
>>         Srcref        "GenomicFeatures/R/TxDb-class.R#377 "
>>
>>         The problem was from
>>
>>           GenomicFeatures/R/TxDb-class.R#377
>>
>>         which has
>>
>>             mode <- GenomeInfoDb:::getSeqlevelsReplacementMode(value,
>>         x_seqlevels0)
>>             if (mode == -2L) {
>>
>>         I looked at
>>
>>             GenomeInfoDb:::getSeqlevelsReplacementMode
>>
>>         function (new_seqlevels, old_seqlevels)
>>         {
>>             ...
>>
>>         and saw that its code returns a vector with length > 1
>> intentionally
>>         under some specific circumstances. Also, all other uses of the
>>         return
>>         value of this function (in the GenomeInfoDb and GenomicFeatures
>>         package)
>>         test for identity of the return value via `identical()`, which
>>         is always
>>         a scalar. This suggests the fix
>>
>>         GenomicFeatures$ svn diff R/TxDb-class.R
>>         Index: R/TxDb-class.R
>>         ============================================================
>> =======
>>         --- R/TxDb-class.R    (revision 127829)
>>         +++ R/TxDb-class.R    (working copy)
>>         @@ -374,7 +374,7 @@
>>              ## detect the situation where the user intention is to
>>         subset the
>>         "real"
>>              ## seqlevels.
>>              mode <- GenomeInfoDb:::getSeqlevelsReplacementMode(value,
>>         x_seqlevels0)
>>         -    if (mode == -2L) {
>>         +    if (identical(mode, -2L)) {
>>                  ## "subsetting of the real seqlevels" mode
>>                  x$user_seqlevels <- value
>>                  x$user2seqlevels0 <- match(value, x_seqlevels0)
>>
>>         I did do some more digging around.
>>
>>         From the report, the failure was detected while running the
>>         example page
>>         script generated while checking GenomeInfoDb.
>>
>>           GenomeInfoDb.Rcheck/GenomeInfoDb-Ex.Rout
>>
>>         So I generated this
>>
>>           R CMD build --no-build-vignettes GenomeInfoDb
>>           R CMD check GenomeInfoDb_1.27.11.tar.gz
>>
>>         and processed it to find the error
>>
>>           _R_CHECK_LENGTH_1_CONDITION_=TRUE R -f
>>         GenomeInfoDb.Rcheck/GenomeInfoDb-Ex.R
>>
>>         It failed while running the example on the help page with name
>>         "seqlevels-wrapper"
>>
>>             ### Name: seqlevels-wrappers
>>             ### Title: Convenience wrappers to the seqlevels() getter
>>             and setter
>>             ### Aliases: seqlevels-wrappers keepSeqlevels dropSeqlevels
>>
>>         renameSeqlevels
>>
>>             ###   restoreSeqlevels standardChromosomes
>>             keepStandardChromosomes
>>             ### Keywords: methods utilities
>>
>>             ### ** Examples
>>
>>         ...
>>
>>             renameSeqlevels(txdb, sub("chr", "CH", seqlevels(txdb)))
>>
>>         Error in if (mode == -2L) { : the condition has length > 1
>>         Calls: renameSeqlevels -> seqlevels<- -> seqlevels<-
>>         Execution halted
>>
>>         I then could get a relatively simple reproducible example in R
>> with
>>
>>         $ _R_CHECK_LENGTH_1_CONDITION_=TRUE R --vanilla
>>
>>             suppressPackageStartupMessages(library(GenomeInfoDb))
>>             example("seqlevels-wrappers")
>>
>>
>>         After installing the updated GenomicFeatures package, the error
>>         did not
>>         occur. Likewise, running GenomeInfoDb-Ex.R did not generate any
>>         errors.
>>
>>         Martin
>>
>>
>>         This email message may contain legally privileged and/or
>>         confidential
>>         information.  If you are not the intended recipient(s), or the
>>         employee
>>         or agent responsible for the delivery of this message to the
>>         intended
>>         recipient(s), you are hereby notified that any disclosure,
>> copying,
>>         distribution, or use of this email message is prohibited.  If
>>         you have
>>         received this message in error, please notify the sender
>>         immediately by
>>         e-mail and delete this email message from your computer. Thank
>> you.
>>         _______________________________________________
>>         Bioc-devel at r-project.org <mailto:Bioc-devel at r-project.org>
>>         mailing list
>>         https://stat.ethz.ch/mailman/listinfo/bioc-devel
>>         <https://stat.ethz.ch/mailman/listinfo/bioc-devel>
>>
>>
>>
>>     This email message may contain legally privileged and/or
>>     confidential information.  If you are not the intended recipient(s),
>>     or the employee or agent responsible for the delivery of this
>>     message to the intended recipient(s), you are hereby notified that
>>     any disclosure, copying, distribution, or use of this email message
>>     is prohibited.  If you have received this message in error, please
>>     notify the sender immediately by e-mail and delete this email
>>     message from your computer. Thank you.
>>
>>     _______________________________________________
>>     Bioc-devel at r-project.org <mailto:Bioc-devel at r-project.org> mailing
>> list
>>     https://stat.ethz.ch/mailman/listinfo/bioc-devel
>>     <https://stat.ethz.ch/mailman/listinfo/bioc-devel>
>>
>>
>>
>
> This email message may contain legally privileged and/or confidential
> information.  If you are not the intended recipient(s), or the employee or
> agent responsible for the delivery of this message to the intended
> recipient(s), you are hereby notified that any disclosure, copying,
> distribution, or use of this email message is prohibited.  If you have
> received this message in error, please notify the sender immediately by
> e-mail and delete this email message from your computer. Thank you.
>

	[[alternative HTML version deleted]]



More information about the Bioc-devel mailing list