AssessORF - Githubissues

dkorandla commented 6 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/DRK248/AssessORF

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 Git (optionally via GitHub).

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

bioc-issue-bot commented 6 years ago

Hi @DRK248

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: AssessORF
Type: Package
Title: Assess Gene Predictions Using Proteomics and Evolutionary Conservation
Version: 0.1.6
Date: 2018-06-04
Authors@R: c(
    person("Deepank", "Korandla", email = "dkorandl@andrew.cmu.edu", role = c("aut", "cre")),
    person("Erik", "Wright", email = "eswright@pitt.edu", role = "aut"))
Description: In order to assess the quality of a set of predicted genes for a genome, evidence
    must first be mapped to that genome. Next, each gene must be categorized based on how strong
    the evidence is for or against that gene. The AssessORF package provides the functions and
    class structures necessary for accomplishing those tasks, using proteomic hits and
    evolutionarily conserved start codons as the forms of evidence.
Depends: R (>= 3.5.0), DECIPHER (>= 2.8.0)
Imports: DECIPHER
Suggests: AssessORFData,
    BiocStyle,
    knitr,
    rmarkdown
biocViews: ComparativeGenomics, GenePrediction, GenomeAnnotation, Genetics, Proteomics,
    QualityControl, Visualization
License: GPL-3
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.0
VignetteBuilder: knitr

dkorandla commented 6 years ago

AdditionalPackage: https://github.com/DRK248/AssessORFData

bioc-issue-bot commented 6 years ago

Can't build unless issue is open and '2. review in progress' label is present, or issue is closed and 'TESTING' label is present.

bioc-issue-bot commented 6 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 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: "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:

d539ed8 Update DESCRIPTION Changed version number to sign...

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: "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:

5db6bce Update DESCRIPTION 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:

4a7e403 Add files via upload Temporarily removed dependen...

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: "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:

cf4d501 Add files via upload Fixed spelling mistake in Sc...

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: "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:

6dcb9e3 Add files via upload Decreased package size

bioc-issue-bot commented 6 years ago

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

edd0342 Update DESCRIPTION

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: "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

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

bioc-issue-bot commented 6 years ago

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

d420956 Add files via upload Added a runnable example to ...

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: "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.

bioc-issue-bot commented 6 years ago

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

54fde06 Add files via upload Addresses some warnings

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:

00785f6 Add files via upload

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:

3e847f5 Add files via upload Made changes to the vignette

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:

03b2627 Uploading Files 521294d Removed branch Merge branch 'master-holder' # Co... 0f687b2 clean up Merge branch 'master' of https://github....

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:

9c10468 add man pages 7493a21 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:

5fe7ddf Added details and value to ScoreAssessmentResults ... 889af7e Re-built package for pushing

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.

dkorandla commented 6 years ago

I have done my best to address the errors and warnings returned by the R CMD CHECK and R CMD BiocCheck. I believe that the warnings that are left are unavoidable. For example, BiocCheck is asking for value sections to be added to the man pages for S3 methods extended by the package that do not have value sections in their base man pages. BiocCheck is also asking for more runnable code chunks in my vignette, and I plan to address this by making the installation code chunks runnable once the package is available on Bioconductor.

Since I have done my best to clear up the errors and avoidable warnings, do you think that my package is ready for review? If so, when will my package be reviewed, and can you give me a rough idea of the package review timeline?

Additionally, I plan on submitting a data package along with AssessORF. I should submit the data package and configure both packages to work with each other once AssessORF has been reviewed, correct?

mtmorgan commented 6 years ago

Thanks for your package. Please address the following comments and when ready add a short comment indicating how you have responded.

It would be appropriate to add to this issue an 'AdditionalPackage' line as described in the documentation; when it is also ready I will review it.

DESCRIPTION, NAMESPACE

Good

vignette

update installation instructions to use BiocManager::install(), see, e.g., instructions at https://bioconductor.org/packages/devel/DECIPHER
unevaluated code chunks. The problem with unevaluated code chunks is that they quickly enter 'bit rot' where the code is no long correct. It is much better to provide toy data to work with in your vignette, e.g., for the code chunk at l. 79 with files under inst/extdata etc., and accessed in the vignette with system.file(package="AssessORF", "extdata", "...")
l. 96: is sapply() simply basename()?

l 109: this looks more natural as a vapply() to avoid memory management

pass <- vapply(ftps, function(ftp) {
    t <- try(Seqs2DB(ftp, "FASTA", dbConn, as.character(i),
                 compressRepeats=TRUE, verbose=FALSE), silent=TRUE)
    !is(t, "try-error")
}, logical(1))

R

AssessGenes.R:116 geneLeftPos / geneRightPos / geneStrand look like GenomicRanges; use that class as input. This will avoid confusion about coordinate system (1-based) and interval (closed) that are otherwise ambiguous. It will also provide validity checking, e.g., that left pos <= right pos and, when genome is retrieved from inputMapObj, that the coordinates are consistent. Likely parts of the implementation in this function can be simplified and made more robust by using the GenomicRanges interface.

(Optional) adopt a formatting style that allows for longer lines, e.g., AssessGenes.R:415

cat1A_ORFs <- matrix(
    0, nrow = sum(sapply(stops, length)), ncol = 5,
    dimnames = list(
        NULL, c("Start", "End", "Length", "Frame", "OtherProtFrame")
    )
)

e.g., AssessGenes.R:489 usually it's not necessary to use which(); the logical vector is sufficient for subsetting. Again, adopt a formatting style that increases legibility, e.g., by avoiding excessive nesting
```
idx <- strandFrameStarts >= orfStart & strandFrameStarts <= orfEnd
regionalPredictedStart <-strandFrameStarts[idx]
```

AssessGenes.R:756 Use S4 rather than S3 classes; it is not that much more verbose and offers significant advantages, e.g., type checking. Re-use existing classes such as GRanges.

setClass("Results")
.Assessment <- setClass(
    "Assessment", contains = "Results",
    slots = c(granges = "GRanges", ...)
)

Assessment <- function(...) {
    .Assessment(...)
}

setMethod("show", "Assessment", function(object) {
    ## display informative but not overwhelming information -- body of print.Assessment
})

Assessment-class.R:184 This looks like it can be vectorized

printOut <- character(length(allCatSums))
idx <- names(allCatSums) == "Y CS+ PE+"
printOut[idx] <- paste0(currCat[idx], " (correct with strong evidence):", allCatSums[idx])
...
paste(printOut, collapse="\n ")

MapAssessmentData.R:207: use message() when communicating with users (other than during object display)

MapAssessmentData.R:646: this also looks like it can be vectorized, at least in part

gPos <- seq_len(genomeLen - 2L)
fwdCodon <- subseq(fwdCodon, gPos, gPos + 2)
idx <- fwdCodon %in% startCodons
fwdCodonStarts[gpos[idx]] <- 0L
...

ScoreAssessmentResults.R:30 use is() rather than string identity to test for class

man

ok

bioc-issue-bot commented 6 years ago

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

2577864 Addressed reviewer comments 7750bf7 Addressed reviewer comments (cont.)

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: "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:

6526d36 Add files via upload

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: "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.

dkorandla commented 6 years ago

Thank you so much for reviewing my package, 'AssessORF'. I have detailed my response to the feedback below:

For the vignette, I have made all the suggested changes. Besides the installation code chucks, only one of the code chunks in the vignette is not evaluated. That particular code chunk shows a generalized call to a key function in the package and outlines what parameters to specify in most situations. Prior to that code chunk in the vignette, I provided a runnable example of that key function, demonstrating how it works in one particular situation. I plan on making the installation code chunks runnable once the package is in Bioconductor as well, reducing the number of unevaluated code chunks to one.

For my R code, I have made most of the suggested changes. I chose not to adopt an optional formatting style change that allows for longer lines. I also did not make two other changes, and I have detailed my reasoning below:

AssessGenes.R:116 geneLeftPos / geneRightPos / geneStrand look like GenomicRanges; use that class as input. This will avoid confusion about coordinate system (1-based) and interval (closed) that are otherwise ambiguous. It will also provide validity checking, e.g., that left pos <= right pos and, when genome is retrieved from inputMapObj, that the coordinates are consistent. Likely parts of the implementation in this function can be simplified and made more robust by using the GenomicRanges interface.

Using a 'GenomicRanges' object in the input for 'AssessGenes' is inappropriate because it makes it more difficult for users to utilize the function. The primary purpose of the 'AssessGenes' function is to assess the quality of predictions from a gene finding program. These programs typically have no interface with R, which means that a user will have to load the output from such a program into R and then parse the output appropriately before they can use the function. Asking users to convert the parsed output into a 'GenomicRanges' object on top of that before using 'AssessGenes' will create an additional burden for the user. Furthermore, I do the required validity checking internal to the function, and I am explicit about how users need to input gene positional information to the function. I do not want to add an additional dependency to 'AssessORF' either.

AssessGenes.R:756 Use S4 rather than S3 classes; it is not that much more verbose and offers significant advantages, e.g., type checking. Re-use existing classes such as GRanges.
setClass("Results")
.Assessment <- setClass(
  "Assessment", contains = "Results",
  slots = c(granges = "GRanges", ...)
)

Assessment <- function(...) {
  .Assessment(...)
}

setMethod("show", "Assessment", function(object) {
  ## display informative but not overwhelming information -- body of print.Assessment
})

The S3 object system suits the needs of 'AssessORF' better than the S4 object system. The main benefit of using the S4 system in place of the S3 system is type-checking. However, all the functions within the package that utilize S3 objects type-check the objects internally so there would be no benefit to switching to S4 in that regard. Additionally, the two objects within 'AssessORF' that utilize the S3 system are just lists with defined class attributes. There is no complex relationship between those two objects that might necessitate the use of the S4 system. For these reasons, I have chose not to switch my package over to using the S4 object system.

bioc-issue-bot commented 6 years ago

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

cfcd458 Addressed reviewer comments (cont.) f90b12f Version skip 8c7c796 Attempt to fix Git error ac86565 Updating documentation 30ad90f Updating documentation (cont.) 30363cb Made changes to finding synteny and conserved stop... 528fa63 Documentation update from previous commit 6c8ae0f Version bump for previous 2 commits 58096fe Adjusted parameters and fixed coverage calculation d044a37 Documentation update for previous commit

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:

71c22b8 Updates to documentation Changes to stop codon cat... 2776ca9 AssessGene Fix Updates to MAN pages

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:

dd75d08 Vignette update 32d4bb2 Vignette fix

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.

mtmorgan commented 6 years ago

Please be sure to address the straight-forward notes and warnings about missing imports (importFrom(...), use of vapply() / seq_len() / seq_along(), duplicating DECIPHER import / depends.

View the suggestion to use GenomicRanges as 'and' rather than 'or' -- implement the appropriate method to support GRanges input, or your ad hoc method.

Please let me know by posting a brief comment here when your package is ready for final review.

bioc-issue-bot commented 6 years ago

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

8073ab6 Minor revisions to the vignette and documentation b3cb75f Corresponding manual page updates 4608752 Clearing up check warnings and notes; updating the... 251d4df Documentation for previous commit 8b7601a Documentation for previous commit b943b13 NAMESPACE update for previous commit 0b7c5d4 test fix e96b230 test fix 490345f test fix 4c20659 test fix b28cb23 test fix 4da37e7 test fix d25dd24 test fix 0e07a68 test fix f01997d test fix 016a5a9 test fix e07f655 test fix 37f31d8 test fix

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, TIMEOUT, 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:

709644e Minor changes: sapply --> vapply, 1:.. --> seq_len 27453c5 NEWS file added cf65025 NEWS file added 3787dde NEWS file added fba36fa NEWS file added 21c27f5 NEWS file added

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, TIMEOUT, 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:

5fc81c6 Minor edits to man pages and the vignette

Bioconductor / Contributions

AssessORF #858