search

Bioconductor / Contributions

Contribute Packages to Bioconductor
135 stars 33 forks source link

gwasurvivr #749

Closed aarizvi closed 6 years ago

aarizvi commented 6 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 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 build report for more details.

vobencha commented 6 years ago

Hi,

I've done an initial review. Let me know if you have questions.

1) Depends field in DESCRIPTION Please add 'Depends' that mentions the version of R.

2) I'd recommend reformatting the code to max 80 characters wide.

3) Saved file extensions It looks like the default names for saving files (under certain circumstances) are ".snps_removed" and ".coxph". These extensions aren't mentioned in the man pages and probably should be.

4) BiocParallel Is there a reason why you did't use BiocParallel? This sort of manual creation and management of a cluster dependent on OS type is already integrated into BiocParallel.

    ########## Cluster object ########################
    # create cluster object depending on user pref or OS type,
    # also create option to input number of cores
    if(!is.null(clusterObj)){
        cl <- clusterObj
    }else if(.Platform$OS.type == "unix") {
        cl <- makeForkCluster(getOption("gwasurvivr.cores", 2L))
    } else {
        cl <- makeCluster(getOption("gwasurvivr.cores", 2L))
    }
    on.exit(stopCluster(cl), add=TRUE)
    ################################################

5) I'd suggest renaming the vignette file to something more descriptive. Often it's named after the package or with the word "Introduction" etc.

bioc-issue-bot commented 6 years ago

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

2521aa6 version bump, reformatted code to 80 characters wi...

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.

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 6 years ago

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

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

bioc-issue-bot commented 6 years ago

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

9825876 - version bump, made roxygen man files less than 8...

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.

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.

bioc-issue-bot commented 6 years ago

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

5972864 attempt to fix some spacing issues for man file

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.

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.

bioc-issue-bot commented 6 years ago

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

2d6af6f version bump, man fixes

bioc-issue-bot commented 6 years ago

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

e674444 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.

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.

bioc-issue-bot commented 6 years ago

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

fa51f72 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.

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.

bioc-issue-bot commented 6 years ago

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

872ab6a version bump, changed R version, added biocstyle v...

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.

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.

aarizvi commented 6 years ago

Hi Valerie,

Thank you very much for your initial review.

I am going to reply to each of your comments:

1. Depends field in DESCRIPTION Please add 'Depends' that mentions the version of R.

I added the Depends section to my DESCRIPTION file. I seem to be getting a warning if I try to make it depend on any R version < R 3.5. As many people haven't still updated to R 3.5 yet, nor have some academic clusters (neither of the two that I have access to), I was wondering if it were possible to make the R version >= 3.4 instead of >= 3.5.

2. I'd recommend reformatting the code to max 80 characters wide.

I reformatted most of the code to less than 80 characters, the remaining 3% seem to mostly be either in my vignette or in the Roxygen documentation preamble. Should I still try to get this to 0%? I believe all if not nearly all of the actual code is < 80 characters.

3. Saved file extensions It looks like the default names for saving files (under certain circumstances) are ".snps_removed" and ".coxph". These extensions aren't mentioned in the man pages and probably should be.

Thank you for noticing this. I briefly added these descriptions to the VALUE/@return part of the man documentation.

4. BiocParallel Is there a reason why you did't use BiocParallel? This sort of manual creation and management of a cluster dependent on OS type is already integrated into BiocParallel. 

    ########## Cluster object ########################
    # create cluster object depending on user pref or OS type,
    # also create option to input number of cores
    if(!is.null(clusterObj)){
        cl <- clusterObj
    }else if(.Platform$OS.type == "unix") {
        cl <- makeForkCluster(getOption("gwasurvivr.cores", 2L))
    } else {
        cl <- makeCluster(getOption("gwasurvivr.cores", 2L))
    }
    on.exit(stopCluster(cl), add=TRUE)
    ################################################

We made this if statement to pass the Windows build.

We definitely know about BiocParallel and are aware of its strengths and robustness. However, when we were writing this package, we did some system.time and microbenchmark tests comparing parApply with bplapply and found that on one node across multiple cores (only tests with cores=4 to cores=8), that parApply would finish each row about 1 second quicker, and across millions of SNPs this made a difference to us. Also bplapply requires a list and returns a list, while parApply can conduct row-wise matrix iterations. (We now know of bpiterate and are interested in this for a future release).

Now, what we did not realize, is that we were not parallel computing across multiple nodes. Which of course we now realize could be advantageous and could be implemented using BiocParallel or batchtools. We would like to take advantage of this, but this would most likely not be implemented until our next release, as we are quite satisfied with how gwasurvivr is operating at this time.

I could of course reproduce these benchmark comparisons for you if you would like to see that.

5. I'd suggest renaming the vignette file to something more descriptive. Often it's named after the package or with the word "Introduction" etc.

Thanks for the suggestion -- I renamed the vignette to gwasurvivr_Introduction.Rmd

Lastly, I was wondering if you knew what was going on with this warning while doing the R CMD BiocCheck during the automated package build:

Warning in res[i] <- withCallingHandlers(if (tangle) process_tangle(group) else process_group(group),  :
  number of items to replace is not a multiple of replacement length
Warning in res[i] <- withCallingHandlers(if (tangle) process_tangle(group) else process_group(group),  :
  number of items to replace is not a multiple of replacement length`

From what I'm seeing is that this has to do with either BiocStyle or knitr. I was having this issue on my personal laptop until I updated BiocStyle. I tried addressing this by adding a BiocStyle version (BiocStyle (>= 2.9.1)) but that didn't seem to fix the probelm. However, when I build the vignette on my personal laptop, I'm no longer seeing this warning, but it seems to be coming back in the Bioconductor building.

Thanks again,

Abbas

vobencha commented 6 years ago

Hi, Re (1) Yes, that's fine. Re (2) Thank you. Re (3-4) OK Re (5) warnings

I'm no expert with knitr but the warning your seeing is coming from knitr:::process_file(). knitr::purl() calls down to knitr:::knit() which makes it's way down to process_file().

The bash script code chunk is the problem. knitr:::process_tangle() can't seem to parse that chunk as labeled (it produces a list of length > 1). If you change the label to {r, eval = FALSE} it works fine. I'm sure there is a way to correctly specify shell scripts or 'asis' chunks - you could ask on the mailing list, someone may know.

I was testing with BiocStyle 2.9.2. You say this went away with BiocStyle 2.9.1? We currently have 2.9.2 on the devel build machine so that is the version the version the SPB will use. I did also cleaned up a few things in BiocCheck so if you're testing locally be sure to get the latest version.

Why don't you investigate that bash code chunk and see if you can find a solution. Bump the version and see if we get a clean build. If the bash code label should work as advertised you could open an issue on the knitr github site.

Valerie

bioc-issue-bot commented 6 years ago

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

081af25 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.

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 6 years ago

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

31d62f0 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.

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 6 years ago

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

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

bioc-issue-bot commented 6 years ago

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

2f70bf2 version bump, removed bash

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

aarizvi commented 6 years ago

Hi,

I got it to pass the OS builds and you were correct about it being the bash code chunk in my .Rmd giving those warnings in the vignette.

I removed Depends: R (>= 3.5) from my DESCRIPTION file.

If I put any older version I get this warning message during the R CMD BiocCheck:

* Checking R Version dependency...
    * WARNING: Update R version dependency from 3.4 to 3.5.

Is there a way to bypass this so I can have users of this package install it with R >= 3.4?

Thanks, Abbas

vobencha commented 6 years ago

We can ignore the warning from BiocCheck during the review process. You won't see this with R CMD check on the build machines.

bioc-issue-bot commented 6 years ago

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

c2cfe31 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.

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.

bioc-issue-bot commented 6 years ago

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

e68ccb7 version bump, fixed neg times again

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.

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.

vobencha commented 6 years ago

Thanks for making the changes. The package is approved.

bioc-issue-bot commented 6 years ago

Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be posed to this issue in the next several days.

Thank you for contributing to Bioconductor!

mtmorgan commented 6 years ago

The master branch of your GitHub repository has been added to Bioconductor's git repository.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/aarizvi.keys is not empty), then no further steps are required. Otherwise, do the following:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using biocLite(\"gwasurvivr\"). The package 'landing page' will be created at

https://bioconductor.org/packages/gwasurvivr

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.