CEMiTool - Githubissues

csbl-usp commented 7 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

Repository: https://github.com/csbl-usp/CEMiTool

Confirm the following by editing each check box to '[x]'

[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] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[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 7 years ago

Hi @csbl-usp

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: CEMiTool
Title: Finds Gene Co-expression Modules
Version: 0.99.0
Authors@R: c(
  person("Pedro", "Russo",
          email = "pedro.russo@usp.br",
          role = c("aut")),
  person("Gustavo", "Ferreira",
          email = "gustavo.rodrigues.ferreira@usp.br",
          role = c("aut")),
  person("Helder", "Nakaya",
          email = "hnakaya@usp.br",
          role = c("aut", "cre")),
  person("Matheus", "Bürger",
          email = "burger@usp.br",
          role = c("aut")),
  person("Lucas", "Cardozo",
          email = "lucas.cardozo@usp.br",
          role = c("aut")))
Description: CEMiTool improves WGCNA gene co-expression module discovery by
    optimizing network parameter selection. This package also perfoms enrichment
    analysis with the discovered modules.
Depends:
    R (>= 3.3.1)
Imports:
    methods,
    scales,
    gRbase,
    data.table (>= 1.9.4),
    WGCNA,
    ggplot2,
    clusterProfiler,
    fgsea,
    stringr,
    knitr,
    rmarkdown,
    igraph,
    ggnetwork,
    DT,
    htmltools,
    pracma,
    intergraph,
    modes,
    diptest,
    stats,
    grDevices,
    utils
Suggests:
    testthat
Remotes: lecardozo/ggnetwork
License: GPL-3
Encoding: UTF-8
biocViews: GeneExpression, Transcriptomics, GraphAndNetwork, mRNAMicroarray,
    RNASeq, Network, NetworkEnrichment
LazyData: true
RoxygenNote: 5.0.1
VignetteBuilder: knitr

bioc-issue-bot commented 7 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.

bioc-issue-bot commented 7 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, 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/CEMiTool_buildreport_20170531132739.html

csbl-usp commented 7 years ago

Hi @mtmorgan, the error in the build is actually one that we already had previously, and it stems from a package dependency on GitHub which had some import problems. To address that, one of our collaborators did a fork and fixed the issue inside it, but it seems that the build script isn't using the forked version, but the original version from CRAN instead. So is a GitHub dependency a problem? Should we try to implement the used functions inside our own package?

mtmorgan commented 7 years ago

Dependencies must come from CRAN or Bioconductor. The best solution is to get the updated version on CRAN, have you spoken to the original package maintainer?

csbl-usp commented 7 years ago

Yes, I just sent an e-mail to the original package maintainer. However, he hasn't updated the repository (briatte/ggnetwork) since Dec 26, 2016 , in spite of several issues being reported and pull requests. I'll wait a couple of days to see if he answers, but if he doesn't, what would you suggest?

mtmorgan commented 7 years ago

It would be better to find another package than to try to re-implement yourself. But let's give the maintainer a day or so first...

bioc-issue-bot commented 7 years ago

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

c7b1843 Version bumping

bioc-issue-bot commented 7 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: "TIMEOUT". 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/CEMiTool_buildreport_20170606125040.html

llrs commented 7 years ago

Dear @csbl-usp, I see you export a read_gmt function, how is it different from the GSEABase function getGmt? You don't seem to have examples in the help pages (at least on the filter.R file) about how to use the functions. In find_modules:

Why don't you allow to make the WGCNA network construction using the bicor function of WGCNA?
How much is (usually) your beta different from the beta calculated using the pickSoftThreshold of WGCNA? In the vignette I couldn't find how does this procedure improve the network construction.

csbl-usp commented 7 years ago

Hi @llrs thanks for your input. We actually hadn't considered the bicor function, we'll include it in our package. As for the read.gmt function we opted to create our own as we thought that thus we could avoid one more package dependency for only one function. As for the differences in the beta values, we'll discuss that in the upcoming paper for the package, but briefly we address a few problematic cases in which WGCNA's beta value returns NA, and we use a more stringent approach to calculate the value.

llrs commented 7 years ago

The pickSoftThreshold function return a powerEstimate of NA only if "R^2 is below RsquaredCut for all powers". I don't see problematic cases here (either select power independently of what it recommends, or decrease the RsquaredCut) but I am looking forward the paper.

One last thing, in the vignette several slots are access via @, in general this is discouraged and a pertinent method should be provided. But I am sure that the official review will provide more insights on the package.

csbl-usp commented 7 years ago

Hi @llrs, yes we're well aware of the motivation behind the use of NA for beta values, but we've found a few cases where WGCNA doesn't work as it should; again, it will be better discussed in the paper. We're also already working on providing accessor functions for the S4 object slots, so thanks again for your input.

llrs commented 7 years ago

If WGCNA doesn't work as it could be a bug. Contact the maintainer as he usually answers quite quickly and can correct the error in the package. I would like to know the cases that create that error, because I am modifying the package in a fork adding test to the functions, and I would like to add these cases as test for the package. Let me know when the paper is published, many thanks.

csbl-usp commented 7 years ago

Dear Lluís,

We will submit the manuscript on the next few days. Meanwhile, we are planning to get the package ready for bioconductor. Will let you know! Thanks a lot.

Best,

Helder

Helder Nakaya, Ph.D. Assistant Professor

Department of Clinical and Toxicological Analyses School of Pharmaceutical Sciences University of Sao Paulo, Brazil

Computational Systems Biology Laboratory http://csbiology.com/

On Thu, Jun 15, 2017 at 4:27 AM, Lluís notifications@github.com wrote:

If WGCNA doesn't work as it could be a bug. Contact the maintainer as he usually answers quite quickly and can correct the error in the package. I would like to know the cases that create that error, because I am modifying the package in a fork https://github.com/llrs/WGCNA/ adding test to the functions, and I would like to add these cases as test for the package. Let me know when the paper is published, many thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/386#issuecomment-308651604, or mute the thread https://github.com/notifications/unsubscribe-auth/AIdI86zmVFd1AXFFr9t7eIVtPS_O9e6Zks5sENzQgaJpZM4Nr6Z5 .

bioc-issue-bot commented 7 years ago

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

9ac44e0 Version bump

bioc-issue-bot commented 7 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/CEMiTool_buildreport_20170628133206.html

bioc-issue-bot commented 7 years ago

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

ad19cb2 "Version bump" 4bbe51d Merge branch 'master' of https://github.com/csbl-u...

bioc-issue-bot commented 7 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/CEMiTool_buildreport_20170628162002.html

bioc-issue-bot commented 7 years ago

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

ba4d706 Version bump

bioc-issue-bot commented 7 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/CEMiTool_buildreport_20170628164551.html

bioc-issue-bot commented 7 years ago

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

8de7096 Version bump

bioc-issue-bot commented 7 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/CEMiTool_buildreport_20170703134433.html

csbl-usp commented 7 years ago

Hello, @mtmorgan it's been a few days since the OK label has been added to our issue. Could you please provide us an estimate of when we could expect the official review to be out?

dvantwisk commented 7 years ago

We've reviewed your package! It looks very good! We do have some critiques.

DESCRIPTION

Looks great!

NAMESPACE

L39-53: Consider explicitly stating which symbols are being imported from the various packages using importFrom() unless so many symbols are being used from a package that it becomes tedious.

R

R/cemitool.R

L60-75: Consider replacing the initialize method with a constructor. In addition to making the code more efficient, this will avoid errors that code occur if the CEMiTool class is extended in the future.
L177: Better to include numeric in signature.
L673-681: The for-loop with nested rbind.data.frame() creates an inefficient copying motif. It would be better to replace the entire for-loop with a call to rbind.data.frame() that takes an lapply() statement as an argument. Consider making this change.

R/datasets.R

requires further description

R/enrichment.R

L17: [Consider] You can reuse a function from another BioConductor package to perform the task of reading a gmt file. The GSEABASE package has this functionality with the function toGMT()

-L64: paste() here is unnecessary.

L119-123: cbind() loop is expensive as it also has the inefficient copying motif. Consider a similar change as mentioned in cemitools.R:L673.
L235: paste0() unnecessary.

R/filter.R

L63-66, 107-108: consider using matrixStats::rowVar().

R/integrate.R

L72: use rowSums().

R/modules.R

L96: Code can be vectorized:

A <- sum(vapply(2:length(fit), function(cont)
        0.5 * (fit[cont] + fit[cont-1]) * (powers[cont] - powers[cont-1]),
    numeric(1)))

tests

Tests seem sparse in general, consider adding more (e.g. add tests that verify input, test further function and methods, etc.)

man

Looks good!

vignettes

Looks good!

dvantwisk commented 6 years ago

Hello,

Is the package ready for further review? Also, we need to request that you perform a version bump.

bioc-issue-bot commented 6 years ago

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

a12481d Version bump

bioc-issue-bot commented 6 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/CEMiTool_buildreport_20170721122626.html

csbl-usp commented 6 years ago

Hi @dvantwisk, thanks for reviewing our package, it has been very helpful. I just bumped up our package version, as requested. Just a few comments on specific points of your review:

R/cemitool.R L177: Better to include numeric in signature. Could you elaborate on why you made this suggestion? It would seem that the module colors would usually come as character values.

R/enrichment.R L17: [Consider] You can reuse a function from another BioConductor package to perform the task of reading a gmt file. The GSEABASE package has this functionality with the function toGMT() We've updated our read_gmt to use the mentioned package.

Thanks again

dvantwisk commented 6 years ago

Hello,

The changes you made so far look good! We have a few elaborations and changes:

R/cemitool.R L177: You're right, instead of numeric we recommend instead putting character in the signature, which is the type of mod_colors.

R/modules.R L96: Looking back at the code, we realized our suggestion could be improved further with:

sum(0.5 * (tail(fit, -1) + head(fit, -1)) * (tail(powers, -1) - head(powers, -1)))

dvantwisk commented 6 years ago

One last thing! You have the files CEMitool.Rproj and .travis.yml in your package's root directory. It would be best to include this in .gitignore.

bioc-issue-bot commented 6 years ago

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

02926d1 Version bump

bioc-issue-bot commented 6 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/CEMiTool_buildreport_20170725150122.html

dvantwisk commented 6 years ago

Can you perform a version bump?

bioc-issue-bot commented 6 years ago

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

16950c1 Version bump

bioc-issue-bot commented 6 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/CEMiTool_buildreport_20170801103827.html

bioc-issue-bot commented 6 years ago

Your package has been accepted. It will be added to the Bioconductor svn repository and nightly builds. Additional information will be sent to the maintainer email address in the next several days.

Thank you for contributing to Bioconductor!

Bioconductor / Contributions

CEMiTool #386