ExperimentSubset - Githubissues

irzamsarfraz commented 4 years ago

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

Repository: https://github.com/campbio/ExperimentSubset

Confirm the following by editing each check box to '[x]'

[x] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] I understand that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.

I am familiar with the essential aspects of Bioconductor software management, including:

[x] The 'devel' branch for new packages and features.
[x] The stable 'release' branch, made available every six months, for bug fixes.
[x] Bioconductor version control using Git (optionally via GitHub).

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

bioc-issue-bot commented 4 years ago

Hi @irzamsarfraz

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: ExperimentSubset
Type: Package
Title: Manages subsets of data with Bioconductor Experiment objects
Version: 0.99.0
Authors@R: c(person(given="Irzam", family="Sarfraz", email="irzam9095@gmail.com", role=c("aut"),
    comment = c(ORCID = "0000-0001-8121-792X")),
  person(given="Muhammad", family="Asif", email="asif@ntu.edu.pk", role=c("aut","ths"),
    comment = c(ORCID = "0000-0003-1839-2527")),
  person(given=c("Joshua","D."), family="Campbell", email="camp@bu.edu", role=c("aut","cre"),
         comment = c(ORCID = "0000-0003-0780-8662")))
Description: Experiment objects such as the SummarizedExperiment or SingleCellExperiment are data containers for one or more matrix-like assays along with the associated row and column data. Often only a subset of the original data is needed for down-stream analysis. For example, filtering out poor quality samples will require excluding some columns before analysis. The ExperimentSubset object is a container to efficiently manage different subsets of the same data without having to make separate objects for each new subset.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 7.1.1
Depends:
  R (>= 4.0.0),
  SummarizedExperiment,
  SingleCellExperiment
biocViews: Infrastructure, Software, DataImport, DataRepresentation
Imports:
  methods,
  Matrix
Suggests: 
    BiocStyle,
    knitr,
    rmarkdown,
    testthat,
    covr,
    stats,
    scran,
    scater,
    scds,
    TENxPBMCData
VignetteBuilder: knitr

mtmorgan commented 4 years ago

It looks like the person submitting this package is not listed as the maintainer ("cre" in the Authors@R field). Please update this so that the maintainer and submitter are the same, and post a comment here.

irzamsarfraz commented 4 years ago

I have updated the author information in the DESCRIPTION file.

lawremi commented 4 years ago

Presumably a similar efficiency could be achieved with DelayedArray and DelayedDataFrame. Tracking the subset information at the SE level may be useful from the provenance perspective.

If the ExperimentSubset class inherited from SummarizedExperiment, it would be a drop-in replacement and thus a very useful tool. While SummarizedExperiment does carry slots, they could simply be ignored. The overall decorator approach is sound.

The vignette currently just describes the individual methods. It would be more useful if it described an practical workflow.

joshua-d-campbell commented 4 years ago

Hi @lawremi, thanks for your feedback. The provenance and streamlined framework for storing/accessing subsets is definitely the primary objective of the package. For single cell RNA-seq analysis, a lot of subsets of the original data are taken at different times (e.g. filtering out empty droplets, filtering out poor quality cells, limiting to highly variable genes, clustering with a subpopulation of cells). Right now in the singleCellTK package, we are keeping these in different objects in a list, which is cumbersome. We will add a practical example to the vignette shortly to illustrate a workflow.

Interestingly, the first version of the package we made did have the ExperimentSubset class inheriting from SummarizedExperiment. But then we had to have a second class called SingleCellExperimentSubset which inherited from SingleCellExperiment. We changed the framework to make one class that would generally work with any Experiment class by having a "root" slot storing the original data. We are happy to change it back if there is a preference for the first approach though. The ability to use it as a drop-in replacement is certainly a benefit.

We were also a little bit worried about unforeseen consequences with replacing several of the standard generics from S4vectors (e.g. rownames) so they could take in additional parameters . Just let us know if there are any issues with that.

bioc-issue-bot commented 4 years 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 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, 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. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lawremi commented 4 years ago

I appreciate the complications brought about by inheritance, which need to be weighed against the convenience of having an interoperable object. By continuing to rely on the decorator pattern, i.e., having an internal SE as the delegate, one could define methods on e.g. SingleCellExperiment-specific generics which would just force the subset and delegate to the result (failing if the delegate is not of the correct type).

There are ongoing discussions between R core and the tidyverse around a new OOP framework for R, which could support implicit conversions to facilitate decoration.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR, 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. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, TIMEOUT". 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. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "WARNINGS". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

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

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

irzamsarfraz commented 4 years ago

Hi all,

We have fixed all build issues and updated the vignette in detail that now illustrates practical workflows with real datasets and outlines the structure of our package.

mtmorgan commented 4 years ago

DESCRIPTION

good

vignettes

quite a nice vignette!
ExperimentSubset.Rmd:91 An important aspect of object-oriented approaches is to separate the implementation (slots) from the interface; the heading here undermines this, at least in a user-oriented (section) of a vignette. Perhaps this is better discussed in a section about ## Implementation details?

R (comments are on specfic lines, but apply generally)

line 70: use accessors, even internally; these do not need to be exported, e.g., .root <- function(x) x@root.
line 71 this looks like a stop() rather than return value. Also, a best practice is for a function to reutrn a consistent value; here you sometimes return character(1) and sometimes logical(1), which only makes work for the user.
line 115: no need to prefix ExperimentSubset:: here, or methods:: later.
line 133: start the parameter description with a statement of expected object type, e.g., character(1) Specify....
line 157: likely you only intend (there is only one method) to dispatch on the first argument of the generic; use the signature = argument of setGeneric() to limit dispatch (and simplify method specification) to the first argument. Methods should still validate (e.g., via stopifnot()) the remaining arguments.

line 196: avoid nested statements, especially in tests, e.g., write as

test <- 
    parentAssay %in% SummarizedExperiment::assayNames(object@root) ||
    parentAssay %in% subsetAssayNames(object)
if (test) ...

line 334: stop(paste(...)) can usually be replace with stop(...) (which calls paste(...) internally).
line 1664: a cleaner presentation is to write a single cat():
```
cat(
    "class: ExperimentSubset\n",
    "root ",
    ...
```
line 1703: the formatting of this method is different from others, requiring much more indentation and less space per line; revise as
```
setReplaceMethod(
    f = "assay",
    signature = ...
```
line 1755 avoid use of F in favor of FALSE, since F can be redefined but FALSE cannot.

tests

I'm not sure I understand what tests/test-ExperimentSubset.R is doing here, rather than within tests/testthat/?
there is value in writing smaller test_that() clauses, since failures are reported at this level and one then has to step through the test to identify the problem.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

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

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

On one or more platforms, the build results were: "ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

irzamsarfraz commented 4 years ago

@mtmorgan

Hi,

We have made almost all of the suggested changes. Please see our responses against each comment for further clarification.

Thank you!

ExperimentSubset.Rmd:91 An important aspect of object-oriented approaches is to separate the implementation (slots) from the interface; the heading here undermines this, at least in a user-oriented (section) of a vignette. Perhaps this is better discussed in a section about ## Implementation details?

Done! We have added an Implementation Details section at the end of the vignette, so regular users don't really have to bother with these details.

line 70: use accessors, even internally; these do not need to be exported, e.g., .root <- function(x) x@root.

Done!

line 71 this looks like a stop() rather than return value. Also, a best practice is for a function to reutrn a consistent value; here you sometimes return character(1) and sometimes logical(1), which only makes work for the user.

Done!

line 115: no need to prefix ExperimentSubset:: here, or methods:: later.

Done!

line 133: start the parameter description with a statement of expected object type, e.g., character(1) Specify....

Done! Modified throughout the package!

line 157: likely you only intend (there is only one method) to dispatch on the first argument of the generic; use the signature = argument of setGeneric() to limit dispatch (and simplify method specification) to the first argument. Methods should still validate (e.g., via stopifnot()) the remaining arguments.

Unfortunately, this is something we haven't been able to modify yet as the suggestion is a little unclear to us. Could this be explained in a little more depth and we would definitely update the package asap!

line 196: avoid nested statements, especially in tests, e.g., write as

test <- parentAssay %in% SummarizedExperiment::assayNames(object@root) || parentAssay %in% subsetAssayNames(object) if (test) ...

Done!

line 334: stop(paste(...)) can usually be replace with stop(...) (which calls paste(...) internally).

Done!

line 1664: a cleaner presentation is to write a single cat():

cat( "class: ExperimentSubset\n", "root ", ...

We have removed as many cat() calls as possible, but because we need to call the show() method internally, it wasn't really working as intended and the output got cluttered. As a result, we had to use at least 3 of the cat() calls (alongwith use of paste() and paste0() as we needed different separators at times).

line 1703: the formatting of this method is different from others, requiring much more indentation and less space per line; revise as

setReplaceMethod( f = "assay", signature = ...

Done!

line 1755 avoid use of F in favor of FALSE, since F can be redefined but FALSE cannot.

Done!

tests

I'm not sure I understand what tests/test-ExperimentSubset.R is doing here, rather than within tests/testthat/?

Done! Fixed.

there is value in writing smaller test_that() clauses, since failures are reported at this level and one then has to step through the test to identify the problem.

Done! Added smaller test_that() units relevant to a particular functionality or a group of functions.

mtmorgan commented 4 years ago

For the generic, if one writes

setGeneric("foo", function(x, y) standardGeneric("foo"))

then the generic will dispatch on values of x and y. But often 'y' is not meant for dispatch, e.g.,

setGeneric("bar", function(x, verbose=TRUE) standardGeneric("bar"))

From the perspective of the person writing this generic, verbose will always be logical, or else an error. So one writes

setGeneric(
    "bar",
    function(x, verbose = TRUE) standardGeneric("bar"),
    signature = "x"
)

and methods are

setMethod(
    "foo",
    "A",
    function(x, verbose = TRUE) {
        stopifnot(is.logical(verbose))
        ....
    }
)

Here the number of methods necessary is equal to the number of classes x that are allowed, rather than the combinatorial explosion implied by methods on each argument.

bioc-issue-bot commented 4 years ago

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

irzamsarfraz commented 4 years ago

Hi @mtmorgan

This definitely makes sense now. We have updated the package and pushed the changes.

Thank you!

bioc-issue-bot commented 4 years ago

Dear Package contributor,

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

Your package has been built on Linux, Mac, and Windows.

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

Please see the build report for more details. This link will be 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/ExperimentSubset to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 4 years ago

Your package has been accepted. It will be added to the Bioconductor nightly builds.

Thank you for contributing to Bioconductor!

mtmorgan commented 4 years ago

The master branch of your GitHub repository has been added to Bioconductor's git repository.

To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/irzamsarfraz.keys is not empty), then no further steps are required. Otherwise, do the following:

See further instructions at

https://bioconductor.org/developers/how-to/git/

for working with this repository. See especially

https://bioconductor.org/developers/how-to/git/new-package-workflow/ https://bioconductor.org/developers/how-to/git/sync-existing-repositories/

to keep your GitHub and Bioconductor repositories in sync.

Your package will be included in the next nigthly 'devel' build (check-out from git at about 6 pm Eastern; build completion around 2pm Eastern the next day) at

https://bioconductor.org/checkResults/

(Builds sometimes fail, so ensure that the date stamps on the main landing page are consistent with the addition of your package). Once the package builds successfully, you package will be available for download in the 'Devel' version of Bioconductor using BiocManager::install("ExperimentSubset"). The package 'landing page' will be created at

https://bioconductor.org/packages/ExperimentSubset

If you have any questions, please contact the bioc-devel mailing list (https://stat.ethz.ch/mailman/listinfo/bioc-devel); this issue will not be monitored further.

Bioconductor / Contributions

ExperimentSubset #1673