Contributions icon indicating copy to clipboard operation
Contributions copied to clipboard

SplineDV

Open Xenon8778 opened this issue 1 year ago • 31 comments
trafficstars

Hello Bioconductor team, I am submitting my R package {SplineDV} for consideration for Bioconductor release.

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

  • Repository: https://github.com/Xenon8778/SplineDV

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.

Xenon8778 avatar Jul 12 '24 17:07 Xenon8778

Hi @Xenon8778

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: SplineDV
Type: Package
Title: Differential Variability (DV) analysis for single-cell RNA
        sequencing data. (e.g. Identify Differentially Variable Genes
        across two experimental conditions)
Description: A spline based scRNA-seq method for identifying differentially variable (DV) genes across two experimental conditions. Spline-DV constructs a 3D spline from 3 key gene statistics: mean expression, coefficient of variance, and dropout rate. This is done for both conditons. The 3D spline provides the “expected” behavior of genes in each condition.The distance of the observed mean, CV and dropout rate of each gene from the expected 3D spline is used to measure variability. As the final step, the spline-DV method compares the variabilities of each condition to identify differentially variable (DV) genes.
Version: 0.99.0
Authors@R: c(person(given = "Shreyan", family = "Gupta", email = "[email protected]", role = c("aut","cre"), comment = c(ORCID = '0000-0002-1904-9862')),
person(given = "James", family = "Cai", role = "aut", comment = c(ORCID = '0000-0002-8081-6725')),
person(given = "Selim", family = "Romero", role = "aut", comment = c(ORCID = '0000-0002-2401-6272'))
)
Maintainer: Shreyan Gupta <[email protected]>
BugReports: https://github.com/Xenon8778/SplineDV/issues
URL: https://github.com/Xenon8778/SplineDV
License: GPL (>=2)
Encoding: UTF-8
LazyData: false
biocViews: Sequencing, SingleCell, Software, SystemsBiology,
        DifferentialExpression, RNASeq, FunctionalGenomics,
        GeneExpression, Transcription, Transcriptomics,
        FeatureExtraction
Imports: plotly, dplyr, Seurat, methods, Biobase, sparseMatrixStats,
        Matrix (>= 1.6.4), utils
Suggests: knitr, rmarkdown
VignetteBuilder: knitr
RoxygenNote: 7.3.1
NeedsCompilation: no
Packaged: 2024-07-12 16:58:19 UTC; xenon8778
Author: Shreyan Gupta [aut, cre] (<https://orcid.org/0000-0002-1904-9862>),
  James Cai [aut] (<https://orcid.org/0000-0002-8081-6725>),
  Selim Romero [aut] (<https://orcid.org/0000-0002-2401-6272>)

bioc-issue-bot avatar Jul 12 '24 17:07 bioc-issue-bot

  • [ ] Please remove inst/doc these files should be generated automatically with R CMD build. Also please remove vignettes/SplineDV.nb.html .

  • [ ] If you are working with Single cell data, your package should utilize SingleCellExperiment objects. SingleCellExperiment is the main class structure utilized in Bioconductor.

lshep avatar Jul 17 '24 19:07 lshep

Hi,

Thanks for your suggestions, I have removed the inst/doc directory and the removed vignettes/SplineDV.nb.html. The package now uses SingleCellExperiment objects instead of Seurat.

Thanks

Xenon8778 avatar Jul 18 '24 02:07 Xenon8778

  • [x] Please remove inst/doc these files should be generated automatically with R CMD build. Also please remove vignettes/SplineDV.nb.html .
  • [x] If you are working with Single cell data, your package should utilize SingleCellExperiment objects. SingleCellExperiment is the main class structure utilized in Bioconductor.

Xenon8778 avatar Jul 18 '24 02:07 Xenon8778

Please do not read data in that is solely hosted on github. The data (or an acceptable smaller subset if large) should be included in the package to use for examples and vignettes.

lshep avatar Aug 05 '24 16:08 lshep

Hi,

Thanks for the suggestion. The package now reads smaller data subsets ('.rda' files) stored in the data/ directory for vignettes and examples.

Thanks

Xenon8778 avatar Aug 05 '24 22:08 Xenon8778

The vignette is still referencing Seurat but we can move it forward to test on the build system. When a reviewer is assigned they will be looking for documentation updates.

lshep avatar Aug 06 '24 12:08 lshep

Your package has been added to git.bioconductor.org to continue the pre-review process. A build report will be posted shortly. Please fix any ERROR and WARNING in the build report before a reviewer is assigned or provide a justification on why you feel the ERROR or WARNING should be granted an exception.

IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. All changes should be pushed to git.bioconductor.org moving forward. It is required to push a version bump to git.bioconductor.org to trigger a new build report.

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 avatar Aug 06 '24 12:08 bioc-issue-bot

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: Linux (Ubuntu 22.04.3 LTS): SplineDV_0.99.0.tar.gz macOS 12.7.1 Monterey: SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 06 '24 12:08 bioc-issue-bot

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

bioc-issue-bot avatar Aug 06 '24 18:08 bioc-issue-bot

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

The following are build products from R CMD build on the Single Package Builder: macOS 12.7.1 Monterey: SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 06 '24 19:08 bioc-issue-bot

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

bioc-issue-bot avatar Aug 08 '24 21:08 bioc-issue-bot

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: "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 Single Package Builder: Linux (Ubuntu 22.04.3 LTS): SplineDV_0.99.2.tar.gz macOS 12.7.1 Monterey: SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 08 '24 21:08 bioc-issue-bot

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

bioc-issue-bot avatar Aug 08 '24 21:08 bioc-issue-bot

Dear Package contributor,

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

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

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 Single Package Builder: Linux (Ubuntu 22.04.3 LTS): SplineDV_0.99.3.tar.gz macOS 12.7.1 Monterey: SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Aug 08 '24 22:08 bioc-issue-bot

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

bioc-issue-bot avatar Aug 09 '24 12:08 bioc-issue-bot

  • Vignette needs to have an Installation section. See Installation.
  • The vignette uses eval = FALSE. No code chunks are allowed to use eval = FALSE. Please see Vignettes.
  • Don't use example data from Seurat. Please consider scRNAseq or ExperimentHub.
  • Vignette defines a plotting function getdensity. Should be a documented function in the package.
  • Use spaces around binary operators. For example, CV <- SDs/Means does not. See Use of Space in R Code.
  • Don't use for loops and append to empty lists but vectorise with mapply. See Vectorize.
    Dist_HVG <- c()
    dvecx <- c()
    dvecy <- c()
    dvecz <- c()
    nearidx <- c()
    df <- as.matrix(xyz1)
    pb <- txtProgressBar(min = 0, max = nrow(xyz), initial = 0, style = 3)
    for (i in 1:nrow(xyz)){
  • Inconsistent variable naming style. For example, is.mito, qc_df. Bioconductor requires camelCase for consistency.

  • Result floods user's R console with output.

load(system.file("extdata", "WT_count.rda", package = "SplineDV")) # WT Sample
load(system.file("extdata", "KO_count.rda", package = "SplineDV")) # KO Sample
DV_res <- DV_splinefit(X = KO_count, Y = WT_count)

image

  • Result is claimed to be DataFrame type. It is not. It is data.frame. Please make it DataFrame.
\value{
A DataFrame with DV Statistics
}
> class(DV_res)
  "data.frame"
  • Package lacks unit tests. Please read Chapter 14: Unit Tests and add some meaningful correctness tests.
  • Don't stop execution on a warning. You probably meant to write stop("Sample has too few genes."). You wrote:
    warning('Sample has too few genes')
    stop()

DarioS avatar Sep 04 '24 02:09 DarioS

Hi @DarioS ,

Thank you for your review and comments! I had a few questions about the following suggestions -

  1. The vignette already has an installation section. This section is "eval=False" as mentioned in the Package manual. See [Installation (https://contributions.bioconductor.org/docs.html#installation). Do I need to make any changes here? image

  2. No part in the vignette uses example data in the vignette. The file paths in the vignette are just dummy paths, hence these sections were kept eval=False. I just wanted to show users how to load data from Seurat in the vignette. Where should I put that in the vignette?

I am working on the other issue and will soon update the package. Thanks once again.

Xenon8778 avatar Sep 16 '24 16:09 Xenon8778

Yes, should be

```
BiocManager::install("Xenon8778/SplineDV")
```

See Data

When developing a software package, an excellent practice is to give a comprehensive illustration of the methods in the package using an existing experiment data package, annotation data or data in the ExperimentHub or AnnotationHub, or submitting new data to those resources yourself.

DarioS avatar Sep 17 '24 06:09 DarioS

Thank you. I'll incorporate the changes.

Xenon8778 avatar Sep 17 '24 13:09 Xenon8778

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

bioc-issue-bot avatar Sep 18 '24 17:09 bioc-issue-bot

Dear Package contributor,

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

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

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 Single Package Builder: Linux (Ubuntu 22.04.3 LTS): SplineDV_0.99.4.tar.gz Linux (Ubuntu 24.04.1 LTS): SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Sep 18 '24 17:09 bioc-issue-bot

Hi @DarioS,

The issues have been fixed-

  • Vignette has an installation section. image

  • No code chunks use eval=False.

  • Examples from Seurat have been removed. The vignette uses only example data within the package.

  • The plotting function is documented in the package.

  • Spaces fixed.

  • lapply is used instead of for loop

  • All variable names with "." have been changed to one naming style that uses "_"

  • Can't replicate "Result floods user's R console with output." Seems like you were printing the output data.frame instead of assigning it to a variable. image

  • Unit tests included and Citation included.

  • Executions now stop with stop()

Thanks! Hope to hear from you soon.

Xenon8778 avatar Sep 18 '24 17:09 Xenon8778

  • You converted the naming style to snake_case, not to camelCase.
  • Results in the vignette don't have any explanation. Every plot or table should have a couple of sentences explaining what information the result reveals.
  • If you vary parameters from default, explain why they should have a different value. For example,
HVG_res <- HVG_splinefit(WT_count, nHVGs = 20, ncells = 3, ncounts = 200)

has no explanation why defaults are not used but different numbers are.

  • Any data sets included in the package must have documentation pages in man folder. See Data.

DarioS avatar Sep 21 '24 12:09 DarioS

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

bioc-issue-bot avatar Oct 10 '24 19:10 bioc-issue-bot

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: Linux (Ubuntu 24.04.1 LTS): SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Oct 10 '24 20:10 bioc-issue-bot

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

bioc-issue-bot avatar Oct 10 '24 20:10 bioc-issue-bot

Dear Package contributor,

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

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

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 Single Package Builder: Linux (Ubuntu 24.04.1 LTS): SplineDV_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 [email protected]:packages/SplineDV to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot avatar Oct 10 '24 20:10 bioc-issue-bot

Hi @DarioS,

The issues have been fixed-

  1. Naming style for objects has been converted to camelCase.
  2. Results in the vignette describe every column in output data.frame and plot have a description now.
  3. Parameter change has been justified in the vignette.
  4. Datasets have been added to /data and package and vignette calls it from there.

Thanks for the suggestions! I look forward to hearing from you soon.

Xenon8778 avatar Oct 10 '24 20:10 Xenon8778

  • Example data loading needs to be realistic, using R functions a user would use to load their own data set.
WTcount <- SplineDV::WTcount
KOcount <- SplineDV::KOcount

is not acceptable. See LazyData section of developer's guide.

  • There are plenty of single-cell RNA-seq data sets in Bioconductor already. Justify why you need a new one.

  • Function documentation is often too brief and vague. For example,

\value{
A data.frame with DV Statistics
}
\description{
Differential Variability analysis
}

Each description section should be at least a couple of sentences long and informative.

  • Vignette states what the columns contain. This should be in the Rd files, not the vignette. For example,

The splineDV result data.frame contains the following data: genes - Contains the name of the gene. mu1 - log1p(mean) expression of gene in test sample. mu2 - log1p(mean) expression of gene in control sample. CV1 - log1p(CV) expression of gene in test sample. CV2 - log1p(CV) expression of gene in control sample. drop1 - Dropout rate of gene in test sample. drop2 - Dropout rate of gene in control sample. dist1 - Distance of real gene statistics from nearest point on spline in test sample. dist2 - Distance of real gene statistics from nearest point on spline in control sample.

Vignette should contain a case study and biological interpretation of the output. It still lacks interpretation. For example,

fig <- DVPlot(dvRes)
fig
# Then no interpretation but simply a new section titled Highly Variable Genes (HVGs) Using Spline-HVG.
  • Bioconductor has DataFrame in S4Vectors for tabular data but SplineDV relies on data.frame.
> dvRes <- DataFrame(dvRes)
> DVPlot(dvRes)
Error in vapply(all_objects, nrow, integer(1L), USE.NAMES = FALSE) :
  values must be length 1,
 but FUN(X[[2]]) result is length 0

Please improve interoperability with standard Bioconductor classes and read Common Classes section.

DarioS avatar Oct 14 '24 04:10 DarioS