search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

DMCFB #1174

Closed shokoohi closed 5 years ago

shokoohi commented 5 years ago

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

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

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

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

bioc-issue-bot commented 5 years ago

Hi @shokoohi

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: DMCFB
Type: Package
Title: Differentially Methylated Cytosines via a Bayesian Functional Approach
Version: 0.99.0
Author: Farhad Shokoohi
Maintainer: Farhad Shokoohi <shokoohi@icloud.com>
Description: A pipeline for identifying differentially methylated cytosines
    using a Bayesian functional regression model in bisulfite sequencing data.
Depends: R (>= 3.6.0),
    SummarizedExperiment,
    methods,
    S4Vectors,
    BiocParallel,
    GenomicRanges,
    IRanges
Imports: utils,
    stats,
    speedglm,
    MASS,
    data.table,
    splines,
    arm,
    grDevices,
    rtracklayer,
    benchmarkme,
    tibble,
    matrixStats,
    fastDummies,
    graphics
Suggests: testthat, 
    knitr
VignetteBuilder:
    knitr
biocViews: DifferentialMethylation, Sequencing, Coverage, Bayesian, Regression
License: GPL-3
Encoding: UTF-8
LazyData: true
BugReports: https://github.com/shokoohi/DMCFB/issues
RoxygenNote: 6.1.1
bioc-issue-bot commented 5 years ago

A reviewer has been assigned to your package. Learn what to expect during the review process.

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 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

0fc1f89 v02 0c2d7e0 Merge branch 'master' of https://github.com/shokoo...

shokoohi commented 5 years ago

I just did the version bump. Why the error appears?

bioc-issue-bot commented 5 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, 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 build report for more details.

shokoohi commented 5 years ago

I do not understand why the package gives error under windows. I actually do not know how to fix it. The following error does not appear on my Mac.

Error: processing vignette 'DMCFB.Rmd' failed with diagnostics:

bioc-issue-bot commented 5 years ago

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

034d9f8 v03

bioc-issue-bot commented 5 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 build report for more details.

mtmorgan commented 5 years ago

3 July, 2019

DESCRIPTION

NAMESPACE

NEWS

vignettes

R code

man pages

Unit tests

bioc-issue-bot commented 5 years ago

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

993f33b v04

bioc-issue-bot commented 5 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, 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 build report for more details.

shokoohi commented 5 years ago

Hello, I do not understand why there is ERROR here. On my Mac it does not give any error.

bioc-issue-bot commented 5 years ago

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

6d6dd00 v05

bioc-issue-bot commented 5 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 build report for more details.

shokoohi commented 5 years ago

Hello, I do not have a windows machine to see why this error occurs.

bioc-issue-bot commented 5 years ago

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

28a9450 v06

bioc-issue-bot commented 5 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 build report for more details.

shokoohi commented 5 years ago

@mtmorgan Hello. I cannot fix this problem. Since I have no windows I do not know why the problem occurs. Any thought on this?

mtmorgan commented 5 years ago

I would guess it is in the code from the package that you import that tries to determine memory use. Address the other issues and I will try to look into this on the next iteration of the review.

shokoohi commented 5 years ago

@mtmorgan All other issues are addressed except two:

1) AllGenerics.R: 204 ...

I did not understand what you meant by this.

2) findDMCFB-method.R:234 ...

I want to keep it as it is. This is because when jobs are sent to CPU Cores they produce too much warnings that I do not want them to appear in the final report.

mtmorgan commented 5 years ago

  1. You've implemented writeBED, but there is already rtracklayer::export()

  2. Your implementation is broken because it forces the user option to be, e.g., show.error.message = TRUE after calling your function call, even though they had explicitly set it, for their own reasons, to show.error.message = FALSE. But generally there are alternative ways such as suppressWarnings() and withCallingHandlers() to programmatically manage these circumstances. If you provide a SIMPLE example with a small part of your code (see also the comment about re-factoring your code for easy unit testing) I will attempt to provide further guidance.

mtmorgan commented 5 years ago

Will you address the remaining issues?

shokoohi commented 5 years ago

Hello,

I am moving to UNLV in few days and I will be very busy. As soon as I settle down at UNLV I will start working on the issues.

Thank you,

mtmorgan commented 5 years ago

OK I'll mark this as 'inactive'. When you're settled in and ready to continue the review process add a comment here and I'll re-open the issue.

bioc-issue-bot commented 5 years ago

This issue is being closed because there has been no progress for an extended period of time. You may reopen the issue when you have the time to actively participate in the review / submission process. Please also keep in mind that a package accepted to Bioconductor requires a commitment on your part to ongoing maintenance.

Thank you for interest in Bioconductor.

shokoohi commented 5 years ago

Hello, I have started to work on the package. The new version is ready. I thought I should create a new issue. But it gave me error. What should I do?

mtmorgan commented 5 years ago

I have re-opened the issue. Increment the version and the package should build.

bioc-issue-bot commented 5 years ago

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

62b7bea Ver07

bioc-issue-bot commented 5 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 build report for more details.

shokoohi commented 5 years ago

I do not understand the errors. Everything is working on my computer.

lshep commented 5 years ago

Linux and Mac ERRORs appear to be on our end. I am manually rerunning the issue to see if they resolve and you should hopefully have a new build report soon. If the ERRORs on those platforms continue I will investigate these further.

As for the windows ERROR that is occurring, this is a real ERROR. I did some investigation and the problem is in the internal function .MCMCFB that gets called by findDMCFB. It appears at some point you use get_ram() to determine the maxram variable but get_ram() is a platform specific function (as described in its help page) that results in a NA. The help page suggests for windows to use memory.size().

For reference this is the code I used to determine all of this (understanding that this would probably only be useful if you had access to a windows machine) . I used R CMD Stangle vignettes/DMCFB.Rmd to get a DMCFB.R file that could be sourced. 

> source("DBCFB.R", echo=TRUE)

> OBJ2 = findDMCFB(object = OBJ1, bwa = 30, bwb = 30, nBurn = 10, nMC = 10,
+   nThin = 1, alpha = 0.05, pSize = 500, sfiles = FALSE)
------------------------------------------------------------
Running Bayesian functional regression model ...
The priors's SD = 0.3027, estimated from data ...
Error in if (maxram/multicoreWorkers() > 5) { :
  missing value where TRUE/FALSE needed
>
>
> traceback()
7: .MCMCFB(object, bwa, bwb, nBurn, nMC, nThin, sdv, nCores, pSize,
       sfiles)
6: findDMCFB(object = OBJ1, bwa = 30, bwb = 30, nBurn = 10, nMC = 10,
       nThin = 1, alpha = 0.05, pSize = 500, sfiles = FALSE)
5: findDMCFB(object = OBJ1, bwa = 30, bwb = 30, nBurn = 10, nMC = 10,
       nThin = 1, alpha = 0.05, pSize = 500, sfiles = FALSE) at DMCFB.R#76
4: eval(ei, envir)
3: eval(ei, envir)
2: withVisible(eval(ei, envir))
1: source("DMCFB.R", echo = TRUE)

Then I did a debug(DMCFB:::.MCMCFB) and stepped through the code when running source("DBCFB.R", echo=TRUE) again. 

Browse[2]>
debug: maxram <- (get_ram())[[1]]/1e+09
Browse[2]> maxram
[1] NA
 Browse[2]> get_ram()
NA B

Which led me to the help page for that function.

Cheers!

bioc-issue-bot commented 5 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

fb2f90e Ver08

bioc-issue-bot commented 5 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, 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 build report for more details.

bioc-issue-bot commented 5 years ago

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

d7e438e Ver09

bioc-issue-bot commented 5 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 build report for more details.

shokoohi commented 5 years ago

What's next? The warnings are not related to my package. I don't know how to fix them.

mtmorgan commented 5 years ago

This warning

  Warning: replacing previous import 'IRanges::windows' by 'grDevices::windows' when loading 'DMCFB'

can be addressed by selectively importing from packages -- importFrom rather than import in the NAMESPACE, avoiding @import grDevices and / or @import IRanges tags; base R support import(..., except=) https://cran.r-project.org/doc/manuals/r-release/R-exts.html#Specifying-imports-and-exports but I don't know the @import syntax for that.

mtmorgan commented 5 years ago

Once this is addressed, the next step is to wait for the reviewer (me) to review the package.

shokoohi commented 5 years ago

I couldn't fix the problem. IRanges and grDevices must be imported in my package. I use RStudio and the "NAMESPACE" is generated automatically by "roxygen2" and I cannot change it manually.

mtmorgan commented 5 years ago

Only import the functions that you need from each package, rather than importing the entire package. To figure out what needed to be imported, I edited the NAMESPACE file to comment out the lines import(IRanges) and import(grDevices) and then ran R CMD build and R CMD check. The latter complains:

* checking R code for possible problems ... NOTE
.MCMCFB : .fitpar: no visible binding for global variable ‘median’
.readBismark: no visible global function definition for ‘IRanges’
readBismark,character-DataFrame: no visible global function definition
  for ‘IRanges’
Undefined global functions or variables:
  IRanges median
Consider adding
  importFrom("stats", "median")
to your NAMESPACE file.

It looks like you don't use grDevices at all? So I removed grDevices from the DESCRIPTION file and made the following changes to the source code

DMCFB master$ git diff R/
diff --git a/R/AllGenerics.R b/R/AllGenerics.R
index 9ea95c5..d7205d3 100644
--- a/R/AllGenerics.R
+++ b/R/AllGenerics.R
@@ -202,7 +202,7 @@ setGeneric("combine", function(obj1, obj2) standardGeneric("combine"))
 #' @author Farhad Shokoohi <shokoohi@icloud.com>
 #' @name readBismark-method
 #' @import GenomicRanges
-#' @import IRanges
+#' @importFrom IRanges IRanges
 #' @import S4Vectors
 #' @inheritParams params
 #' @return A \code{\link{BSDMC-class}} object
@@ -279,7 +279,6 @@ setGeneric("findDMCFB", function(object, bwa, bwb, nBurn, nMC, nThin, alpha,
 #' @import BiocParallel
 #' @import GenomicRanges
 #' @import S4Vectors
-#' @import grDevices
 #' @import rtracklayer
 #' @import graphics
 #' @importFrom graphics abline axis layout legend lines par points segments
diff --git a/R/findDMCFB-method.R b/R/findDMCFB-method.R
index 6946052..27af35b 100644
--- a/R/findDMCFB-method.R
+++ b/R/findDMCFB-method.R
@@ -1,4 +1,4 @@
-
+#' @importFrom stats median
 .MCMCFB <- function(object, bwa, bwb, nBurn, nMC, nThin, sdv, nCores, pSize,
     sfiles) {
     if (missing(object)) {

and ran devtools::document() to update the NAMESPACE file. Running R CMD build / check again, I was told

* checking dependencies in R code ... NOTE
Namespace in Imports field not imported from: ‘BiocStyle’
  All declared Imports should be used.
Package in Depends field not imported from: ‘GenomeInfoDb’
  These packages need to be imported from (in the NAMESPACE file)
  for when this namespace is loaded but not attached.

BiocStyle is used in building the vignette, and should be in the Suggests: field. Your package doesn't use GenomeInfoDb, so it should not be mentioned in the DESCRIPTION file.

bioc-issue-bot commented 5 years ago

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

87b7984 v10

bioc-issue-bot commented 5 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 build report for more details.

shokoohi commented 5 years ago

Would you kindly let me know what is next?

mtmorgan commented 5 years ago

Have you addressed the issues in https://github.com/Bioconductor/Contributions/issues/1174#issuecomment-507912754 ? For instance I see many uses of options() in a way that would change user settings

DMCFB/R master$ grep options *
findDMCFB-method.R:    # options(warn=-1)
findDMCFB-method.R:        options(show.error.messages = FALSE)
findDMCFB-method.R:        options(show.error.messages = TRUE)
findDMCFB-method.R:        options(warn = -1)
findDMCFB-method.R:        options(warn = 0)
...
shokoohi commented 5 years ago

Description: too brief; should read like a paper abstract highlighting new features and contrasting with alternatives

Done

Make sure NEWS file can be parsed by utils::news()

Done

Including only source (e.g., .Rmd) files, not outputs (e.g., .html, .pdf)

Done

Update the %\VignetteIndexEntry{} field to match the title of the vignette

Done

Consider using BiocStyle for constent vignette formatting

Done

Minimize use of {..., message = FALSE} is used to suppress package startup messages, include package loading in its own chunk.

Done

Format code chunks following standard programming conventions, e.g., indentation of continuation lines, vertical spacing to separate conceptually different steps.

Done

Provide more extensive text to describe use of your package, including in describing in general terms inputs and outputs of important function calls.

Done

Include the output of sessionInfo() in your vignette

Done

AllClasses.R, ...: Follow standard code formatting conventions, such as those seen when the code is viewed in R or suggested by packages such as styler or formatR

Done

AllClasses.R: the setValidity() function should return TRUE when the object is valid.

Done

AllGenerics.R:38 Consider carefully whether multiple dispatch is required in the S4 generic; multiple dispatch is difficult to reason about. Use argument validation in method implementations if only a single type of argument is expected.

Considered

AllGenerics.R: 204 Re-use functionality from 'core' package, see here

Decided to stay with my own function; will change in the next version in both of my packages "DMCHMM" and this one

findDMCFB-method.R:17 Use stop(), warning(), message() to signal error or informative messages to the user; use cat() or print() only to display an object in a print() or show() method. cat(), stop(), warning(), message() do not usually need to paste() arguments together

Done

findDMCFB-method.R:234 Do not change user settings fo global options options() ; for instance if the user had options(show.error.message = FALSE), your code would change the value to TRUE. Use tryCatch() to silently handle error messages, and withCallingHandlers() to handle warnings. Generally it seems like warnings should not be supressed if the function is being used appropriately.

Few datasets will create warning due to low coverage data. It will be messy but the analysis is correct. However, I don't know how to avoid these unnecessary warnings.

findDMCFB-method.R:532 Use matrix operations, e.g., colSums() or as defined in matrixStats, rather than apply().

Done

zzz.R:1 Carefully consider the need for a startup message; if all packages do this, then the user simply learns to ignore the message

Done

Effective unit tests often benefit from more modular code, where each module can be tested quickly and with simple inputs and outputs rather than requiring complicated inputs / outputs to comprehensive functions

Done

bioc-issue-bot commented 5 years ago

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

049d5d0 Ver11

mtmorgan commented 5 years ago

I think you pushed the version bump but not the code changes? This https://github.com/Bioconductor/Contributions/issues/1174#issuecomment-530830752 still applies...

bioc-issue-bot commented 5 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 build report for more details.