NicoleEO commented 2 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/Nanostring-Biostats/NanoStringExperiment

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 2 years ago

Hi @NicoleEO

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: NanoStringExperiment
Type: Package
Title: Base NanoString Experiment Class
Version: 0.99.0
Authors@R: 
    c(person("Nicole", "Ortogero", email = "nortogero@nanostring.com", 
        role = c("cre", "aut")))
Description: 
    Base experiment class holding common NanoString Experiment functions 
    and data structure methods.Extends SummarizedExperiment for data produced 
    with NanoString Technologies instruments.
License: MIT
Encoding: UTF-8
RoxygenNote: 7.1.2
Depends: 
    R (>= 2.10),
    SummarizedExperiment
Imports: 
    Biobase, 
    BiocGenerics,
    stats
Suggests: 
    knitr,
    rmarkdown
Collate: 
    SignatureSet-class.R
    NanoStringExperiment-class.R
    SignatureSet-methods.R
    NanoStringExperiment-accessors.R
    NanoStringExperiment-subset.R
    NanoStringExperiment-apply.R
    NanoStringExperiment-summary.R
    NanoStringExperiment-munge.R
    NanoStringExperiment-normalize.R
    NanoStringExperiment-qc.R
biocViews: 
    Spatial,
    GeneExpression, 
    Transcription, 
    CellBasedAssays, 
    DataImport, 
    Transcriptomics, 
    Proteomics, 
    mRNAMicroarray, 
    ProprietaryPlatforms, 
    RNASeq, 
    Sequencing, 
    ExperimentalDesign, 
    Normalization
VignetteEngine: knitr::rmarkdown
VignetteBuilder: knitr

vjcitn commented 2 years ago

Please look at your compiled vignette. Much too much data is being dumped out raw. The browse cannot really render it. nse

That goes on for a very long time. Doesn't belong in vignette.

NicoleEO commented 2 years ago

@vjcitn I've fixed in last push.

vjcitn commented 2 years ago

--- re-building ‘Introduction.Rmd’ using rmarkdown
Quitting from lines 62-73 (Introduction.Rmd) 
Error: processing vignette 'Introduction.Rmd' failed with diagnostics:
invalid class "NanoStringExperiment" object: invalid object for slot "signatures" in class "NanoStringExperiment": got class "NULL", should be or extend class "SignatureSet"
--- failed re-building ‘Introduction.Rmd’

SUMMARY: processing the following file failed:
  ‘Introduction.Rmd’

NicoleEO commented 2 years ago

@vjcitn Fixed to match recent slot update.

bioc-issue-bot commented 2 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 2 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/NanoStringExperiment to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

NicoleEO commented 2 years ago

@LiNk-NY Hi Marcel, Not sure if this is possible, but can I sacrifice my review spot in line for Maddy's SpatialOmicsOverlay package? We are most interested in getting SpatialOmicsOverlay (SOO) in the release. My NanoStringExperiment package is a data structure change that we are hoping to slowly roll out while SpatialOmicsOverlay is a really exciting image overlay package. We believe Maddy's SOO package should be pre-check passable as of today. Thanks, Nicole

bioc-issue-bot commented 2 years ago

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

vjcitn commented 2 years ago

Apropos sacrificing review spot -- please see https://github.com/Bioconductor/Contributions/issues/2604#issuecomment-1103736459 ...

bioc-issue-bot commented 2 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/NanoStringExperiment to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/NanoStringExperiment to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/NanoStringExperiment to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 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/NanoStringExperiment to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

LiNk-NY commented 2 years ago

Hi Nicole, @NicoleEO

Thank you for your submission!

Please see my review below. Best regards, Marcel

NanoStringExperiment #2602

The package needs reconsideration of design. For increased usability, it must disentangle the re-use of methods from both Biobase and SummarizedExperiment, as well as, reduce the number of methods that are not used in the package but are created perhaps as placeholders. It is recommended that the SummarizedExperiment class and interface be used to achieve the design goals.

DESCRIPTION

Update the Description to say NanoStringExperiment inherits from SummarizedExperiment.
Update the R version dependency to 4.2. See BiocCheck.
Include BugReports and URL links to GitHub.

NAMESPACE

Why the use of Biobase? If your data class inherits from SummarizedExperiment then you likely do not need Biobase because the former supercedes the latter.

vignettes

Consider using output: BiocStyle::html_vignette for Bioconductor styling of the vignette.
The arguments in the main function are not consistent with the SummarizedExperiment interface.
Mixing of interfaces is not recommended. Why assays over exprs?
There is a colData extractor and a phenoData argument. What is the difference between these in your class? It may only increase confusion among users.

R

Isn't dimLabels the same as dimnames? I don't see why this was chosen over the other.
Please make sure that the consistency of methods is being adhered to. For example, a user would expect the exprs method to return the matrix in the object and not the assay named "exprs" in the data. The change in behaviour would likely lead to confusion.
It is not adviseable to pull data from the metadata if you have control of the class. It may be better to create a protocolData/phenotypeCols slot in your class rather than checking the metadata for it.
Why rename the metadata to experimentData?
Avoid creating generics and methods instead of plain function for the constructor function. The plan function should be able to handle all types of input and make a call to the internal .NanoStringExperiment function.
Some signature arguments refer to an object class. Should this be NanoStringExperiment?

tests

Please add unit tests to the package:

covr::package_coverage()
NanoStringExperiment Coverage: 0.00%
R/NanoStringExperiment-accessors.R: 0.00%
R/NanoStringExperiment-apply.R: 0.00%
R/NanoStringExperiment-class.R: 0.00%
R/NanoStringExperiment-munge.R: 0.00%
R/NanoStringExperiment-normalize.R: 0.00%
R/NanoStringExperiment-qc.R: 0.00%
R/NanoStringExperiment-subset.R: 0.00%
R/NanoStringExperiment-summary.R: 0.00%

NicoleEO commented 2 years ago

Hi @LiNk-NY. Thanks for reviewing! Yes, we know this is definitely an odd package in its current form. The addition of the Biobase methods is to support legacy packages and workflows as well our customers that have built pipelines using the old ExpressionSet inherited class. That being said we would like to move to SummarizedExperiment for all of our new packages as well as our current packages on Bioconductor, which is why we are releasing this new package, but some Biobase methods are reimplemented. I have added notes throughout the docs indicating that developers should use the SummarizedExperiment methods from here on. Here's a summary of our planned class structure (Note: GeomxTools is at the crux of several of our other packages so this change propagates further).

LiNk-NY commented 2 years ago

Hi Nicole, @NicoleEO

It may be better to provide awareness for the ExpressionSet to SummarizedExperiment coercion method.

I am not seeing why implement a mixed interface when you can only stick to the modern one and provide help for those who are still lagging behind with ExpressionSet. It is especially important to have the design of the class straightened out before expanding and inheriting from it. Currently, the schematic provided should also include Biobase along side SummarizedExperiment...

Best, Marcel

LiNk-NY commented 2 years ago

Hi Nicole, @NicoleEO

Are there any updates on the trajectory of this package? We'd be happy to provide further assistance.

Best regards, Marcel

NicoleEO commented 2 years ago

Hi Marcel, @LiNk-NY

I am still working on this. I have some other projects that are taking priority right now, but I'm planning on getting back to this soon.

Thanks, Nicole

NicoleEO commented 2 years ago

@LiNk-NY

Hi Marcel, We will not be able to hit release time for this package as it still needs some work and after Bioc we started debating base class (SummarizedExperiment of SpatialExperiment). Is there a process for retracting the submission? I want to delay and resubmit hopefully next release. Thanks, Nicole

LiNk-NY commented 2 years ago

Hi Nicole! @NicoleEO

I hope you're doing well.

We can mark the issue as inactive and you can resubmit when ready.

Feel free to tag me or the core team with @Bioconductor/core if you have any questions throughout the process. I am also on Bioc-Community Slack.

Best regards, Marcel

NicoleEO commented 2 years ago

Will do. Thanks Marcel!

Bioconductor / Contributions

(inactive) NanoStringExperiment #2602

NanoStringExperiment #2602

DESCRIPTION

NAMESPACE

vignettes

R

tests