Closed npcooley closed 4 years ago
Hi @npcooley
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: SynExtend
Type: Package
Title: Tools for working with Synteny Objects
Version: 0.99.05
Date: 2020-02-12
Authors@R: c(
person("Nicholas", "Cooley", email = "npc19@pitt.edu", role = c("aut", "cre"), comment = c(ORCID = "0000-0002-6029-304X")),
person("Adelle", "Fernando", email = "Adelle.fernando@gmail.com", role = "ctb"),
person("Erik", "Wright", email = "ESWRIGHT@pitt.edu", role = "aut"))
BiocVews: Genetics, Clustering, ComparativeGenomics
Description: Tools for working with Synteny Objects
Depends: R (>= 3.6.0), DECIPHER (>= 2.14.0), igraph (>= 1.2.4.1)
Imports: Biostrings, IRanges, S4Vectors
Suggests: knitr
License: GPL-3
Encoding: UTF-8
LazyData: true
NeedsCompilation: no
VignetteBuilder: knitr
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.
please provide a more extensive Description:, like a paper abstract describing the unique contributions your package is making.
Please mark the first code chunk of your vignette with {r, eval = FALSE}
so that building the vignette does not trigger installing the package.
In the vignette, please use rtracklayer::import()
to import the gff file into a standard R data representation; make sure that this representation can be used as inputs to your functions.
The assigned reviewer will provide additional comments after you have made these (and any other issues identified by the automatic checks) changes.
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.
Received a valid push; starting a build. Commits are:
9c7f526 Vignette and Description edits
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.
Received a valid push; starting a build. Commits are:
bd79693 warnings and errors 001
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.
Received a valid push; starting a build. Commits are:
c0b28fc version bump?
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.
Received a valid push; starting a build. Commits are:
bc20016 address warnings?
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.
Received a valid push; starting a build. Commits are:
ef5a5e9 added imports adjusted examples
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.
Received a valid push; starting a build. Commits are:
96ef990 version bump?
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.
Received a valid push; starting a build. Commits are:
6205e22 formatting and DESC adjustments
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.
Received a valid push; starting a build. Commits are:
aa3327f vignette formatting
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.
Received a valid push; starting a build. Commits are:
bcd7c60 vignette formatting
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.
Received a valid push; starting a build. Commits are:
dc64ae8 vignette formatting
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.
Received a valid push; starting a build. Commits are:
2781165 R Version Dependency
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.
Build Report
[ ] We highly suggest having unit tests
[ ] We highly suggest adding a NEWS file. The NEWS file is what we use to generate the release announcements to recognize changes and updates to code. example release announcement
README
DESCRIPTION
[ ] Remove Date field. Should be generated automatically and not needed to put directly in the DESCRIPTION
[ ] Please remove LazyData: true
we have found this rarely proves useful and can actually slow down the installation of a package
NAMESPACE
INST
Data
Vignette
rtracklayer::import()
to import the gff file into a standard R data representation; make sure that this representation can be used as inputs to your functions. GenomicRanges GRanges object is the common class structure in Bioconductor to represent genomic positions. man pages
data man pages
TBA
NucleotideOverlap
PairSummaries
gffToDataFrame
rtracklayer::import
R code In general the code need clean up and compression.
PairSummaries
[ ] GeneCalls should be GRange or GRangeList
[ ] Do all checking for valid arguments at the start of the function. Include additional checks for model, correction, etc.
[ ] can some of the conditional code be together. Example: All if(PID) in the same location instead of multiple separate conditionals. condense where possible
[ ] Instead of multiple
TotalCoverage <- vector("list",
length = Total)
MinCoverage <- vector("list",
length = Total)
MaxCoverage <- vector("list",
length = Total)
It would be more readable to do
TotalCoverage <- MinCoverage <- MaxCoverage <- vector("list", length = Total)
It looks like you did this for some but not all.
[ ] Minimize function calls. It would be cleaner to assign arguments in conditional and have a single call to the functions. This is an example but should be applied throughout
if (!(GeneCalls[[m1]][SyntenyLinks[[m1, m2]][i, "QueryGene"], "Coding"]) |
!(GeneCalls[[m2]][SyntenyLinks[[m1, m2]][i, "SubjectGene"], "Coding"]) |
IgnoreDefaultStringSet) {
# align as nucleotides
IDType[[Count]][i] <- "DNA"
Scores[[Count]][[i]] <- 1 - DistanceMatrix(myXStringSet = AlignSeqs(myXStringSet = Scores[[Count]][[i]],
verbose = FALSE),
penalizeGapLetterMatches = GapPenalty,
includeTerminalGaps = TerminalPenalty,
correction = Correction,
type = "matrix",
verbose = FALSE)[1, 2]
} else {
# align as AAs
IDType[[Count]][i] <- "AA"
Scores[[Count]][[i]] <- 1 - DistanceMatrix(myXStringSet = AlignTranslation(myXStringSet = Scores[[Count]][[i]],
sense = "+",
direction = "5' to 3'",
type = "DNAStringSet",
readingFrame = 1L,
verbose = FALSE),
penalizeGapLetterMatches = GapPenalty,
includeTerminalGaps = TerminalPenalty,
correction = Correction,
type = "matrix",
verbose = FALSE)[1, 2]
Would become.
if (!(GeneCalls[[m1]][SyntenyLinks[[m1, m2]][i, "QueryGene"], "Coding"]) |
!(GeneCalls[[m2]][SyntenyLinks[[m1, m2]][i, "SubjectGene"], "Coding"]) |
IgnoreDefaultStringSet) {
myXStringSet = AlignSeqs(myXStringSet = Scores[[Count]][[i]], verbose = FALSE)
type = "DNA"
}else{ myXStringSet = AlignTranslation(myXStringSet = Scores[[Count]][[i]], sense = "+", direction = "5' to 3'",type = "DNAStringSet", readingFrame = 1L, verbose = FALSE) type = "AA } IDType[[Count]][i] <- type Scores[[Count]][[i]] <- 1 - DistanceMatrix(myXStringSet = myXStringSet, penalizeGapLetterMatches = GapPenalty, includeTerminalGaps = TerminalPenalty, correction = Correction, type = "matrix", verbose = FALSE)[1, 2]
This eliminates redundant code and condense the code to make it easier to read. Please apply throughout this script and NucleotideOverlap.R
Another example
if (PIDs) { DF <- data.frame("TotalCoverage" = unlist(TotalCoverage), "MaxCoverage" = unlist(MaxCoverage), "MinCoverage" = unlist(MinCoverage), "NormDeltaStart" = unlist(NormDeltaStart), "NormDeltaStop" = unlist(NormDeltaStop), "NormGeneDiff" = unlist(NormGeneDiff), "ExactMatch" = unlist(ExactMatch),
# "RightDeviation" = unlist(AbsStopDelta),
# "SubjectLeft" = unlist(ExactLeftS),
# "SubjectRight" = unlist(ExactRightS),
# "QueryLeft" = unlist(ExactLeftQ),
# "QueryRight" = unlist(ExactRightQ),
"PID" = unlist(Scores),
"PIDType" = unlist(IDType),
stringsAsFactors = FALSE)
rownames(DF) <- unlist(LabelNames)
} else { DF <- data.frame("TotalCoverage" = unlist(TotalCoverage), "MaxCoverage" = unlist(MaxCoverage), "MinCoverage" = unlist(MinCoverage), "NormDeltaStart" = unlist(NormDeltaStart), "NormDeltaStop" = unlist(NormDeltaStop), "NormGeneDiff" = unlist(NormGeneDiff), "ExactMatch" = unlist(ExactMatch),
# "RightDeviation" = unlist(AbsStopDelta),
# "SubjectLeft" = unlist(ExactLeftS),
# "SubjectRight" = unlist(ExactRightS),
# "QueryLeft" = unlist(ExactLeftQ),
# "QueryRight" = unlist(ExactRightQ),
stringsAsFactors = FALSE)
rownames(DF) <- unlist(LabelNames)
}
You could construct the overlapping DF first and then apply the additional columns in the conditional only.
And again with choosing the model. The conditional should be the model difference
and then only a single call
if (Model == "Global") { data("GlobalSelect", envir = environment(), package = "SynExtend") modelData = GlobalSelect else if (Model == "Local") { data("LocalSelect", envir = environment(), package = "SynExtend") modelData = LocalSelect }
KeepSet <- predict(object = modelData, DF, type = response") DF <- cbind(DF,"ModelSelect" = KeepSet >= 0.5)
- [ ] Remove commented out code
_NucleotideOverlap_
- [ ] OutputFormat doesn't change between options? Also have a check for valid
value of OutFormat at the start of the function.
Links <- NucleotideOverlap(SyntenyObject = Syn,
- GeneCalls = GeneCalls,
- LimitIndex = FALSE,
- Verbose = FALSE) Links 1 2 3 1 1683 Pairs 1760 Pairs 2 609223 Nucleotides 1781 Pairs 3 802650 Nucleotides 623239 Nucleotides
Links <- NucleotideOverlap(SyntenyObject = Syn,GeneCalls=GeneCalls, LimitIndex=FALSE, Verbose=FALSE, OutputFormat="Comprehensive") Links 1 2 3 1 1794 Genes 1683 Pairs 1760 Pairs 2 609223 Nucleotides 2264 Genes 1781 Pairs 3 802650 Nucleotides 623239 Nucleotides 2216 Genes Links <- NucleotideOverlap(SyntenyObject = Syn,GeneCalls=GeneCalls, LimitIndex=FALSE, Verbose=FALSE, OutputFormat="Sparse") Links 1 2 3 1 1683 Pairs 1760 Pairs 2 609223 Nucleotides 1781 Pairs 3 802650 Nucleotides 623239 Nucleotides
- [ ] Check valid argument values at the start of function.
- [ ] GeneCalls should be GRange or GRangeList
- [ ] Apply comments to avoid redundant code and to condense where possible to this script as well.
_gffToDataFrame_
- [ ] Remove this entirely and use already implemented Bioconductor function `rtracklayer::import`
Please apply the above corrections and clean up. When ready make sure there is a new build with a valid version bump and comment back here with a summary of the updates.
Thank you for your submission to Bioconductor
~ Lori
Hi Lori,
Thanks for all the reviewing! I'll get started on your comments as I have time.
I would be happy to make NucleotideOverlap flexible in that it could take in a Genomic Ranges object, but just the wholesale import of the GFF is actually not useful to us. The text parsing that the gffToDateFrame performs is just sort of speculative, but all the parent/id matching and condensing would still have to be performed on the Genomic Ranges object anyway. It made more sense for us to parse it straight from the original file.
I.e. a naive user would be just fine pulling the gene bounds from a gff generated by Prodigal or Prokka using a rtracklayer::import
because those programs don't think about genes with complex bounds, but if I pull a generic refseq or genbank gff, it will need additional condensing to drop redundant bound sets (such as were the same bound is marked for "gene", "mrna", "exon", and "cds"), and to correctly think about the coding space for cases where multiple exons exist.
Keeping a condensed format is useful for me internally because it allows me to correctly reconcile CDS and gene bounds in a single place, and keep track of indices in the source FASTA file. It seems useful for a naive user as well because the data format is just simpler.
Once again I'm happy to make it flexible, and if GRanges objects can support ragged ranges for gene positions I can convert my output to that, but those are changes I would prefer to implement in a later release if possible.
Cheers, Nick
In Bioconductor the recommended and almost always required structure for data involving genomic position information is GenomicRanges in either a GRanges or GRangesList object. Users that have been using Bioconductor pipelines and workflows will expect this and at minimum expect the function to be able to handle this type of object. There is also some consideration of how your parser handles the range and range intervals. Bioconductor standards for ranges are 1-based, closed intervals and this would have to be implemented so there is no discrepancy from standard usage throughout Bioconductor; this is the reasoning for using the rtracklayer::import function as it takes all of this into consideration when doing the parsing; it is a fast and developed function that would guarantee correctness. It seems like your other manipulations (parent/id matching, etc) and additions of information can be performed on the imported GenomicRanges object and this would be preferred. The GenomicRanges package has been highly developed over the years, as a very mature package, and has many useful functionalities that could prove useful in development. Please update the package in the current version before acceptance; in our experience promises of updating in future implementations never come to fruition.
Received a valid push; starting a build. Commits are:
13b5d14 man and r code comments, GRange functionality incl...
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.
Received a valid push; starting a build. Commits are:
bfa4380 version
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.
Received a valid push; starting a build. Commits are:
18c7b85 Warnings
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.
Received a valid push; starting a build. Commits are:
b6da027 removed R version dependency
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.
Received a valid push; starting a build. Commits are:
402182c R version readded
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.
Received a valid push; starting a build. Commits are:
eb70704 man edits
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.
Received a valid push; starting a build. Commits are:
4143df3 Vignette edits
Received a valid push; starting a build. Commits are:
1c7d13a vignette edits
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.
Received a valid push; starting a build. Commits are:
c611677 more vignette edits
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.
Is the package ready for a re-review? If so please summarize changes that have been made here.
Please remember all packages must be reviewed and accepted by april 22 to be included in the 3.11 release else they will be in devel until the released in fall under bioc 3.12
Ohp, I missed your earlier notification, sorry! Yes, the package is ready for review.
Hi,
I know this is cuttiing it close to the deadline, but is it possible to get a review back for this package today?
Cheers, Nick
On Thu, Apr 16, 2020 at 9:51 AM lshep notifications@github.com wrote:
Please remember all packages must be reviewed and accepted by april 22 to be included in the 3.11 release else they will be in devel until the released in fall under bioc 3.12
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/1369#issuecomment-614665887, or unsubscribe https://github.com/notifications/unsubscribe-auth/AJMES4WOEYFG2ZIGNYYEXZLRM4EMTANCNFSM4KU2LNPQ .
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]'
[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:
For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.