Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

SFEData, SpatialFeatureExperiment, and Voyager #2719

Closed lambdamoses closed 2 years ago

lambdamoses commented 2 years ago

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

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

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

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 @lambdamoses

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: SFEData
Title: Example SpatialFeatureExperiment datasets
Version: 0.99.0
Authors@R: 
    c(person("Lambda", "Moses", email = "dlu2@caltech.edu", 
    role = c("aut", "cre"),
    comment = c(ORCID = "0000-0002-7092-9427")),
    person("Lior", "Pachter", email = "lpachter@caltech.edu",
    role = c("aut", "ths"),
    comment = c(ORCID = "0000-0002-9164-6231")))
Description: 
    Example spatial transcriptomics datasets with Simple Feature annotations
    as SpatialFeatureExperiment objects. In the first version, the only full
    dataset is a mouse skeletal muscle Visium dataset 2 days after notexin
    injury from 'Large-scale integration of single-cell transcriptomic data 
    captures transitional progenitor states in mouse skeletal muscle 
    regeneration'. More datasets will be added in later versions.
Imports:
    AnnotationHub,
    ExperimentHub
Suggests: 
    knitr,
    rmarkdown,
    SpatialFeatureExperiment,
    testthat (>= 3.0.0)
License: GPL-3
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.0
biocViews: 
    ExperimentHub,
    ExpressionData,
    Mus_musculus_Data,
    SpatialData,
    Tissue,
    SingleCellData
VignetteBuilder: knitr
Config/testthat/edition: 3
vjcitn commented 2 years ago

Will you be following the https://github.com/Bioconductor/Contributions#submitting-related-packages so that SpatialFeatureExperiment is in place for checking SFEData?

vjcitn commented 2 years ago

I installed SpatialFeatureExperiment from pachterlab repo to check this package and it seemed fine. You will need to submit SpatialFeatureExperiment too, as a related package.

lshep commented 2 years ago

@vjcitn to have the interface for submitting related packages work correctly they can't submit it until the data package is already in review with a 'review in progress' tag (and therefore already initialized in the review process). If it is good please proceed with the 'pre-check passed'

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, 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/SFEData to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.

lambdamoses commented 2 years ago

AdditionalPackage: https://github.com/pachterlab/SpatialFeatureExperiment

bioc-issue-bot commented 2 years ago

Hi @lambdamoses, Thanks for submitting your additional package: https://github.com/pachterlab/SpatialFeatureExperiment. We are taking a quick look at it and you will hear back from us soon.

lambdamoses commented 2 years ago

AdditionalPackage: https://github.com/pachterlab/Voyager

bioc-issue-bot commented 2 years ago

Hi @lambdamoses, Thanks for submitting your additional package: https://github.com/pachterlab/Voyager. We are taking a quick look at it and you will hear back from us soon.

vjcitn commented 2 years ago

Hi, bump your version numbers/commit/push to get a new build.

bioc-issue-bot commented 2 years ago

Additional Package has been approved for building.

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.

bioc-issue-bot commented 2 years ago

Additional Package has been approved for building.

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.

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, 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/SpatialFeatureExperiment 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

Additional Package has been approved for building.

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.

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/SpatialFeatureExperiment 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

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/Voyager 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

Additional Package has been approved for building.

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.

bioc-issue-bot commented 2 years ago

Additional Package has been approved for building.

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.

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/SpatialFeatureExperiment 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

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/SpatialFeatureExperiment 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: 11579860423af00cea4cf7f2f36a5298c659b02d

bioc-issue-bot commented 2 years ago

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

bioc-issue-bot commented 2 years ago

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

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/SpatialFeatureExperiment 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

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/SFEData 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: e05e870d05ceeebf15498c0c27cad69a31419a09

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/Voyager 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

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/SFEData 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: d219d04006c84d50843413acdc5f2e4e0bcab5f6

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/Voyager 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: b3524c1da0a87a9aac1974d7190e7a3e41703aac

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/Voyager 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: bc10e766faa0573951752cf498c6b1329ecac666

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

Kayla-Morrell commented 2 years ago

Hello @lambdamoses - I'm working through the initial reviews of these packages. I wanted to check in and see how you wanted to handle the review process. Would you prefer working through one package at a time or would you rather I post all three reviews at the same time?

Best, Kayla

lambdamoses commented 2 years ago

I suppose one package at a time.

Kayla-Morrell commented 2 years ago

@lambdamoses - Great, then below is the initial review of SFEData. The required changes must be made while the suggested changes do not have to be (though we strongly encourage them). Comment back here with updates that have been made and when the package is ready for a re-review.

DESCRIPTION

Vignette

R code

Best, Kayla

bioc-issue-bot commented 2 years ago

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

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

lambdamoses commented 2 years ago

DESCRIPTION

  • [x] SUGGESTION: We encourage using the 'BugReports' field and including the relevant link to GitHub for reporting issues. (I also added URL in the DESCRIPTION file.)

Vignette

  • [x] SUGGESTION: We encourage the use of BiocStyle for formatting.
  • [x] REQUIRED: Add an 'Installation' section that demonstrates to the user how to download and load the package from Bioconductor. This code can be included in an eval = FALSE code chunk.

R code

  • [ ] SUGGESTION: For formatting reasons, consider shorter lines. There are 2 lines that are > 80 characters long. (I can't locate those lines in the R code files. It's probably referring to the plain text in the vignette, which I think is fine since RStudio wraps the plain text.)
  • [ ] SUGGESTION: For formatting reasons, consider multiples of 4 spaces for line indents. There are 14 lines that are not. (I'm accustomed to indenting by 2 spaces.)
Kayla-Morrell commented 2 years ago

@lambdamoses - Great, thank you for making those changes. I've looked them over and SFEData now looks good. Below is the initial review of SpatialFeatureExperiment. Again, the required changes must be made while the suggested changes do not have to be (though we strongly encourage them). Comment back here with updates that have been made and when the package is ready for a re-review.

General package development

DESCRIPTION

Data

Vignette

Man pages

R code

Best, Kayla

lambdamoses commented 2 years ago

Question about using :::: I used S4Vectors:::disableValidity because I found SingleCellExperiment and SpatialExperiment using it, so I thought it's allowed. I also used some internal functions of SingleCellExperiment that I find useful. If it's not allowed, then what alternative do you suggest? Like copying and pasting the code for the internal functions?

hpages commented 2 years ago

I'd say it's ok to ignore the R CMD check NOTE about S4Vectors:::disableValidity. In some rare situations one actually needs to temporarily disable validation of S4 objects but unfortunately the methods package doesn't provide a mechanism for that. A few Bioconductor packages use S4Vectors:::disableValidity already: InteractionSet, miloR, NxtIRFcore, PhosR, SingleCellExperiment, SpatialExperiment, SpliceWiz, and TreeSummarizedExperiment. Of course S4Vectors:::disableValidity should be exported and documented but that's on me.

lambdamoses commented 2 years ago

I also used some internal functions of SingleCellExperiment though, like SingleCellExperiment:::.get_internal_character, which is used in the implementation of reducedDims in SingleCellExperiment, because the new field colGeometries in int_colData is implemented in a similar way as reducedDims. I just don't want to reinvent the wheel. But then if the internals of SingleCellExperiment change, my package will break. Then I suppose I'd better copy and paste the code from SingleCellExperiment? Suggestions?

Voyager uses some internal functions in SpatialFeatureExperiment; I suppose I can export them even though they're not meant for users.

LiNk-NY commented 2 years ago

Hervé @hpages I could move it and document it in BiocBaseUtils as we did with replaceSlots > setSlots. Best, Marcel

hpages commented 2 years ago

@lambdamoses,

But then if the internals of SingleCellExperiment change, my package will break.

You can also look at it the other way around: if they improve SingleCellExperiment:::.get_internal_character, or fix a bug in it, then your package will immediately benefit. Also note that if they break your package, it'll be in Bioc devel only, so it won't affect regular users and you'll have time to fix. That being said, SingleCellExperiment:::.get_internal_character is such a simple/small function that maybe it's not a big deal to just have your own copy in your package? It's a trade-off.

@LiNk-NY Sounds good. Almost everything in S4Vectors/R/S4-utils.R should probably make it to BiocBaseUtils. Plus all the stuff in S4Vectors/R/show-utils.R and most of the stuff in S4Vectors/vector-utils.R, S4Vectors/integer-utils.R, S4Vectors/character-utils.R, S4Vectors/raw-utils.R, and S4Vectors/eval-utils.R. But let's not discuss this here.

bioc-issue-bot commented 2 years ago

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

lambdamoses commented 2 years ago

General package development

  • [x] REQUIRED: In the README file the installation instructions should also include Bioconductor installation instructions.

DESCRIPTION

  • [x] SUGGESTION: 'SpatialExperiment' is misspelled in the Title field.
  • [x] SUGGESTION: It is encouraged to include the BugReports field with relevant links to GitHub for reporting issues.

Data

  • [x] REQUIRED: Raw data files should be included in the 'inst/extdata' directory. We also require documentation on these files in an 'inst/script/' directory.
  • [x] REQUIRED: Documentation of the raw data is needed in the 'inst/script/' directory. The scripts in this directory vary. It's most important to include how the data was generated and source information. It should include source URLs and any key information regarding filtering or processing. It can be executable code, sudo code, or a text description. Users should be able to download and roughly reproduce the file or object that is present as data.
  • [x] REQUIRED: The 'data-raw' and 'inst/testdata' are not acceptable locations for data. I believe that the 'inst/testdata' belongs in the 'inst/extdata' directory and the 'data-raw' belongs in the 'inst/script' directory. When moving data to 'inst/extdata' you will access it using system.file() instead of readRDS().

Vignette

  • [x] REQUIRED: Add an 'Installation' section that show to users how to download and load the package from Bioconductor. The instructions should be in an eval = FALSE code chunk.

Man pages

  • [x] SUGGESTION: The use of dontrun{} / donttest{} is discouraged and generally not allowed. However, if this is necessary than we prefer donttest{} over dontrun{} since donttest{} requires valid R code while dontrun{} does not.

R code

  • [ ] REQUIRED: Unexported object imported by a ':::' call: 'S4Vectors:::disableValidity'. See the note in ?':::' about the use of this operator.
  • [x] REQUIRED: Avoid 1:...; use seq_len() or seq_along() instead.

Question: I did get rid of 1:... and replaced them with seq_len. While I understand that 1:n and 1:length(x) may be dangerous as n and length(x) may be 0, all cases when I used 1:... did not involve variables, only fixed numbers, for subsetting, like 1:3. So is something like 1:3 still discouraged?

  • [ ] REQUIRED: Avoid the use of paste in condition signals.

All cases when I used paste in warning or stop is to collapse multiple items to show, which is allowed per https://contributions.bioconductor.org/r-code.html?q=paste#end-user-messages

  • [ ] REQUIRED: Avoid supressWarnings/*Messages if possible (found 1 times)

suppressWarnings is used in the [ method for SpatialFeatureExperiment: suppressWarnings(x <- callNextMethod()), which calls the [ method for SpatialExperiment. I need to suppress the warning here because the SFE method uses the drop argument in its unique way that does not apply to SpatialExperiment and SingleCellExperiment. I get this warning if the drop argument is used. The warning comes from callNextMethod, and the drop argument is used in the code after callNextMethod.

sfe_subset <- sfe[1:10, 1:10, drop = TRUE]
Node indices in the graphs are no longer valid after subsetting. Dropping all row and col graphs.
Warning message:
In .nextMethod(x = x, i = i, j = j, drop = drop) :
  'drop' ignored '[,SpatialFeatureExperiment,ANY,ANY-method'
  • [ ] SUGGESTION: For formatting reasons, consider shorter lines. There are 312 lines that are > 80 characters long.

I did some reformatting to shorten some lines, and the remaining ones over 80 characters long aren't that much longer than 80 characters.

  • [ ] SUGGESTION: For formatting reasons, consider multiples of 4 spaces for line indents. There are 1240 lines that are not.

I usually and consistently indent by 2 spaces as in RStudio's default.

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, 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/SpatialFeatureExperiment to trigger a new build. A quick tutorial for setting up remotes and pushing to upstream can be found here.