AndreaRMICL commented 8 years ago

Repository: https://github.com/AndreaRMICL/MetaboSignal
[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

I am familiar with the essential aspects of Bioconductor software management, including:

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Subversion (optionally via GitHub).

For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.

bioc-issue-bot commented 8 years ago

Hi @AndreaRMICL

Thanks for submitting your package. We are taking a quick look at it and you will hear back from us soon.

The DESCRIPTION file for this package is:

Package: MetaboSignal
Type: Package
Title: MetaboSignal: a network-based approach to overlay and explore metabolic and signalling KEGG pathways
Version: 0.99.0
Date: 2016-06-27
Author: Andrea Rodriguez-Martinez <andrea.rodriguez-martinez13@imperial.ac.uk>, Rafael Ayala <r.ayala14@imperial.ac.uk>, Joram M. Posma <j.posma11@imperial.ac.uk>, Ana L. Neves <ana.luisa.neves14@imperial.ac.uk>, Marc-Emmanuel Dumas <m.dumas@imperial.ac.uk>
Maintainer: Andrea Rodriguez-Martinez <andrea.rodriguez-martinez13@imperial.ac.uk>, Rafael Ayala <r.ayala14@imperial.ac.uk>
Description: MetaboSignal is an R package that allows merging, analyzing and customizing metabolic and signaling KEGG pathways. It is a network-based approach designed to explore the topological relationship between genes (signaling- or enzymatic-genes) and metabolites, representing a powerful tool to investigate the genetic landscape and regulatory networks of metabolic phenotypes.
License: GPL-3
Depends: R(>= 3.2)
Suggests: RUnit, BiocGenerics, knitr
VignetteBuilder: knitr
Imports: KEGGgraph, mygene, hpar, org.Hs.eg.db, igraph, RCurl, biomaRt, AnnotationDbi
biocViews: GraphAndNetwork, GeneSignaling, GeneTarget, Network, Pathways, KEGG, Reactome, Software
NeedsCompilation: no
LazyData: true
Encoding: UTF-8

bioc-issue-bot commented 8 years ago

Your package has been approved for building. Your package is now submitted to our queue.

IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.

mtmorgan commented 8 years ago

I took a preliminary look at your package while moderating it. I'm concerned that the 'copy-and-append' pattern (where a vector is created by growing it, x=c(); for (...) x = c(x, new_value), or using rbind() and a data.frame), e.g., here but at a very large number of places in your code, is known to be very inefficient when data are reasonably sized, as explained here (point 2). In at least some places, it seemed that the results could be computed as a single function call without iteration. In other places it would be better to use lapply() and then combine the results in one call, e.g., unlist() or do.call("rbind", ...).

I'll provide more comprehensive comments at a later date, once the package has built successfully, but it would be great if you could address the above point before a more complete review.

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160721142519.html

AndreaRMICL commented 8 years ago

Hi Martin,

Thanks very much for your fast reply. I will try to address your comments as soon as possible.

Based on the message I just received from Bioconductor, the package builds OK in Mac, Linux and Windows. However, I got a skipped message for build in in Linux and in postprocessing in Windows. I still will work on it to understand why this happens.

Thanks very much,

Andrea

From: Martin Morgan notifications@github.com Sent: 21 July 2016 19:02:12 To: Bioconductor/Contributions Cc: Rodriguez Martinez, Andrea; Mention Subject: Re: [Bioconductor/Contributions] MetaboSignal (#68)

I took a preliminary look at your package while moderating it. I'm concerned that the 'copy-and-append' pattern (where a vector is created by growing it, x=c(); for (...) x = c(x, new_value), or using rbind() and a data.frame), e.g., herehttps://github.com/AndreaRMICL/MetaboSignal/blob/master/R/MetaboSignal_matrix.R#L461 but at a very large number of places in your code, is known to be very inefficient when data are reasonably sized, as explained here (point 2)http://bioconductor.org/developers/how-to/efficient-code/#common-solutions. In at least some places, it seemed that the results could be computed as a single function call without iteration. In other places it would be better to use lapply() and then combine the results in one call, e.g., unlist() or do.call("rbind", ...).

I'll provide more comprehensive comments at a later date, once the package has built successfully, but it would be great if you could address the above point before a more complete review.

You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/68#issuecomment-234334212, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ATh5ZBG72Ma60t7INiLWe4WKTjZpVduSks5qX7QkgaJpZM4JR7U6.

mtmorgan commented 8 years ago

It is failing on the BiocCheck part of the check, specifically because

* Checking for bioc-devel mailing list subscription...
    * REQUIRED: Maintainer should subscribe to the bioc-devel mailing
      list. Subscribe here:
      https://stat.ethz.ch/mailman/listinfo/bioc-devel

Make sure you subscribe with the same address as the maintainer address in your package.

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

9557eae v 0.99.1

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160722060833.html

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

c864cef v 0.99.2

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160722073320.html

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

8f967c3 v0.99.3

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160725122304.html

AndreaRMICL commented 8 years ago

Hi Martin,

I have been trying to address your comment regarding the 'copy-and-append' pattern. Based on your suggestion, I have tried to replace this pattern by mapply, lapply and sapply in several places my code. I hope it looks better now, otherwise I can keep on working on it.

Thanks very much in advance, Andrea

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

bcc9a66 v 0.99.4

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160810182519.html

mtmorgan commented 8 years ago

@AndreaRMICL @lshep the Mac 'post processing' error can be ignored in the previous post; it appears to be a problem with the build system.

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

af910aa v 0.99.5

AndreaRMICL commented 8 years ago

Hi Martin, I was a bit worried, so before reading your message I submitted another push. Sorry about this. Thanks, Andrea

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160810191909.html

bioc-issue-bot commented 8 years ago

We only start builds when the Version field in the DESCRIPTION file is incremented. For example, by changing

Version: 0.99.0

to

Version 0.99.1

If you did not intend to start a build, you don't need to do anything. If you did want to start a build, increment the Version: field and try again.

bioc-issue-bot commented 8 years ago

We only start builds when the Version field in the DESCRIPTION file is incremented. For example, by changing

Version: 0.99.0

to

Version 0.99.1

If you did not intend to start a build, you don't need to do anything. If you did want to start a build, increment the Version: field and try again.

bioc-issue-bot commented 8 years ago

We only start builds when the Version field in the DESCRIPTION file is incremented. For example, by changing

Version: 0.99.0

to

Version 0.99.1

If you did not intend to start a build, you don't need to do anything. If you did want to start a build, increment the Version: field and try again.

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

d621a0b v 0.99.6 58d3b4a v 0.99.6

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

2210117 v 0.99.7

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160818082231.html

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160818082304.html

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

d4a3bd1 v 0.99.8

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "

Error response

Error code 414.

Message: Request-URI Too Long.

Error code explanation: 414 = URI is too long.. ". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself. Please see the following build report for more details: http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160818084438.html

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

9a16a26 v 0.99.9

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160818085844.html

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

38e35c6 v 0.99.10

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160818095156.html

AndreaRMICL commented 8 years ago

Hi Martin,

We added a README file to the package. However, when we pushed the package again we got a warning message from Linux, which seems to be :"checking a package with encoding 'UTF-8' in an ASCII locale". We did not get this message before, so we thought it could be because of some text in the Rmd files of the vignette. So we resaved the vignette and push the package again and we still get this warning....

Thanks in advance, Andrea

mtmorgan commented 8 years ago

Hi,

The UTF-8 / ASCII issue seems to be a change in the build environment of zin1; please ignore it.

Please also address the NOTE to add

 importFrom("graphics", "hist")
  importFrom("stats", "na.omit")
  importFrom("utils", "capture.output", "write.table")

to the NAMESPACE file.

I'll provide a formal review after your next commit.

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

288ba77 v 0.99.11

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160819054555.html

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

653e133 v 0.99.12

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160819060436.html

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160819153025.html

AndreaRMICL commented 8 years ago

Hi Martin,

Thanks for your reply. Just now we got for windows: Error in loadNamespace(j <- i[[1L]], c(lib.loc, .libPaths()), versionCheck = vI[[j]]) : there is no package called 'graph'

I am not sure what the problem is..

Thanks very much in advance, Andrea

mtmorgan commented 8 years ago

Yep, its from trying to fix another problem with the build system; ignore this warning, please.

AndreaRMICL commented 8 years ago

Ok! Thanks very much.

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160822074502.html

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

Congratulations! The package built without errors or warnings on all platforms.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160822075559.html

mtmorgan commented 8 years ago

Thanks for your package and dilegent pursuit of an error free build report!

DESCRIPTION, NAMESPACE, NEWS

Great!

vignettes

Good; a completely optional suggestion is to use BiocStyle to format the vignette. Benefits would include a look consistent with other Bioconductor packages (including, e.g., a format appropriate for printing) and a dis-incentive to include html in the markdown!
In the YAML at the top of the vignette, the text ' %\VignetteIndexEntry{...' should be on new lines, as
```
vignette: |
%\VignetteEngine{knitr::rmarkdown}
%\VignetteIndexEntry{MetaboSignal}
\usepackage[utf8]{inputenc}
```
The text for "VignetteIndexEntry" should be the title of the the vignette. The relevance can be seen by building and installing the package, and then looking at vignette(package="MetaboSignal"), where the text of VignetteIndexEntry is shown (this also shows up in the HTML help page for the package).
```
R CMD build MetaboSignal
R CMD INSTALL MetaboSignal_0.99.12.tar.gz 
R -e "vignette(package='MetaboSignal')"
```

tests

nice to see unit tests, thanks!

R. The following are meant to be examples of areas for improvement, rather than an exhaustive enumeration.

1 changes the global options(). Usually it's better to write code that does not rely on particular values of options(). If absolutely necessary, the best practice is to ensure that the options are reset to their original value whether or not the function succeeds. This is typically done by
```
oopt = options(warn=-1)
on.exit(options(oopt), add=TRUE)
```
The pattern of setting a result list or vector to length zero, then 'copy-and-appending' in a loop (e.g., for entrezgene starting at 2 is very inefficient; it is better to 'pre-allocate and fill' or use e.g., lapply() to manage the result vector; see item 2 of 3. A similar concern involves rbind() / cbind() and implicit vector extension (x[i] = 1, where i > length(x)) elsewhere in your code.
using 4 as an example, it's better to write code to avoid errors rather than relying on try() / tryCatch() to recover from errors. Is the relevant test that generates an error
```
response <- query(q="fdf", size=1, species=organism_name)
if (NROW(response$hits) != 0L) {
    ...
```
The KEGG query at 5 is one gene at a time. The API supports multiple queries, using + as a separator, and this will be much more efficient.
```
readLines("http://rest.kegg.jp/conv/genes/ncbi-geneid:293949+ncbi-geneid:293949")
```
Consider also using the KEGGrest package as a more robust interface, rather than relying on direct readLine() access
Your code seems to do a good job of validating that the user has provided reasonable input. A common pattern is to put this validation at the very top of the function, so no unnecessary computation occurs. For instance 6 might be moved to be immediately after line 12.

Watch for opportunities to vectorize, typically in for loops such as 7, which might be replaced by something along the lines of

idx = grepl("_", MetaboSignal_table)
nodes = MetaboSignal_table[idx]
MetaboSignal_table[idx] = substr(nodes, 11, nchar(nodes))

man

The help pages seem to be well done, thanks!

bioc-issue-bot commented 8 years ago

Received a valid push; starting a build. Commits are:

d5002c9 v 0.99.13

bioc-issue-bot commented 8 years ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the following build report for more details:

http://bioconductor.org/spb_reports/MetaboSignal_buildreport_20160905111553.html

AndreaRMICL commented 8 years ago

Hi Martin,

Again thanks very much for your feedback. As suggested, we have modified the vignette, which looks much better now. Thanks! Regarding the R section, we have tried to modify the code taking into account your recommendations. Some specific comments below:

-When possible, we have used the KEGGREST package instead of "readLines()". Regarding "multiple-query", after doing some tests, we came to the conclusion that the maximum number of queries for "keegConv()" is 100. However, I think this is not specified in the reference manual, so I hope the assumption we made is correct....

-Regarding "try()", we have replaced it in the example you mentioned and in some other places. However, in other cases, we think it'd be safer to leave "try()" if possible. See some examples below: --"getReactions()" gives an error if the pathway does not have an appropriate format (e.g. if the pathway is not actually a metabolic-pathway). -- "keggList()" gives a very non-specific error when the "organism_code" is not valid. We think this error might be difficult to interpret and hence fix by the user. --"MS_ChangeNames()" depends of "http://rest.kegg.jp/get/" . When the "query" is not valid, and error is generated and therefore the function stops. However, we want the function to keep on working with the remaining queries. -- select(org.Hs.eg.db, keys = symbols, columns = c("ENSEMBL"), keytype = "SYMBOL"), generates an error when none of the keys (in this case symbols) can be linked to ENSEMBL ids.

We have done a number of tests, and we think the package is now working like before. We hope the R code looks better now.

Thanks very much in advance,

Best wishes,

Andrea

Bioconductor / Contributions

MetaboSignal #68

Error response