Closed ycao6928 closed 1 year ago
Hi @ycao6928
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: scFeatures
Title: scFeatures: Multi-view representations of single-cell and spatial data for disease outcome prediction
Version: 0.99.1
Authors@R:
c(
person(given = "Yue",family = "Cao", email = "yue.cao@sydney.edu.au", role=c("aut", "cre")),
person(given = "Yingxin",family = "Lin", email = "yingxin.lin@sydney.edu.au", role="aut" ),
person(given = "Ellis",family = "Patrick", email = "ellis.patrick@sydney.edu.au", role="aut" ),
person(given = "Pengyi",family = "Yang", email = "pengyi.yang@sydney.edu.au", role="aut" ),
person(given = "Jean Yee Hwa",family = "Yang", email = "jean.yang@sydney.edu.au", role="aut" ))
Description: scFeatures constructs multi-view representations of single-cell and spatial data.
License: GPL-3
Encoding: UTF-8
LazyData: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 7.2.1
Remotes:
sqjin/CellChat
biocViews: CellBasedAssays, SingleCell, Software
Imports:
ClassifyR,
DelayedArray,
DelayedMatrixStats,
EnsDb.Hsapiens.v79,
EnsDb.Mmusculus.v79,
GSVA,
Seurat,
ape,
caret,
glue,
dplyr,
ensembldb,
gtools,
msigdbr,
plyr,
proxyC,
reshape2,
spatstat.core,
spatstat.geom,
tidyr,
AUCell,
BiocParallel,
CellChat,
SpatialExperiment,
SummarizedExperiment,
rmarkdown
Suggests:
knitr,
inline
VignetteBuilder: knitr
You have
Remotes:
sqjin/CellChat
All submissions must constrain their dependencies to packages that are in CRAN or Bioconductor. If CellChat is going to Bioconductor, please have it submitted and reviewed before submitting this one.
Additionally, I cannot speak for the build system per se, but on two independent linux systems that have succeeded with over 1000 current Bioc packages, I am seeing
tvjc@visually-civil-zebra:~$ R CMD build scFeatures
* checking for file 'scFeatures/DESCRIPTION' ... OK
* preparing 'scFeatures':
* checking DESCRIPTION meta-information ... OK
* installing the package to build vignettes
* creating vignettes ... ERROR
--- re-building ‘Vignette_associationstudy.Rmd’ using rmarkdown
Warning in png(..., res = dpi, units = "in") :
unable to open connection to X11 display ''
Warning: The vignette title specified in \VignetteIndexEntry{} is different from the title in the YAML metadata. The former is "Vignette_associationstudy", and the latter is "Vignette - association study of features with conditions". If that is intentional, you may set options(rmarkdown.html_vignette.check_title = FALSE) to suppress this check.
--- finished re-building ‘Vignette_associationstudy.Rmd’
--- re-building ‘Vignette_detail.Rmd’ using rmarkdown
make cmd is
make -f '/home/stvjc/BBS_space/R-4-2-dist/lib/R/etc/Makeconf' -f '/home/stvjc/BBS_space/R-4-2-dist/lib/R/share/make/shlib.mk' SHLIB='file54a0312495fd.so' OBJECTS='file54a0312495fd.o'
make would use
if test "zfile54a0312495fd.o" != "z"; then \
echo gcc -shared -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -L/usr/local/lib -o file54a0312495fd.so file54a0312495fd.o /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -lR; \
gcc -shared -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -L/usr/local/lib -o file54a0312495fd.so file54a0312495fd.o /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -lR; \
fi
Quitting from lines 22-36 (Vignette_detail.Rmd)
Error: processing vignette 'Vignette_detail.Rmd' failed with diagnostics:
/usr/bin/ld: cannot find /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so: No such file or directorycollect2: error: ld returned 1 exit statusmake: *** [/home/stvjc/BBS_space/R-4-2-dist/lib/R/share/make/shlib.mk:10: file54a0312495fd.so] Error 1
--- failed re-building ‘Vignette_detail.Rmd’
--- re-building ‘Vignette_summary.Rmd’ using rmarkdown
make cmd is
make -f '/home/stvjc/BBS_space/R-4-2-dist/lib/R/etc/Makeconf' -f '/home/stvjc/BBS_space/R-4-2-dist/lib/R/share/make/shlib.mk' SHLIB='file54a05b06e4fc.so' OBJECTS='file54a05b06e4fc.o'
make would use
if test "zfile54a05b06e4fc.o" != "z"; then \
echo gcc -shared -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -L/usr/local/lib -o file54a05b06e4fc.so file54a05b06e4fc.o /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -lR; \
gcc -shared -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -L/usr/local/lib -o file54a05b06e4fc.so file54a05b06e4fc.o /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so -L"/home/stvjc/BBS_space/R-4-2-dist/lib/R/lib" -lR; \
fi
Quitting from lines 23-37 (Vignette_summary.Rmd)
Error: processing vignette 'Vignette_summary.Rmd' failed with diagnostics:
/usr/bin/ld: cannot find /usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so: No such file or directorycollect2: error: ld returned 1 exit statusmake: *** [/home/stvjc/BBS_space/R-4-2-dist/lib/R/share/make/shlib.mk:10: file54a05b06e4fc.so] Error 1
--- failed re-building ‘Vignette_summary.Rmd’
SUMMARY: processing the following files failed:
‘Vignette_detail.Rmd’ ‘Vignette_summary.Rmd’
Error: Vignette re-building failed.
Execution halted
Are there specific system configuration steps needed to get this package to build?
NB this Warning in png(..., res = dpi, units = "in") : unable to open connection to X11 display ''
is ameliorated by running with xvfb-run, it is not an issue; the remaining errors noted arise under xvfb-run.
library(inline)
openblas.set.num.threads <- cfunction( signature(ipt="integer"),
body = 'openblas_set_num_threads(*ipt);',
otherdefs = c ('extern void openblas_set_num_threads(int);'),
libargs = c ('/usr/lib/x86_64-linux-gnu/openblas-pthread/libblas.so'),
language = "C",
convention = ".C"
)
openblas.set.num.threads(1)
must be done in a portable way ... this is from Vignette_details
Finally I'd suggest not using "Vignette_" to prefix the name of the vignette files.
Use the package name or something more descriptive.
Apropos https://github.com/Bioconductor/Contributions/issues/2815#issuecomment-1271492099
please see https://stackoverflow.com/a/45796357/547331 and the associated https://cran.r-project.org/package=RhpcBLASctl -- thanks to Martin Morgan for this observation.
Let us know if you are going to work on this. @vjcitn tagging myself to ensure I hear about it.
Dear @vjcitn, Thank you so much for looking into the package! Yes, we are actively working on it this week and addressing all the comments.
@vjcitn We have made significant changes. Could you have another look?
I think that running install.packages(RhpcBLASctl)
might fix it. It is listed in the suggests dependencies, so it might not be installed automatically when building.
Another way to solve the dependency issues might be to run devtools::load_all()
in the repository directory.
Yes, that was my bad, with the BLASctl failure.... but now we have a package of 127MB that is supposed to be no more than 5MB. So you'll have to discuss with @lshep whether this submission should include an ExperimentData package
It seems to be passing check cleanly, so we just have to meet the guidelines.
We downsampled the data and the package is now under 5MB.
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
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: 0e77e169f8adaa118afb879eccd533d4e3e2001c
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: 494098d7b6b9d1e31b3ece63b88af3160e4d6b0f
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
@PeteHaitch Could you please advise how to address the warning? We are not sure.
Warning: replacing previous import 'dplyr::select' by 'ensembldb::select' when loading 'scFeatures'
You are importing both in your NAMESPACE
: https://github.com/SydneyBioX/scFeatures/blob/master/NAMESPACE#L44-L45
I think you will have to choose to import only one and then refer to the other using ::
in your package code.
E.g., you could choose to have importFrom(dplyr, select)
(or the equivalent via roxygen2) and then use ensembldb::select()
everywhere in your package code.
Alternatively, you could import neither in your NAMESPACE
and use the ::
versions everywhere in your package code.
Received a valid push on git.bioconductor.org; starting a build for commit id: 3cdbf3426a09d48bad978ab6060ca608c9f0ec0d
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: b8aef87b57ef47a03ec6a594cccc231be26f7729
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
@PeteHaitch Sorry for the delay. The package is now building without errors or warnings.
Sorry to ping you @PeteHaitch, but today is the deadline for package review for the next Bioconductor release. Is there anything that you would like us to address in the package?
Hi @nick-robo,
I'm sorry that I have not had time to review scFeatures before the deadline. scFeatures only got an 'OK' on the build system 3 days ago and there were several packages ahead of yours in the reviewing queue.
While scFeatures won't make the next Bioconductor release (3.16), once it's been through the review process and accepted it will be immediately available in the development branch (3.17). I know this may be disappointing, but unfortunately there is only so much time available this close to the release.
Cheers, Pete
No worries! We appreciate all the effort that goes into the reviewing process. Thanks for letting us know.
I'll return to reviewing packages now that the release of Bioconductor 3.16 is done. However, I also have a Masters thesis to review and abstracts for BioC Asia to review, both with deadlines next week, so I'm afraid I won't be able to start my package reviews until those commitments are behind me. Thanks for your patience.
Hi @ycao6928 and @nick-robo,
Apologies for the delay, but I'm finally back to having time to review BioC packages.
Unfortunately, scFeatures isn't building on BioC 3.17 (devel branch) due to the classifyR dependency being unavailable (see the build report for ClassifyR). I'm also unable to build ClassifyR on my laptop (macOS with Intel).
Would you please:
I'll then be able to do my review proper.
You might also take this opportunity to clarify the nature of the classifyR dependency. It appears that you only use ClassifyR in one of the vignettes, so it could be a suggested dependency (see https://contributions.bioconductor.org/description.html#depends-imports-suggests-enhances for how to do this properly.). There may be other packages that could similarly be moved into suggested packages.
Cheers, Pete
@ycao6928 and @nick-robo: would you also please clarify who is maintainer of scFeatures and taking responsibility for the review (this should be the same person). Thanks!
Hi @PeteHaitch, many thanks for the suggestion! We will move classifyR to suggested dependency. I am the maintainer and taking responsibility of the review.
Received a valid push on git.bioconductor.org; starting a build for commit id: 6baa2daa7a380e1345866851526cbf61e97438a7
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Received a valid push on git.bioconductor.org; starting a build for commit id: 0fa955d8ecc099351ab68efe8fd564988b80c062
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
@PeteHaitch I am just helping out with getting things working :)
Got a new version that builds! Not sure about the warning, effectiveLibSizes
is not mentioned anywhere in the project.
Got a new version that builds! Not sure about the warning, effectiveLibSizes is not mentioned anywhere in the project.
I believe the warnings regarding effectiveLibrarySizes
is coming from recent/in-progress changes to edgeR:
We prefer to review a package that is passing without any warnings or errors on the build report, so please try to resolve these first. If you're having trouble resolving it, you might need to post to the bioc-devel mailing list.
I might be wrong, but it seems like the problem is upstream.
edgeR
is not used anywhere in scFeatures
, but it is imported by dependency of a core dependency. It is not declared in scFeatures
' namespace, so the warning (declared in NAMESPACE but not found
) cannot be related to us.
r$> pak::local_deps_explain("edgeR")
scFeatures -> SpatialExperiment -> DropletUtils -> edgeR
Additionally, I cannot replicate the warning in a local build on a Linux system (all my packages are updated to the latest versions).
Okay, I'll test on my macbook tomorrow.
Received a valid push on git.bioconductor.org; starting a build for commit id: 80034c0ec37c206d63a08198e22dda351126b805
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/scFeatures
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Hi @ycao6928,
Thank you for submitting scFeatures to Bioconductor.
I think the package needs a little more work, particularly to improve the documentation (man pages and vignettes), before it can be accepted.
It also feels a bit awkward for a Bioconductor package to not have first-class support for Bioconductor data structures (SingleCellExperiment and SpatialExperiment).
Yes, there's the makeSeurat()
converter function but the main functionality requires a Seurat object as input.
It would good in future versions to allow a user to pass a SingleCellExperiment/SpatialExperiment object as input (perhaps doing the conversion on the fly, if necessary, or, better yet, directly extracting the relevant data from the SingleCellExperiment/SpatialExperiment object)
In my review below I have separated the issues into Required and Recommended points that I would ask you to address before the package can be accepted. 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.
Cheers, Pete
R CMD check
. Bioconductor expects that vignettes contain non-trivial evaluated code chunks and static vignettes are generally not acceptable.
R CMD check
if the vignette code was evaluated. Some examples of where I encountered errors when running the vignette code:> feature_gene_prop_celltype <- run_gene_prop_celltype(data_remove_mito, genes = genes_of_interest)
Error: BiocParallel errors
1 remote errors, element index: 1
1 unevaluated and other errors
first remote error:
Error: Under current subsetting parameters, the default assay will be removed. Please adjust subsetting parameters or change default assay.
> feature_CCI <- run_CCI(data , species = "Homo sapiens" )
Error in run_CCI(data, species = "Homo sapiens") :
could not find function "run_CCI"
genes_of_interest <- c("TIGIT", "PDCD1")
feature_gene_prop_bulk <- run_gene_prop(data, genes = genes_of_interest )
Error: Under current subsetting parameters, the default assay will be removed. Please adjust subsetting parameters or change default assay.
scfeatures_result <- scFeatures(data,
# input selective feature types to generate.
feature_types = c("proportion_raw", "pathway_gsva", "L_stats", "gene_mean_celltype", "gene_prop_aggregated") ,
# by default assumes data is "scrna" (single-cell RNA-seq), this is now set to "spatial_p" (spatial proteomics).
type = "spatial_p" ,
# by default assumes "Homo sapiens", this is now set to `Mus musculus`.
species = "Mus musculus" ,
# by default uses the 50 hallmark genes, now set to user specified pathways.
geneset = list("pathway_a" = c("SMA" , "Vimentin" ), "pathway_b" = c("B7H3" ,"FoxP3" )) ,
# by default uses top variable genes to generate the celltype specific gene expression feature category,now set to user defined genes.
celltype_genes = data.frame(celltype = c("Macrophages" , "Mono/Neu" ) , marker = c("CD3" , "p53")) ,
# by default uses top variable genes to generate the overall aggregated gene expression feature category, now set to user defined genes.
aggregated_genes = c("CD3" , "p53") ,
# When passing as SingleCellExperiment or SpatialExperiment, by default we use the assay stored in "logcount"
assay = "norm",
# By default we look for the sample info in "sample" column and the celltype info in "celltype" column
sample = "imageID",
celltype = "cellType",
# If users want to construct features from the spatial metrics category, by default we look for the "x_cord" and "y_cord" column
spatialCoords = c("x", "y"),
# by default uses single core
ncores = 8)
Error: Cannot find 'cellType' in this Seurat object
process_data()
:data
, what sort of normalisation is applied when normalise = TRUE
, etc.)scFeatures_detail
vignette, but it properly belongs in the man pages. The vignette should show users how to use the package as a whole and the man pages how to use individual functions.run_proportion_raw()
returns a data.frame but is documented to return 'a matrix of samples x features'. These details matter when it comes to documenting package code.scFeatures_summary
) should read like an abstract; see https://contributions.bioconductor.org/docs.html#vignette-introductionDESCRIPTION
is insufficient because the scFeatures_detail
vignette still uses it to demonstrate the utility of scFeatures. Now that ClassifyR is passing builds again, please re-add ClassifyR to the DESCRIPTION
if using it in the vignettes or elsewhere in scFeatures.scFeatures_detail
vignette is a bit messy because it includes lots of verbose output (progress bars). Please revise.scFeatures_detail
vignette you are subsetting a vector outside its bounds; please fix:> unique(data$sample)[1:5]
[1] "Pre_P7" "Pre_P24" "Pre_P28" "Pre_P33" NA
> unique(data$sample)[1:5]
[1] "Pre_P7_cond_Responder" "Pre_P24_cond_Responder" "Pre_P28_cond_Responder"
[4] "Pre_P33_cond_Responder" NA
scFeatures_associationstudy
vignette, please use a temporary directory via tempdir()
, rather than the current working directory, to demonstrate report generation.tempdir()
but should work.> output_folder <- tempdir()
> run_association_study_report(scfeatures_result, output_folder )
Error in abs_path(input) : The file 'output_report.Rmd' does not exist.
In addition: Warning message:
In normalizePath(path, winslash = winslash, mustWork = mustWork) :
path[1]="output_report.Rmd": No such file or directory
scFeatures_associationstudy
. Please include it as a dependency in the DESCRIPTION
.scFeatures_associationstudy
(https://github.com/SydneyBioX/scFeatures/blob/80034c0ec37c206d63a08198e22dda351126b805/inst/extdata/output_report.Rmd#L228). This means it won't work on anyone else's computer.BugReports
field to the DESCRIPTION
(usually a link to the Issues page of the GitHub repo)..rds
/.Rdata
files in inst/extdata
in an inst/script/ directory
. See data documentation.
inst/extdata
is usually used for 'raw' data, so these data might properly belong under data/
rather than inst/extdata/
; see https://contributions.bioconductor.org/data.html.dev
and docs
folders? Please justify or remove them from the main branch of the git repo (Bioconductor requests that "Any files or directories for other applications (Github Actions, devtools, etc.) should ideally be in a different branch and not submitted to the Bioconductor version of the package." (see https://contributions.bioconductor.org/general.html?q=unnec#undesirable-files)scFeatures::run_*()
functions that underpin the wrapper scFeatures::scFeatures()
function.makeSeurat()
also accepts Seurat objects (https://github.com/SydneyBioX/scFeatures/blob/709f075578bf01b1823dc39fe1d5617472c3f888/R/wrapper_run_scfeatures.R#L225-L237), but this isn't documented. Please document when a user would need this functionality.scFeatures_detail
vignette you write, "the L values between the pairs of proteins are calculated using the L function defined in literature" but no reference is given to the relevant literature.scFeatures_detail
vignette is the advice, "This can be obtained from performing cell type prediction using reference data, for example, using SCTransform from Seurat (see https://satijalab.org/seurat/articles/spatial_vignette.html)". However, to my understanding, SCTransform is a normlization method, not a cell type prediction method, and the link points doesn't point to how to actually do the cell type prediction as best I can tell. Please clarify.BiocCheck::BiocCheck()
. Code styling notes can be regarded as suggestions, but other points should be followed or reasons given for not following them. * NOTE: Update R version dependency from 4.2.0 to 4.3.0.
* NOTE: Consider adding the maintainer's ORCID iD in 'Authors@R' with
'comment=c(ORCID="...")'
* NOTE: Avoid 1:...; use seq_len() or seq_along()
* NOTE: Use accessors; don't access S4 class slots via '@' in
examples/vignettes.
* NOTE: Consider adding runnable examples to man pages that document exported
objects.
* NOTE: Consider adding unit tests. We strongly encourage them. See
https://contributions.bioconductor.org/tests.html
inst/CITATION
file (see https://contributions.bioconductor.org/citation.html)@
or slot()
of S4 objects. Instead, use accessor functions. That said, my understanding is that Seurat authors have not followed this advice, so it's unavoidable when interacting with Seurat objects, but please ensure you are doing this when interacting with Bioconductor objects.remove_mito()
remove more than just mitochondrial genes? I strongly recommend choosing a more precise name for the function or split the functionality up into remove_mito()
, remove_ribo()
, etc.Thank you for the very detailed feedback! @ycao6928 and I are going to start working on it, and we will let you know when we have implemented the changes.
Dear @PeteHaitch, thank you for the very detailed feedback! We have fixed all the required issues and some of the suggested issues. The line by line comments are pasted below:
R CMD check
. Bioconductor expects that vignettes contain non-trivial evaluated code chunks and static vignettes are generally not acceptable.
R CMD check
if the vignette code was evaluated. Some examples of where I encountered errors when running the vignette code:> feature_gene_prop_celltype <- run_gene_prop_celltype(data_remove_mito, genes = genes_of_interest)
Error: BiocParallel errors
1 remote errors, element index: 1
1 unevaluated and other errors
first remote error:
Error: Under current subsetting parameters, the default assay will be removed. Please adjust subsetting parameters or change default assay.
> feature_CCI <- run_CCI(data , species = \"Homo sapiens\" )
Error in run_CCI(data, species = \"Homo sapiens\") :
could not find function \"run_CCI\"
genes_of_interest <- c(\"TIGIT\", \"PDCD1\")
feature_gene_prop_bulk <- run_gene_prop(data, genes = genes_of_interest )
Error: Under current subsetting parameters, the default assay will be removed. Please adjust subsetting parameters or change default assay.
scfeatures_result <- scFeatures(data,
# input selective feature types to generate.
feature_types = c(\"proportion_raw\", \"pathway_gsva\", \"L_stats\", \"gene_mean_celltype\", \"gene_prop_aggregated\") ,
# by default assumes data is \"scrna\" (single-cell RNA-seq), this is now set to \"spatial_p\" (spatial proteomics).
type = \"spatial_p\" ,
# by default assumes \"Homo sapiens\", this is now set to `Mus musculus`.
species = \"Mus musculus\" ,
# by default uses the 50 hallmark genes, now set to user specified pathways.
geneset = list(\"pathway_a\" = c(\"SMA\" , \"Vimentin\" ), \"pathway_b\" = c(\"B7H3\" ,\"FoxP3\" )) ,
# by default uses top variable genes to generate the celltype specific gene expression feature category,now set to user defined genes.
celltype_genes = data.frame(celltype = c(\"Macrophages\" , \"Mono/Neu\" ) , marker = c(\"CD3\" , \"p53\")) ,
# by default uses top variable genes to generate the overall aggregated gene expression feature category, now set to user defined genes.
aggregated_genes = c(\"CD3\" , \"p53\") ,
# When passing as SingleCellExperiment or SpatialExperiment, by default we use the assay stored in \"logcount\"
assay = \"norm\",
# By default we look for the sample info in \"sample\" column and the celltype info in \"celltype\" column
sample = \"imageID\",
celltype = \"cellType\",
# If users want to construct features from the spatial metrics category, by default we look for the \"x_cord\" and \"y_cord\" column
spatialCoords = c(\"x\", \"y\"),
# by default uses single core
ncores = 8)
Error: Cannot find 'cellType' in this Seurat object
[x] The man pages (documentation) is too terse and imprecise.
Each man page should include well-written:
Title, Description and Details (see https://r-pkgs.org/man.html#title-description-details)
Arguments (see https://r-pkgs.org/man.html#arguments)
Return value (see https://r-pkgs.org/man.html#return-value)
Examples (see https://r-pkgs.org/man.html#sec-man-examples)
To take one example, the documentation of process_data()
:
The 'Description' just repeats the title, which doesn't explain what pre-processing actually is.
There's no real description of arguments (what sort of object is data
, what sort of normalisation is applied when normalise = TRUE
, etc.)
The 'Value' section is incorrect (the return value is actually a Seurat object) and it should be explained what the returned object contains.
You might take a look at popular Bioconductor packages such as scater or edgeR for inspiration for the documentation (although it needn't be that detailed).
Some of the required documentation detail is in the scFeatures_detail
vignette, but it properly belongs in the man pages. The vignette should show users how to use the package as a whole and the man pages how to use individual functions.
Fixed: All exported functions now have Title, Description, Arguments, Return value.
Non - exported functions now have description.
[x] The 'Value' section of many man pages is incorrect. E.g., run_proportion_raw()
returns a data.frame but is documented to return 'a matrix of samples x features'. These details matter when it comes to documenting package code.
Fixed: Corrected the return value type.
[x] The 'Introduction' of the each vignette, especially the first or main vignette (which I take to be scFeatures_summary
) should read like an abstract; see https://contributions.bioconductor.org/docs.html#vignette-introduction
Fixed: Edited the introduction in scFeatures_summary
[x] Please give the vignettes informative titles and filenames; see https://contributions.bioconductor.org/docs.html#vignettes
[x] Please use BiocStyle for vignette formatting.
Fixed: All vignettes now use BiocStyle
.
[x] Simply removing ClassifyR from DESCRIPTION
is insufficient because the scFeatures_detail
vignette still uses it to demonstrate the utility of scFeatures. Now that ClassifyR is passing builds again, please re-add ClassifyR to the DESCRIPTION
if using it in the vignettes or elsewhere in scFeatures.
Fixed: Re-added ClassifyR
.
[x] RhpcBLASctl should not be needed or used in a package. Why is it needed (and hidden) in the vignette? Fixed It was added to fix a bug that is now forgotten, I removed all references.
[x] Why is inline needed in the vignettes? Fixed: Same as above. It is no longer needed.
[x] The formatting in the scFeatures_detail
vignette is a bit messy because it includes lots of verbose output (progress bars). Please revise.
Fixed: Messages and warnings have been removed from the output.
[x] In the scFeatures_detail
vignette you are subsetting a vector outside its bounds; please fix:
Fixed This has been fixed by including different data with the package.
[x] In the scFeatures_associationstudy
vignette, please use a temporary directory via tempdir()
, rather than the current working directory, to demonstrate report generation.
[x] That said, the report generation fails when using tempdir()
but should work.
> output_folder <- tempdir()
> run_association_study_report(scfeatures_result, output_folder )
Error in abs_path(input) : The file 'output_report.Rmd' does not exist.
In addition: Warning message:
In normalizePath(path, winslash = winslash, mustWork = mustWork) :
path[1]=\"output_report.Rmd\": No such file or directory
Fixed: We have now used tempdir()
rather than the current working directory. The no such file bug is also fixed.
[x] The org.Hs.eg.db package is an unspecified dependency used in the report generation demonstrated in scFeatures_associationstudy
. Please include it as a dependency in the DESCRIPTION
.
Fixed: Added org.Hs.eg.db
to suggests.
[x] There are hardcoded paths in the report demonstrated in scFeatures_associationstudy
(https://github.com/SydneyBioX/scFeatures/blob/80034c0ec37c206d63a08198e22dda351126b805/inst/extdata/output_report.Rmd#L228). This means it won't work on anyone else's computer.
Fixed: Removed hard-coded path in favour of system.file
call.
[x] Please add a BugReports
field to the DESCRIPTION
(usually a link to the Issues page of the GitHub repo).
Fixed: Added to the description.
[x] Bioconductor requires documentation of .rds
/.Rdata
files in inst/extdata
in an inst/script/ directory
. See data documentation.
Fixed: Added documentation for the example_scrnaseq.rds
.
inst/extdata
is usually used for 'raw' data, so these data might properly belong under data/
rather than inst/extdata/
; see https://contributions.bioconductor.org/data.html.
Fixed: We have tried this, but found the data
directory only allows .Rdata file instead of the .RDS format.
Once we convert to .Rdata, the file size expanded from under 3MB to 16MB and became too big to include in the package. Therefore we decided to keep the data in the inst/extdata
. [x] Please add a table of contents to each vignette. Fixed: Added
[x] All man pages should have runnable examples (see https://contributions.bioconductor.org/docs.html#examples) Fixed: Added examples to all exported functions.
[x] What are the dev
and docs
folders? Please justify or remove them from the main branch of the git repo (Bioconductor requests that \"Any files or directories for other applications (Github Actions, devtools, etc.) should ideally be in a different branch and not submitted to the Bioconductor version of the package.\" (see https://contributions.bioconductor.org/general.html?q=unnec#undesirable-files)
Fixed: The docs
folder is for the pkgdown website.
We have now removed the dev
folder.
[ ] It is strongly recommended to add unit tests. I would suggest starting with the individual scFeatures::run_*()
functions that underpin the wrapper scFeatures::scFeatures()
function.
[x] This may be personal preference, but the function outputs feel the wrong way around (with samples as rows and features as columns). This is the opposite of how 'rectangular' data are usually stored in Bioonductor, e.g., SummarizedExperiment, where rows are features (e.g., genes) and samples are columns. A side effect, is that this leads to very 'wide' objects that don't display very nicely when printed (at least that was my experience with the example data). I recommend at least documenting why you choose to return the results in this orientation. Fixed This is one of the reasons why scFeatures was created.' Rectangular' data are not suitable for statistical modelling, which requires standard sample by feature design matrices. The reason why the output is as such is that the purpose of the features is statistical modelling.
[x] makeSeurat()
also accepts Seurat objects (https://github.com/SydneyBioX/scFeatures/blob/709f075578bf01b1823dc39fe1d5617472c3f888/R/wrapper_run_scfeatures.R#L225-L237), but this isn't documented. Please document when a user would need this functionality.
Fixed: Documented the purpose in the the function description, as added examples of cases when user would need this functionality in the vignettes.
[x] Please try to cite relevant literature. E.g., in the scFeatures_detail
vignette you write, \"the L values between the pairs of proteins are calculated using the L function defined in literature\" but no reference is given to the relevant literature.
Fixed: Added references for Moran's I and L function.
[x] In the scFeatures_detail
vignette is the advice, \"This can be obtained from performing cell type prediction using reference data, for example, using SCTransform from Seurat (see https://satijalab.org/seurat/articles/spatial_vignette.html)\". However, to my understanding, SCTransform is a normlization method, not a cell type prediction method, and the link points doesn't point to how to actually do the cell type prediction as best I can tell. Please clarify.
Fixed: Update the link to https://satijalab.org/seurat/articles/spatial_vignette.html#integration-with-single-cell-data-1, which corresponds to the section using SCTransform for "probabilistic transfer of annotations from a reference to a query set".
[ ] Please consider NOTES raised by BiocCheck::BiocCheck()
. Code styling notes can be regarded as suggestions, but other points should be followed or reasons given for not following them.
* NOTE: Update R version dependency from 4.2.0 to 4.3.0.
* NOTE: Consider adding the maintainer's ORCID iD in 'Authors@R' with
'comment=c(ORCID=\"...\")'
* NOTE: Avoid 1:...; use seq_len() or seq_along()
* NOTE: Use accessors; don't access S4 class slots via '@' in
examples/vignettes.
* NOTE: Consider adding runnable examples to man pages that document exported
objects.
* NOTE: Consider adding unit tests. We strongly encourage them. See
https://contributions.bioconductor.org/tests.html
[ ] Consider adding a top-level README file.
[ ] Recommend adding a NEWS file (see https://contributions.bioconductor.org/news.html)
[x] Recommend adding a inst/CITATION
file (see https://contributions.bioconductor.org/citation.html)
Fixed: This is now added
[ ] Consider adding a package-level man page (see https://contributions.bioconductor.org/docs.html#package-level-documentation).
[ ] It's strongly recommended to avoid direct slot access with @
or slot()
of S4 objects. Instead, use accessor functions. That said, my understanding is that Seurat authors have not followed this advice, so it's unavoidable when interacting with Seurat objects, but please ensure you are doing this when interacting with Bioconductor objects.
[ ] Why does remove_mito()
remove more than just mitochondrial genes? I strongly recommend choosing a more precise name for the function or split the functionality up into remove_mito()
, remove_ribo()
, etc.
Thanks for providing your response, @ycao6928. I'm on leave until January 10 and will resume reviews then. Wishing you a good Christmas and New Year break.
@PeteHaitch Thank you and happy New Year to you too!
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]'
[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:
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.