Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

mobileRNA #3076

Closed KJeynesCupper closed 7 months ago

KJeynesCupper commented 1 year ago

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

Repository: https://github.com/KJeynesCupper/mobileRNA 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 Bioconductor Package Naming Policy and acknowledge Bioconductor may retain use of package name.

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

[ x] I am familiar with the Bioconductor code of conduct and agree to abide by it.

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 questions/help about the submission process, including questions about the output of the automatic reports generated by the SPB (Single Package Builder), please use the #package-submission channel of our Community Slack. Follow the link on the home page of the Bioconductor website to sign up.

bioc-issue-bot commented 1 year ago

Hi @KJeynesCupper

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: mobileRNA
Type: Package
Title: Identify mobile RNA molecules in plant graft systems 
Version: 0.99.0
Authors@R: c(
    person("Katie", "Jeynes-Cupper", email = "katie.jeynescupper@gmail.com", 
    role = c("cre","aut")),
    person("Marco", "Catoni", email = "m.catoni@bham.ac.uk", role = "aut"))
Description: The mobileRNA package allows for the identifcation of mobile small 
  RNA molecules (20-24 nt) and messenger RNAs (mRNAs) derived from one genotype 
  which have travelled across a graft junction into another genotype in a 
  hetero-graft plant system.The package includes pre-mapping steps and analysis 
  steps for sRNA-seq mRNA-seq data with the end goal of identifying mobile RNAs 
  and their potential function or implications on phenotypic changes. 
License: MIT + file LICENSE
URL: https://github.com/KJeynesCupper/mobileRNA.git,
    https://kjeynescupper.github.io/mobileRNA/
Depends:
    R (>= 4.0)
VignetteBuilder:
    knitr
Encoding: UTF-8
LazyData: true
LazyDataCompression: xz
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.3
Suggests: 
    knitr,
    rmarkdown,
    BiocStyle
Imports: 
    dplyr,
    tidyr, 
    ggplot2, 
    BiocGenerics,
    DESeq2,
    edgeR,
    Repitools,
    ggrepel,
    grDevices,
    pheatmap,
    stringr,
    utils,
    tidyselect, 
    RColorBrewer, 
    GenomicRanges,
    rtracklayer, 
    GenomeInfoDb, 
    data.table,
    SimDesign,
    scales,
    IRanges,
    stats,
    Biostrings, 
    xfun,
    magrittr
biocViews: 
    Visualization,
    RNASeq,
    Sequencing,
    SmallRNA
SystemRequirements: ShortStack (https://github.com/MikeAxtell/ShortStack)
BiocType: Workflow
vjcitn commented 1 year ago

Thanks for this submission. I see Load and organise either sRNAseq or mRNAseq results into a single dataframe containing all experimental replicates specified where rows represent either a sRNA locus or gene, respectively. -- Have you considered using SummarizedExperiment to aid in self-descriptiveness and interoperability of your package with others?

KJeynesCupper commented 1 year ago

mobileRNA is currently designed for end-user analysis, with the main aim to detect small sets of mobile RNA molecules and population-scale changes. In doing so it integrates mapping steps where it takes raw counts, RPM, Dicercall predications, and nucleic acid sequences obtained by the pre-processing sRNAseq pipeline (ShortStack). There is no benefit in the use of SummarizedExperiment objects in the package, but I have implemented a function (RNAdf2se) to generate a SummarizedExperiment object from the output of the main analysis for interoperability with other packages.

bioc-issue-bot commented 1 year ago

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

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. It is required to push a version bump to git.bioconductor.org to trigger a new build.

Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.0.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.0.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

KJeynesCupper commented 1 year ago

Thank you for your response, I have made several updates which hopefully resolve the error.

PeteHaitch commented 1 year ago

Hi @KJeynesCupper ,

Can you please try to address the remaining errors in the most recent build report. If you have any questions about them, then post back here and we will help you. Once these are resolved I will begin my review.

Thanks, Pete

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: c7ea23f9c007ba0aa5c86c387b06a6e5495ba634

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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.

The following are build products from R CMD build on the Bioconductor Build System: macOS 12.6.5 Monterey: mobileRNA_0.99.1.tar.gz Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.1.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 9c6a944bc5e4d3c3849e0d1675112cb59454a775

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.2.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.2.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 3090fcacad1ca8e72e5ee355e811f8053ed7b019

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.3.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.3.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: d056039fbfcefaa5de0de8a38fa3d7e5bf9460b0

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

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.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.4.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.4.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 43d998887825202d3b174b8c2d9e786434987ca0

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.5.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.5.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

KJeynesCupper commented 1 year ago

Hi @PeteHaitch,

I have resolved the errors and warnings that arose due to connection issues between the maintainer's email & Bioc-devel, as well as missing \value for data-set docs which had not been previously flagged.

All yours!

Cheers, Katie

PeteHaitch commented 1 year ago

Thanks, Katie!

We aim to have the initial review done within 3 weeks of having the clean build. I should be able to get it done sooner than that and I'll post back here when it's ready.

Cheer, Pete

KJeynesCupper commented 1 year ago

Cheers Pete, I look forward to hearing from you regarding the review. Many thank, Katie

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 48a6ee5eb544871db98e93392af1a27619889c63

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.6.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.6.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

KJeynesCupper commented 1 year ago

Hey @PeteHaitch ,

Just to let you know, noticed a tiny bug and fixed it - but while I was there I made some improvements to some other functions. I hope this is okay, and does not cause any issues on your end.

Looking forward to hearing from you. Many thanks, Katie

PeteHaitch commented 1 year ago

Hi @KJeynesCupper,

There are currently 3 vignettes that are all very long and seem to have some overlap in content. Could you please clarify if all 3 are supposed to be there and/or how they relate to one another.

I started my review by reading through the render HTML version of mobileRNA.Rmd vignette and I noticed a number of issues with formatting, spelling, and grammar that made it a bit hard to follow. For example:

Could you please double-check which vignettes are meant to be in the package, verify they render as you want them to, and then push an updated version to Bioconductor. It'll make my job of reviewing the package a bit easier.

Thanks, Pete

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 5ca18f250bd99013a1fb98776cc0a2e7d8fede02

KJeynesCupper commented 1 year ago

Hi @PeteHaitch,

I have made some amendments, hopefully they help. You are correct, the extra vignettes were not meant to be there, only on the devel - thank you.

Cheers, Katie

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

On one or more platforms, the build results were: "ERROR, skipped". 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.

The following are build products from R CMD build on the Bioconductor Build System: ERROR before build products produced.

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

PeteHaitch commented 1 year ago

Please take a look at the build report and try to address the errors. Please also try running R CMD build and R CMD check (or devtools::check()) on your package on your own machine after making a change to make sure the package is building and checking, correctly.

bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: edd4c0087eec2e2fea3151cb090c52da46655587

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.8.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.8.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

KJeynesCupper commented 1 year ago

Hello @PeteHaitch,

Forgive my late reply, I have been on annual leave. The error report was generated by an accidental push while making amendments in a hurry before departing abroad!

I had pushed the previously requested changes, but not as an updated version - this has been rectified.
Cheers, Katie

PeteHaitch commented 1 year ago

Hi @KJeynesCupper,

Thank you for submitting mobileRNA for consideration to Bioconductor. I've completed the checklist review and identified a number of issues that need to be addressed before mobileRNA can be accepted into Bioconductor.

In my checklist review I have separated the issues into Required and Recommended points. Would you please provide line-by-line comments to my initial review so that I know what changes I'm looking for in my re-review. If you opt to not implement a Recommended point I would ask that you please briefly address it in your response.

Cheers, Pete

Required

if (!require("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("mobileRNA")
* checking package subdirectories ... NOTE
Problems with news in NEWS.md:
No news entries found.

Recommended

x <- 99
stop("The value of x is ", x, " and that's bad.")
bioc-issue-bot commented 1 year ago

Received a valid push on git.bioconductor.org; starting a build for commit id: fc86abadc767ff25d464368013cbe2f9a3f45965

bioc-issue-bot commented 1 year ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Build System.

Congratulations! The package built without errors or warnings on all platforms.

Please see the build report for more details.

The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): mobileRNA_0.99.9.tar.gz macOS 12.6.5 Monterey: mobileRNA_0.99.9.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

KJeynesCupper commented 1 year ago

Hi @PeteHaitch,

Thank you for your review - I have made the ammendments outlined below as requested.

I look forward to hearing from you.

Cheers, Katie JC

Required

  • [x] Is mobileRNA intended as a software or a workflow package? Workflow packages "contain vignettes that describe a bioinformatics workflow that involves multiple Bioconductor packages" and have specific requirements, and usually do not include 'new' code but rather demonstrate how to use existing Bioconductor software. However, because you have written new code that wraps existing Bioconductor functionality (e.g., edgeR, DESeq2), rather than calling it directly, then it feels to me more like a software package. In the DESCRIPTION file you currently have BiocType: Workflow whereas it must be omitted entirely (or set to Software) if it is a software package). Please confirm under which package type you are submitting mobileRNA.

Comment: mobileRNA is a mixture of workflow and software. There appears to be no standarised workflow for this sort of analysis, and the workflow part is the pre-processes. This differs from previous methods, and using multiple sets of simulated data we have found this to be an improved method. This works hand-in-hand with the software-side, which is the analysis with mobileRNA. I agree, it should be a Software package, hence it has been ammended.

  • [x] The vignette is very long and detailed, but I found it a bit confusing in parts due to issues such as grammar, spelling, and the formatting. Please give the vignette a careful proofreading and read the rendered version to ensure it appears as you intend. The man pages (.Rd files) also need proofreading. If you are using RStudio then it includes a spell check function that may be helpful. Some examples of ossues I noticed were:

    • Bullet points not being rendered as intended in the vignette.
    • References to 'R-Studio' that should really just be 'R' because RStudio is a particular interface to R.
    • "The RNAmobile() function can take into account the statistically significance." Typo, I think?
    • Subsection "Step 6: Identify sRNA population difference" comes out of nowhere because there are no steps 1-5?
    • "The plotting functions within the package can be used to display these results, specifically and ." Sentence is incomplete.
    • No example to the reader of what they might do with the outputs in "Calculate RPM and Count means for specific samples". This 'Additional features' section feels like it could be better integrated into the rest of the vignette.
    • "Calcularepeats" in ?RNAfeatures".
  • [x] Upon acceptance, installation instructions (in README, vignettes, and elsewhere) must use the official Bioconductor instructions (see below). You can also include remotes::install_github() instructions if you want, but it's discouraged because it can often leads to users installing incompatible versions of Bioconductor software.
if (!require("BiocManager", quietly = TRUE))
    install.packages("BiocManager")

BiocManager::install("mobileRNA")
  • [x] The vignette includes a number of unevaluated code chunks to illustrate a hypothetical example. In particular, the 'Pre-Processing' section is entirely comprised of unevaluated, hypothetical code. Since a user can't directly run these code chunks on their system, it would be good to emphasise this at the top of such sections/chunks with a comment or something else to distinguish these from the code a user can immediately run when they are learning how to use the package.

Comment: A small paragraph has been added to the section "Pre-Processing"

  • [x] The (unevaluated) code chunk in 'Step 6: Identify sRNA population difference' of the vignette doesn't work when run on a user's system because sRNA_edgeR hasn't been created or loaded. This chunk should be evaluated (see above) and the sRNA_edgeR created/loaded in an evaluated chunk prior to running this code to ensure it works on a user's system.

Comment: Thank you for pointing out this error - this has been amened to the correct pre-existing data set

  • [x] The man directory includes .Rd files for .fa and .gff files that don't themselves seem to be actually used or even included in the package (e.g., system.file("extdata", "chr12_Eggplant_V4.1.fa.gz", package = "mobileRNA") returns ""). Moreover, the documentation seems likely to be incorrect (e.g., Value: Dataframe in global environment.). It seems like the roxygen2 markup for these files (in R/data.R) should just be removed, along with the .Rd files themselves by re-reoxygenise()-ing the package.

Comment: Thank you, I am sorry this slipped through the cracks. I had initally added the files in an "extdata" folder, but they exceaded the required size and size of package, and so ease were add to a public repo. I have removed the man files and data files for these.

  • [x] The documentation of sRNA_data seems to be out-of-sync with data. For example, the documentation references a sample_table function (what's that?) and refers to TomEgg_1, ..., TomTom_3 but that's not how columns are named in the sRNA_data data.

Comment: Thank you for your comment, this has been fixed.

  • [x] RNAimport(input = "mRNA") is not actually implemented as far as I can tell. Reading the code (because there was no example data for me to test on), it seems it would a message saying "This features is under development, and will be available soon.\nPlease see the devel branch on github (https://github.com/KJeynesCupper/mobileRNA/tree/devel)." but it should probably return an error or simply not be an option since it's not yet implemented.

Comment: Thank you for your comment, as you can guess we are developing it for the analysis of mobile mRNAs too at somepoint. I have removed this option from the parameter choices. But retained the input parameter for smooth intergration later on.

  • [x] Please use file.path() instead of paste0() to construct file paths because this works across all platforms. E.g., paste0(directory, file, "/Results.txt") should use file.path(directory, file, "Results.txt").
  • [x] Please use message() rather than cat() for messages. This enables a user to choose to suppress or redirect these messages; see https://contributions.bioconductor.org/r-code.html#end-user-messages.
  • [x] Examples must not write to a users home directory, working directory, or installed package directory. Instead, please use tempdir() or tempfile(). For example, the example for RNAmergeAnnotations must use tempdir() for the out_dir, and you would probably also then document that users will want to change the value of out_dir for real applications.
  • [x] All parallelised functions must use a minimal number of cores (1 or 2) by default; e.g., RNAmergeGenomes() currently has number_cores = 4, which is not permitted.
  • [x] Please add a BugReports field to the DESCRIPTION (see https://contributions.bioconductor.org/description.html#description-bugreport).
  • [x] The NEWS.md file is not properly formatted because running news(package = "mobileRNA") returns NULL (see help("news", "utils") and https://contributions.bioconductor.org/news.html for how this should be formatted). This is also flagged by running R CMD check on the package, albeit not obviously, with the note:
* checking package subdirectories ... NOTE
Problems with news in NEWS.md:
No news entries found.

Recommended

  • [ ] It is strongly recommended that a package includes unit tests covering a large part of core functionality (see https://contributions.bioconductor.org/tests.html) and currently mobileRNA currently has none. A useful function when implementing tests is covr::report() which will produce a report that highlights lines of code that are/aren't being tested.

Comment: Throughout the development, the package was tested with different data sets. I aim to implement unit tests at some point as I think they would be valuable.

Comment: This has been implemented in RNAmergeGenomes()

  • [x] There's no need for paste() within stop(). E.g., you can instead directly write things like stop("The value of x is ", 10, " and that's bad.") and
x <- 99
stop("The value of x is ", x, " and that's bad.")

Comment: paste() has been removed from all stop commands.

Comment: LazyData: FALSE has been set !

  • [x] RNAmergeAnnotations() could be made to work directly with GFF files. I.e. by doing the rtracklayrer::import() within the function itself rather than requiring the user to first import() the data and pass the results to the function. I was confused by this when reading the documentation, thinking the user had to supply the paths to the GFFs rather than have already imported these as GRanges objects.

Comment: Thank you for your comment, I also agree. It has now been included inside the function.

  • [x] Some of the function names aren't all as clear or precise as they could be. For example, RNAanalysis() is rather vague whereas plotSamplePCA() has a more descriptive name. The widespread use of RNA*() prefix for function names is perhaps also unnecessary.

Comment: The use of RNA*() prefix for function names was simply utilised to make the package functions cohesive and easy to implement while coding - particularly for individually with varying skill levels. I have ammended the RNAanalysis to RNAdifferentialAnalaysis as I agree that that particular function name could be confusing.

[ ] I'll preface this to emphasise that this is a only comment/suggestion and not a requirement, but I must echo @vjcitn suggestion that a SummarizedExperiment interface is much more Bioconductor-like, simplifies interoperability with other Bioconductor software, and felt much simpler and tidier to me than the data.frame-first approach. Admittedly, I'm very experienced with R and Bioconductor, but I found the result of mobileRNA::RNAdf2se("sRNA", sRNA_data) much simpler to reason about and manipulate than sRNA_data.

Comment: The package was built for users with varying ability, and with that in mind I believed it was benefial for users to be able to easily scan and manually/visually compare data between samples and conditions, and observe the additional data generated for each sRNA cluster as you work through the steps. Moreover, the aims underpinning some of the functions within mobileRNA, to me at least, suited the the use of a dataframe over a SummarizedExperiment object.

  • [x] Running BiocCheck::BiocCheck() on the package produces 19 NOTES. Please take a look at the results and consider addressing these. I have highlighted elsewhere in my review those that I consider most important to address whereas things about code formating/styling are more about personal preference.

Comment: Added ExperimentalDesign & QualityControl to biocViews in the DESCRIPTION file, included seq_along/seq_len where possible. Reduced line lenghts, typically caused by stop() or message() lines.

  • [x] I'm not sure it's strictly needed to include ShortStack in the SystemRequirements field. The mobileRNA code and documentation don't actually require ShortStack to be installed because all of the examples of its usage are hypothetical and are not runnable by a user.

Comment: Removed ShortStack from the SystemRequirements field.

  • [x] The URL field in the DESCRIPTION is probably not needed and can be omitted. When published by Bioconductor, you will get the canonical URL https://bioconductor.org/packages/mobileRNA/ and https://git.bioconductor.org/packages/mobileRNA should probably become the 'canonical' source code repository.

Comment: Removed URL from the DESCRIPTION file.

PeteHaitch commented 1 year ago

Hi @KJeynesCupper,

Thanks for clarifying the intention of mobileRNA as a software package and making an effort to address the feedback from the initial review. Unfortunately, there still remains work to be done to make the package suitable for Bioconductor.

My overarching concern is that there still a number of cases where there are mistakes in the code, inconsistencies between the code and the documentation, and unclear documentation. These problems are exacerbated by there being so many unevaluated or hypothetical code examples in the man pages and vignette, and the absence of any tests (as raised in the initial review). When I've attempted to adapt some of these hypothetical examples to try the package, I've found I've encountered many problems that have prevented me from really being able to use the package.

I know you've tested the software with example datasets while developing the package, but such tests really need to be part of the software (as runnable examples in the man pages, as evaluated code chunks in the vignette, and as unit tests) to verify that it's working correctly and to properly demonstrate its functionality.

Do you have any colleagues who could try using the package as a 'new user' to get feedback from them? That type of review and feedback is beyond what Bioconductor reviews can or are intended to provide and so unfortunately I really can't volunteer this amount of time to working through these issues that really ought to precede a package being submitted for review[^2]. I don't mean to be discouraging - you've clearly put a lot of work into this and I think mobileRNA can be software that is useful to people and suitable for Bioconductor - so I hope my comments don't come across as too negative.

[^2]: I've already spent more than 10 hours on this review

I'll be on leave (and offline) from today until October 9. The deadline for new packages to be accepted is October 18 (see https://www.bioconductor.org/developers/release-schedule/). If while I'm away you do have any urgent concerns or questions, please post to this issue and tag @vjcitn or @lshep to bring it to their attention, but if it's not urgent then there's no need to tag them and I'll respond when I'm back.

Cheers, Pete

Required

# This code is from RNAmergeGenomes(), the comments are mine.

# If genomeA is a single file then there's no need to loop over anything (let 
# alone use parallelisation for that loop) when reading.
# If `genomeA` could/should be directory, then the code won't work because you 
# never extract the relevant files from that directory for reading.
# If genomeA is multiples files then this code _may_ benefit from 
# parallelisation but that is not what is documented.
ref1_fragments <- BiocParallel::bplapply(genomeA, function(file) {
  Biostrings::readDNAStringSet(file, format = "fasta")
}, BPPARAM = BPPARAM)

Another example from the same function, here where parallelisation is not correctly implemented:

# This code is also from RNAmergeGenomes(), the comments are mine.

# This loops over the number of cores for some reason?
# Furthermore, it writes (in parallel) the same data (`merged_genome`) to 
# `number_cores` 'temporary' files.
BiocParallel::bplapply(1:number_cores, function(i) {
  Biostrings::writeXStringSet(merged_genome,
                              file = paste0(dirname(out_dir), "/tempfile_", i,
                                            ".fa"),
                              format = "fasta")
}, BPPARAM = BPPARAM)
# Amended example for RNAmergeAnnotations()

# Initialize a cache directory, using the BiocFileCache package, to store the 
# downloads used in this example
library(BiocFileCache)
cache_dir <- tools::R_user_dir("mobileRNA", which = "cache")
cache <- BiocFileCache(cache_dir)

# Construct URL to example GFF files
url_remote <- "https://github.com/KJeynesCupper/assemblies/raw/main/"
anno1_url <- file.path(url_remote,"chr12_Eggplant_V4.1_function_IPR_final.gff")
anno2_url <- file.path(url_remote, "chr2_ITAG4.0_gene_models.gff")

# Download example GFF files and add them to cache
anno1 <- bfcrpath(cache, anno1_url)
anno2 <- bfcrpath(cache, anno2_url)

# Merge annotations and write them to a file in out_dir.
# For this example, the result is written to a temporary file.
# For real use cases, the out_dir should be an appropriate location on your 
# computer.
out_dir <- tempfile("merged_annotation", fileext = ".gff3")
merged_anno <- RNAmergeAnnotations(
  annotationA = anno1,
  annotationB = anno2,
  out_dir = out_dir)
# Amended example for RNAmergeGenomes()

# Initialize a cache directory, using the BiocFileCache package, to store the 
# downloads used in this example
library(BiocFileCache)
cache_dir <- tools::R_user_dir("mobileRNA", which = "cache")
cache <- BiocFileCache(cache_dir)

# Construct URL to example FASTA files
url_remote <- "https://github.com/KJeynesCupper/assemblies/raw/main/"
fasta_1_url <- file.path(url_remote, "chr12_Eggplant_V4.1.fa.gz")
fasta_2_url <- file.path(url_remote,"chr2_S_lycopersicum_chromosomes.4.00.fa.gz")

# Download example FASTA files and add them to cache
fasta_1 <- bfcrpath(cache, fasta_1_url)
fasta_2 <- bfcrpath(cache, fasta_2_url)

# Merge FASTA files and write them to a file in out_dir.
# For this example, the result is written to a temporary file.
# For real use cases, the out_dir should be an appropriate location on your 
# computer.
out_dir <- tempfile("merged_annotation", fileext = ".fa")
merged_ref <- RNAmergeGenomes(
  genomeA = fasta_1, 
  genomeB = fasta_2,
  out_dir = out_dir,
  cores = FALSE, 
  number_cores = 1)

[^1]: "It is not allowed to download or write any files to a users home directory, working directory, or installed package directory." (https://contributions.bioconductor.org/r-code.html#web-querying-and-file-caching)

# Should be `!grepl("\\.gff$", out_dir)
if (base::missing(out_dir) || grepl("\\.gff$", out_dir)) {
        stop("Please specify out_dir, a connection to a local directory to write and \n          save merged annotation. \n Ensure file name with extension (GFF) is \n          supplied.")
# Rest of roxygen2 documentation
#' @importFrom BiocParallel SerialParam
RNAmergeGenomes <- function(genomeA, genomeB, out_dir, 
                            abbreviationGenomeA = "A",
                            abbreviationGenomeB = "B",
                            BPPARAM = SerialParam()) { 
  # Rest of function
}

Then, a user would provide a BiocParallelParam object to select their desired form of parallelisation and their code would become something like this

# Someone on a non-Windows machine with at least 4 cores might use
merged_ref <- RNAmergeGenomes(
  genomeA = genomeA 
  genomeB = genomeB,
  out_dir = out_dir, 
  BPPARAM = BiocParallel::MulticoreParam(4))

# Someone on a Windows machine with at least 3 cores might use
merged_ref <- RNAmergeGenomes(
  genomeA = genomeA 
  genomeB = genomeB,
  out_dir = out_dir, 
  BPPARAM = BiocParallel::SnowParam(3))

More advanced users might provide additional parameters in their call to MulticoreParam() or SnowParam().

news(package = "mobileRNA")
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.0 (2023-06-19)
#>       Amendments for Bioconductor
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.0 (2023-07-15)
#>       RNAconsensus changed to RNAdicercall
#>       Alterations to RNAdicercall algorithm, including tie options, altered default tidy method. 
#>       RNAdicercall introduced new column "DicerCount" 
#>       RNAmobile introduced new parameter, "threshold"
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.0 (2023-07-16)
#>       Improved RNAsequences selection algorithm to consider a threshold value, and handling ties.
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.0 (2023-07-19)
#>       Improved error calling on functions
#>       Found error in RNAattributes
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.1 (2023-08-01)
#>       Added RNAdf2e function
#>       Improvements to plotSamplePCA and plotHeatmap
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.1 (2023-08-07) 
#>       Amended RNAmergeAnnotations function to meet requirements
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.2 (2023-08-07)
#>       Contained incorrect maintainer email address
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.3 (2023-08-07)
#>       Bioc-maintainer checks
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.4 (2023-08-07)
#>       Bioc-maintainer checks
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.5 (2023-08-07)
#>       Updated man files for datasets, missing /values.
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.6 (2023-08-15)
#>       Fixed bug in RNAdistribution plot, when sample specific
#>       Improved PCA plot flexibility
#>       Broadened use of RNAattributes function.
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.7 (2023-08-25)
#>       Updated vignette
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.8 (2023-09-05)
#>       Updated vignette
#>       Updated RNAdicercall to allow any dicer-classification (not constricted to 20-24)
#>       Amended RNA distribution to suit. 
#>       Amended plotSamplePCA table and plot
#> Warning: Cannot process chunk/lines:
#>   Version: 0.99.9 (2023-09-23)
#>       Made adjusted required by Bioconductor including:
#>       Improvement to vignette (spelling, general corrections)
#>       Converted cat() to message() for user information from functions 
#>       Amended example data set and docs
#>       Amended CITATION and NEWs file
#>       Altered examples in RNAmergeAnnotations/Genomes functions to prevent examples saving into users directory
#>       Improved the functionality of RNAdicercall, specifically the `exclude` parameter.
#> Version: 0.99.0
#> Date: 2023-06-19
#> Text: Amendments for Bioconductor
#> 
#> Version: 0.99.0
#> Date: 2023-07-15
#> Text: RNAconsensus changed to RNAdicercall Alterations to RNAdicercall
#>         algorithm, including tie options, altered default tidy method.
#>         RNAdicercall introduced new column "DicerCount" RNAmobile
#>         introduced new parameter, "threshold"
#> 
#> Version: 0.99.0
#> Date: 2023-07-16
#> Text: Improved RNAsequences selection algorithm to consider a threshold
#>         value, and handling ties.
#> 
#> Version: 0.99.0
#> Date: 2023-07-19
#> Text: Improved error calling on functions Found error in RNAattributes
#> 
#> Version: 0.99.1
#> Date: 2023-08-01
#> Text: Added RNAdf2e function Improvements to plotSamplePCA and
#>         plotHeatmap
#> 
#> Version: 0.99.1
#> Date: 2023-08-07
#> Text: Amended RNAmergeAnnotations function to meet requirements
#> 
#> Version: 0.99.2
#> Date: 2023-08-07
#> Text: Contained incorrect maintainer email address
#> 
#> Version: 0.99.3
#> Date: 2023-08-07
#> Text: Bioc-maintainer checks
#> 
#> Version: 0.99.4
#> Date: 2023-08-07
#> Text: Bioc-maintainer checks
#> 
#> Version: 0.99.5
#> Date: 2023-08-07
#> Text: Updated man files for datasets, missing /values.
#> 
#> Version: 0.99.6
#> Date: 2023-08-15
#> Text: Fixed bug in RNAdistribution plot, when sample specific Improved
#>         PCA plot flexibility Broadened use of RNAattributes function.
#> 
#> Version: 0.99.7
#> Date: 2023-08-25
#> Text: Updated vignette
#> 
#> Version: 0.99.8
#> Date: 2023-09-05
#> Text: Updated vignette Updated RNAdicercall to allow any
#>         dicer-classification (not constricted to 20-24) Amended RNA
#>         distribution to suit.  Amended plotSamplePCA table and plot
#> 
#> Version: 0.99.9
#> Date: 2023-09-23
#> Text: Made adjusted required by Bioconductor including: Improvement to
#>         vignette (spelling, general corrections) Converted cat() to
#>         message() for user information from functions Amended example
#>         data set and docs Amended CITATION and NEWs file Altered
#>         examples in RNAmergeAnnotations/Genomes functions to prevent
#>         examples saving into users directory Improved the functionality
#>         of RNAdicercall, specifically the `exclude` parameter.
Session info ``` r sessioninfo::session_info() #> ─ Session info ─────────────────────────────────────────────────────────────── #> setting value #> version R version 4.3.1 (2023-06-16) #> os macOS Ventura 13.6 #> system aarch64, darwin20 #> ui X11 #> language (EN) #> collate en_US.UTF-8 #> ctype en_US.UTF-8 #> tz Australia/Melbourne #> date 2023-09-29 #> pandoc 3.1.1 @ /Applications/RStudio.app/Contents/Resources/app/quarto/bin/tools/ (via rmarkdown) #> #> ─ Packages ─────────────────────────────────────────────────────────────────── #> package * version date (UTC) lib source #> cli 3.6.1 2023-03-23 [1] CRAN (R 4.3.0) #> digest 0.6.33 2023-07-07 [1] CRAN (R 4.3.0) #> evaluate 0.21 2023-05-05 [1] CRAN (R 4.3.0) #> fastmap 1.1.1 2023-02-24 [1] CRAN (R 4.3.0) #> fs 1.6.3 2023-07-20 [1] CRAN (R 4.3.0) #> glue 1.6.2 2022-02-24 [1] CRAN (R 4.3.0) #> htmltools 0.5.6 2023-08-10 [1] CRAN (R 4.3.0) #> knitr 1.44 2023-09-11 [1] CRAN (R 4.3.1) #> lifecycle 1.0.3 2022-10-07 [1] CRAN (R 4.3.0) #> reprex 2.0.2 2022-08-17 [1] CRAN (R 4.3.0) #> rlang 1.1.1 2023-04-28 [1] CRAN (R 4.3.0) #> rmarkdown 2.25 2023-09-18 [1] CRAN (R 4.3.1) #> rstudioapi 0.15.0 2023-07-07 [1] CRAN (R 4.3.0) #> sessioninfo 1.2.2 2021-12-06 [1] CRAN (R 4.3.0) #> withr 2.5.1 2023-09-26 [1] CRAN (R 4.3.1) #> xfun 0.40 2023-08-09 [1] CRAN (R 4.3.0) #> yaml 2.3.7 2023-01-23 [1] CRAN (R 4.3.0) #> #> [1] /Library/Frameworks/R.framework/Versions/4.3-arm64/Resources/library #> #> ────────────────────────────────────────────────────────────────────────────── ```
mobile_df_features <- RNAfeatures(data = mobile_sRNA,
                            annotation = "./annotation/merged_annotation.gff3", 
                      repeats = "./annotation/merged_annotation.gff3")

Recommended

citation('mobileRNA')
#> To cite package 'mobileRNA' in publications use:
#> 
#>   Katie , Marco Catoni. mobileRNA: Explore candidate mobile sRNAs &
#>   sRNAs population-scale changes
#> 
#> A BibTeX entry for LaTeX users is
#> 
#>   @Unpublished{,
#>     author = {{Jeynes-Cupper} and {Katie} and {Catoni} and {Marco}},
#>     title = {mobileRNA: Explore candidate mobile sRNAs & sRNAs population-scale changes},
#>     note = {Yet to be published},
#>   }
PeteHaitch commented 11 months ago

Hi @KJeynesCupper,

I'm checking to see if you intend to continue this submission? It's no problem if you're too busy at the moment, but please let us know if that's the case. If there's no response by the end of the week, I'll close this issue until you have further updates (we can re-open this issue and continue the submission if that's what you want to do).

Cheers, Pete

KJeynesCupper commented 11 months ago

Hi @PeteHaitch,

I am sorry for the delayed response following your comments; I do intend to continue the submission and have made alterations which hopefully meet your requirements. Recently I have had to prioritise another project, however, I will look to complete all the checks and resubmit soon.

Thank you for your continued support!

Katie

PeteHaitch commented 11 months ago

You're welcome. I'll close this issue for now, but you can re-open it by posting back here when you're ready to continue.

bioc-issue-bot commented 11 months 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 your interest in Bioconductor.

KJeynesCupper commented 9 months ago

Hello @PeteHaitch,

Sorry for the delay on my behalf, I have now made a number of changes to mobileRNA and before re-submitting, I have a question:

The changes have tried to add clarity surrounding the alignment steps mentioned previously. This includes an additional function called mapRNA to work as an interface for the alignment steps to make the process more understandable, and have included the raw steps in the appendix. In hand with this, I have included FASTQ files and reduced FASTA & GFFs (stored in extdata directory) to be used as examples for users. That said, the overall source package occupies ~10.8Mb (doc=3.1Mb, extdata=5.4Mb, help=1.4Mb) which is double the recommended size - is there any flexibility in allowing this larger source package size?

Many thanks, Katie

PeteHaitch commented 9 months ago

Hi @KJeynesCupper,

Have you tried compressing the FASTQ and GFF files? That might make them small enough to sneak under the file size cutoff.

Otherwise, it might be suitable to upload this data to ExperimentHub rather than include the data in the package itself; see https://contributions.bioconductor.org/data.html Cheers, Pete

KJeynesCupper commented 9 months ago

Dear @PeteHaitch,

Thank you for your support, I have resolved the issues and would like to resubmit. Is it best that I submit a new submission, as this session is now closed? Or is it possible to reopen this submission?

I have pushed the latest update to Bioconductor - but, with the number of revisions I made previously, it has reached version 0.99.9.

Thank you for your continual support on this submission, and I appreciate the dedicated time you have put in.

Look forward to hearing from you, Katie

bioc-issue-bot commented 9 months ago

Dear @KJeynesCupper ,

We have reopened the issue to continue the review process. Please remember to push a version bump to git.bioconductor.org to trigger a new build.

bioc-issue-bot commented 9 months ago

A reviewer has been assigned to your package for an indepth review. Please respond accordingly to any further comments from the reviewer.

lshep commented 9 months ago

I reopened the issue so you should be able to push now; 0.99.10, 0.99.11, 0.99.12 ... you can keep increasing the z of version x.y.z and it is still valid.

bioc-issue-bot commented 9 months ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 7ba2c7be49788bf05abab45c2ba981991ccd1340

bioc-issue-bot commented 9 months ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Single Package Builder.

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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: mobileRNA_0.99.10.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 9 months ago

Received a valid push on git.bioconductor.org; starting a build for commit id: 7dc4085eeac2b4dbf542c316c0f674e02946ec47

bioc-issue-bot commented 9 months ago

Dear Package contributor,

This is the automated single package builder at bioconductor.org.

Your package has been built on the Bioconductor Single Package Builder.

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.

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: mobileRNA_0.99.11.tar.gz

Links above active for 21 days.

Remember: if you submitted your package after July 7th, 2020, when making changes to your repository push to git@git.bioconductor.org:packages/mobileRNA to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.