search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

PharmacoGx #23

Closed p-smirnov closed 8 years ago

p-smirnov commented 8 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 8 years ago

Hi @p-smirnov

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: PharmacoGx
Type: Package
Title: Analysis of Large-Scale Pharmacogenomic Data
Version: 1.1.7
Date: 2016-06-15
Author: Petr Smirnov, Zhaleh Safikhani, Benjamin Haibe-Kains
Maintainer: Benjamin Haibe-Kains <benjamin.haibe.kains@utoronto.ca>
Description: Contains a set of functions to perform large-scale analysis of pharmacogenomic data.
License: Artistic-2.0
Suggests: xtable
Encoding: UTF-8
Imports: Biobase, piano, magicaxis, RColorBrewer, parallel, caTools, methods, downloader, stats, utils, graphics, grDevices, lsa
Depends: R (>= 3.2)
RoxygenNote: 5.0.1
biocViews: GeneExpression, Pharmacogenetics, Pharmacogenomics, Software
BugReports: https://github.com/bhklab/PharmacoGx/issues
p-smirnov commented 8 years ago

Hello Valerie,

I am resubmitting through the new github tracker as you directed. To address the main issues that will come up, we want to transfer the package from CRAN to Bioconductor since it heavily relies on other Bioconductor packages and fits with the focus well. We will ask CRAN to remove the package once the review process is finished, but the version number should remain > 1.1.6 as that is the version currently on CRAN. Should we bump the version number of the submission to 1.99.z or similar?

Best, Petr

vobencha commented 8 years ago

Hi, Thanks for resubmitting in the new location. It's fine to leave the package on CRAN until the approval process is done; just wanted to make sure you were aware it would have to be removed. Why don't you bump the version to 1.1.7 and we'll go from there. Below I've pasted in my comments from the first review. Please let me know which issues have been addressed (of those that are still relevant). Valerie

From the old tracker:

I've done a first review of PharmacoGx. The package looks good, just a few suggestions and comments below.

1) removal from CRAN

We can't add the package to Bioconducotor until it is removed from CRAN.

2) biocCheck issues

Please address these:

3) unnecessary files in vignettes/

I think CreatingPharmacoSet-concordance.tex in vignettes/ is unnecessary? Please remove or explain why you have it there.

4) man pages for data/ files

Please explain the data files in more detail. What type of object are they (e.g., matrix, data.frame, named vector) and what data do they contain. If the data are manipulated or derived please note the origin and what was done to produce the final output.

For example, data(CMAPsmall) loads a PharmacoSet that is 51 by 1018. What do the 51 and 1018 stand for? mention that there are 5 cell lines, 5 drug compounds etc.

5) S4 classes

It looks like you define two S4 classes, PharmacoSig and PharmacoSet but only PharmacoSet is exported. The getters and setters for this class should be defined using '@' slot access. Once that's done all other methods should use the getters and setters and not direct slot access with'@'.

For example, why does the show,PharmacoSet-method use '@'? The show method should use the defined getters and setters.

6) Why the layered PharmacoSet and .PharmacoSet class design?

p-smirnov commented 8 years ago

Hello Valerie,

Going in order:

p-smirnov commented 8 years ago

@vobencha There have also been some new functions added to the package, could you run the package through the bioconductor test build system to test the current version of the package?

vobencha commented 8 years ago

Hi,

To start a new build, commit the changes to your github repo and bump the version DESCRIPTION. The incremented version is the trigger.

Thanks for making the changes and for giving detailed explanations. You asked about the built pdf files - we don't want them in svn because they are redundant when the files (pdf and Rnw) are in sync and stale when they aren't. Unless you build the package yourself each time a change is made the pdf on github will likely be out of date. The problem with using .RBuildIgnore and .RInstallIgnore is that the pdf file is still checked into svn.

If you want to maintain a separate directory of documentation under inst/, such as inst/extdocs you could. The problem is (again) that the vignette pdf stored in inst/extras will be out of snyc of the vignette built with R CMD build unless you hand build, untar, transfer the file and retar.

Valerie

p-smirnov commented 8 years ago

@vobencha Ok, no problem to remove those files! I am not 100% sure about the current build system, but looking at the process for the review of other packages, they were all marked "approved for building" with a message to set up the web hook to the repository, which hasn't happened in this review process. Would the web hook still work without confirmation from your side?

vobencha commented 8 years ago

Did you set up a web hook in the PharmacoGx repo as described here:

https://github.com/Bioconductor/Contributions/blob/master/CONTRIBUTING.md#adding-a-web-hook

If you've done that then the problem is on our side.

vobencha commented 8 years ago

I see, the initial build didn't even happen. Let me look into this with Martin and Lori. In the mean time, please make sure your webhook is set up. Thanks.

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: "abnormal". 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/PharmacoGx_buildreport_20160621114221.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

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

dc97ea6 Trigger Build Attempt to trigger build (first one...

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: "abnormal". 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/PharmacoGx_buildreport_20160621124658.html

p-smirnov commented 8 years ago

@vobencha: Hi Valerie,

If I had to guess, I think the issue is that the default branch (only branch) of our repository is not named master, so the build system cannot fetch the package automatically. The file size is reported as -1 KB.

Petr

bioc-issue-bot commented 8 years ago

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

316224a version bump

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, 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 following build report for more details:

http://bioconductor.org/spb_reports/PharmacoGx_buildreport_20160624144832.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

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

30ef4e8 Version Bump to trigger build (CRAN warnings resol...

p-smirnov commented 8 years ago

Hi @mtmorgan, @vobencha,

The build system still fails to download the package and check it, although the failure is inconsistent between checks.

Also, from the previous check I could not reproduce the vignette building error and it resolved itself this time around.

Best , Petr

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, 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 following build report for more details:

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

vobencha commented 8 years ago

Hi,

We're away at BioC2016 and will look into the build problem when we get back on Wednesday.

A few follow up questions:

1) Are you registered on bioc-devel?

2) I want to revisit this bullet from your message above:

" Value sections: I believe all the function listed are show methods or plotting methods (or datasets which have a format section instead of a value section) Runnable examples added to the some functions, other functions either download objects from the Internet or plot/print values. The PharmacoSet constructor has a vignette dedicated to it as it is more complex than would be intelligible in a man page. "

Which trigger these BiocCheck messages:

i) You said all man pages that did not have \value had a \format section. That is not true for quite a number of those pages. Looks like these have neither \value or \format: checkPSetStructure.Rd computeSlope.Rd dim-PharmacoSet-method.Rd drugDoseResponseCurve.Rd PharmacoSet-class.Rd show-PharmacoSet-method.Rd showSigAnnot.Rd sub-PharmacoSet-method.Rd

ii) The point of the man pages and vignette is to test exported functions in the nightly builds. The idea is early detection when something goes wrong with either your package or a dependency, a change in R etc. Ideally, if a function is not tested in the man page it should be run in the vignette. Because of all the eval=FALSE statements in the vignette, these functions do not have run-able man page examples and are not tested in the vignette so they are not tested anywhere in the package: availablePSets.Rd downloadPertSig.Rd downloadPSet.Rd, drugDoseResponseCurve.Rd mDataNames.Rd showSigAnnot.Rd

Are you concerned about runtime?

iii) Please mention the vignette in the \details or \description section of PharmacoSet.Rd and add an example section that directs the user to the vignette, e.g., browseVignettes("PharmacoGx")

Valerie

bioc-issue-bot commented 8 years ago

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

de51705 Bioconductor submission fixes

p-smirnov commented 8 years ago

Hi Valerie (@vobencha),

Thank you for your help dealing with the issues! The last build seemed to go through, even though moscato still reports a package size of -1.0 KB.

1) The maintainer is subscribed to the mailing list now.

2) For the following files: show-PharmacoSet-method.Rd showSigAnnot.Rd sub-PharmacoSet-method.Rd

They do not return anything, they only print using cat/warnings/message/print/stop to the respective output connections.

The file: drugDoseResponseCurve.Rd Graphs to the current graphics device but does not return anything. The remaining files have value sections added.

Similarly: availablePSets.Rd downloadPertSig.Rd downloadPSet.Rd, All download files from the internet of 100's of MBs. It seems bad practice to have them run on your package check servers. Also, drugDoseResponseCurve is not run by the check in case the servers don't have access to a graphics device (And also because it does not close the graphics device).

The other two files I added runnable examples.

3) Reference added to the documentation page.

I sent the package for another build, hopefully no new errors come up.

Thank you once again! Petr Smirnov

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, 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/PharmacoGx_buildreport_20160627220854.html

vobencha commented 8 years ago

Hi Petr,

According to section 2.1.2 of Writing R Extensions, \format is used to describe data sets, not functions, whereas \value is for function return values.

Additionally, you need to have one \value section per man page. The one page per function approach of roxygen is not very useful and combining methods of similar type or class-related would make more sense. It looks like there are a couple of ways that roxygen documentation can occupy the same man page. One way is to use @rdname when within each component (e.g., parameter args) are collated in the order they appear in the .R file. I think there is also @describeIn. Whatever you decide, each page needs a \value section even if it just says 'no output' or 'describes contents of object "x"', etc.

if interactive() {
 ...
}

This way a user calling example(availablePSets) from a workspace can see what the function does.

Valerie

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.

vobencha commented 8 years ago

Hi, Just checking in. Let me know if you have any questions. Valerie

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:

489f992 transfer changes acf3c97 Fixing some bugs users pointed out

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, 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 following build report for more details:

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

p-smirnov commented 8 years ago

Hi @vobencha,

We were fixing some issues pointed out by users of the package and also addressing the rest of the documentation issues. I think the main issues are address, for some reason BiocCheck does not recognize that these are dataset documentation pages and not functions, requiring a format and not value section: RECOMMENDED: Add non-empty \value sections to the following man pages: CCLEsmall.Rd, CMAPsmall.Rd, GDSCsmall.Rd, HDAC_genes.Rd

Otherwise I think all the other doc pages have the required information.

Petr

p-smirnov commented 8 years ago

Hi @vobencha.

I was wondering if you had a chance to take a look at the latest push. I believe all the issues have been addressed in the build, please let me know if anything else is required.

Thank you, Petr

vobencha commented 8 years ago

No, sorry, I haven't. For some reason I thought you were still working on it. Will look at it today. Valerie

vobencha commented 8 years ago

OK, I think we can call it good. Thanks for making all the changes and improving the package.

You'll get an email from Martin in the next week with svn credentials and next steps. Once the package is in Bioconductor you'll need to contact the CRAN maintainers to get it removed. Hosting in both CRAN and Biocondcutor will be a problem for biocLite() which looks in both repos.

I would also reccomend bumping the github version to 1.2.0 so there is a clear distinction from the 1.1.* version in CRAN.

Valerie

vobencha commented 8 years ago

Actually make that 1.3.0. The convention is that Bioconductor versions (x.y.z) are odd 'y' in devel and even 'y' in release.

p-smirnov commented 8 years ago

Ok, perfect! Thank you for your help and feedback, will bump the version to 1.3.0 before its pushed to svn. We will continue working on the package, incorporating feedback from users and improving the documentation prior to the next BioC release.