Closed jokergoo closed 8 months ago
Hi @jokergoo
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: BioCartaImage
Type: Package
Title: BioCarta Pathway Images
Version: 0.99.0
Date: 2023-09-22
Authors@R: person("Zuguang", "Gu", email = "z.gu@dkfz.de", role = c("aut", "cre"),
comment = c('ORCID'="0000-0002-7395-8709"))
Depends: R (>= 4.2.0)
Imports: magick, grid, stats, grDevices, utils
Suggests: testthat, knitr
biocViews: Software, Pathways, BioCarta, Visualization
Description: The core functionality of the package is to provide coordinates of genes
on the pathway image and to provide methods to add self-defined graphics to the genes of interest.
VignetteBuilder: knitr
URL: https://github.com/jokergoo/BioCartaImage
License: MIT + file LICENSE
NeedsCompilation: no
RoxygenNote: 7.2.3
Encoding: UTF-8
Roxygen: list(markdown = TRUE)
We do not allow data to be downloaded from github. IMAGE_BASE_URL = "https://jokergoo.github.io/BioCartaImage/image/"
Please upload data to a trusted server like zenodo, AWS S3 buckets, Microsoft Azure Data Lakes, etc. or minimally an institutional level server. Un-trusted sites like personal github and dropbox are not allowed.
I found the files are also hosted on https://data.broadinstitute.org/gsea-msigdb/msigdb/biocarta/human/. I will use it instead of GitHub.
Yes updating to grab directly from there will be better. Please let us know when the code has been updated and we will look over again.
It has already been updated.
Your package has been added to git.bioconductor.org to continue the pre-review process. A build report will be posted shortly. Please fix any ERROR and WARNING in the build report before a reviewer is assigned or provide a justification on why you feel the ERROR or WARNING should be granted an exception.
IMPORTANT: Please read this documentation for setting up remotes to push to git.bioconductor.org. All changes should be pushed to git.bioconductor.org moving forward. It is required to push a version bump to git.bioconductor.org to trigger a new build report.
Bioconductor utilized your github ssh-keys for git.bioconductor.org access. To manage keys and future access you may want to active your Bioconductor Git Credentials Account
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on the Bioconductor Build System.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details.
The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): BioCartaImage_0.99.1.tar.gz macOS 12.6.5 Monterey: BioCartaImage_0.99.1.tar.gz
Links above active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/BioCartaImage
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
A reviewer has been assigned to your package for an indepth review. Please respond accordingly to any further comments from the reviewer.
Received a valid push on git.bioconductor.org; starting a build for commit id: fffb3a81ce10a960867c8f2aec9a17a445743d5d
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on the Bioconductor Build System.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details.
The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): BioCartaImage_0.99.2.tar.gz macOS 12.6.5 Monterey: BioCartaImage_0.99.2.tar.gz
Links above active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/BioCartaImage
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
[ ] in the README, please add Bioconductor installation
instructions using BiocManager::install()
[ ] BiocCheck()
NOTE: update R version dependency from 4.2.0 to 4.3.0
(Bioc 3.18 will not be installable with anything less) in the DESCRIPTION
[ ] in the DESCRIPTION, consider adding a BugReports:
field referencing
https://github.com/jokergoo/BioCartaImage/issues
code/
[ ] appreciate there's testing (!), though coverage could be improved
(~66% with 6 hits per line according to devtools::test_coverage()
)
[ ] BiocCheck()
NOTE: avoid using =
for assignment and use <-
instead
[ ] nothing 'wrong', but validity of character string values such as in biocarta.R
lines 471+ is nicely done with match.arg()
(see below); this has the pros that
valid options are directly displayed in the function documentation,
and an automated (informative) error is thrown upon violation
(*note that valid choices can also be checked inside the function only,
and needn't be in the function definition, if that's preferred)
pos_by_polygon <- function(x, y, where=c("left", "right", "top",
"bottom","topleft", "topright", "bottomleft", "bottomright")) {
where <- match.arg(where)
}
switch()
, i.e.:switch(where,
left=c(min(x), mean(y)),
right=c(max(x), mean(y)),
...)
[ ] line 55: consider defining n <- length(unique(BC2ENTREZ$ENTREZ[BC2ENTREZ$BCID %in% x$genes]))
for clarity, and as to not go over the recommended 80-character line limit
[ ] lines 448+ could be written more succinctly as coords[c(1, 1, 3, 3, 1)]
[ ] function documentation is very minimal for various functions
(e.g., all_pathways
, genes_in_pathway
, ...); could you expand these a little?
e.g., rather than stating that "a vector of pathway IDs" is returned,
also write what type of pathways these are/where they come from,
and perhaps list the corresponding resource/reference?
[ ] consider adding a package help/.man package (BioCartaImage
), e.g.,
briefly summarizing the package's key functionality, hyperlinking functions etc.
[ ] please provide source information/a description of how the data/
were generated;
I think the corresponding scripts are already correctly placed in inst/scripts/
vignettes/
r BiocStyle::Biocpkg("BiocStyle")
for formatting with html_document
as rendering target, e.g.:output:
BiocStyle::html_document:
toc: true
toc_depth: 2
[ ] could you clarify: the print method of biocarta_pathway
object p
says
"35 nodes, 16 genes", however, I am getting length(unique(p$genes))
-> 13?
I see that the show method accessed the genes in the corresponding pathway,
however, I find this confusing confusing still; perhaps the show method could
display both (what's in the pathway and what's in the object, if that makes sense?)
[ ] the output of str(p)
is fairly messy and hard to interpret
(especially for less programmatically inclined users); instead, might it be
worth considering describing relevant elements (what's in $genes
, coords
, etc.)?
[ ] the grid.pathway()
outputs produce a lot of white space; would it be possible
to somehow 'prettify' this, e.g., using code chunk options/adjusting certain parameters?
(*I am not referring to the viewport, but rather the space introduced by knitr
defaults)
Received a valid push on git.bioconductor.org; starting a build for commit id: d1857f4992284b724f3aad3676f5e2ecd5152496
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on the Bioconductor Build System.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details.
The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): BioCartaImage_0.99.4.tar.gz macOS 12.6.5 Monterey: BioCartaImage_0.99.4.tar.gz
Links above active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/BioCartaImage
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Thank you for your time and your comments!
BiocManager::install()
BiocCheck()
NOTE: update R version dependency from 4.2.0 to 4.3.0
(Bioc 3.18 will not be installable with anything less) in the DESCRIPTIONBugReports:
field referencing
https://github.com/jokergoo/BioCartaImage/issuescode/
- [x] appreciate there's testing (!), though coverage could be improved (~66% with 6 hits per line according to
devtools::test_coverage()
)
I have tried my best to improve the code coverage. Now it is 83%.
- [ ]
BiocCheck()
NOTE: avoid using=
for assignment and use<-
instead
I prefer to use =
for assignment.
match.arg()
(see below); this has the pros that
valid options are directly displayed in the function documentation,
and an automated (informative) error is thrown upon violation
(*note that valid choices can also be checked inside the function only,
and needn't be in the function definition, if that's preferred)pos_by_polygon <- function(x, y, where=c("left", "right", "top",
"bottom","topleft", "topright", "bottomleft", "bottomright")) {
where <- match.arg(where)
}
switch()
, i.e.:switch(where,
left=c(min(x), mean(y)),
right=c(max(x), mean(y)),
...)
n <- length(unique(BC2ENTREZ$ENTREZ[BC2ENTREZ$BCID %in% x$genes]))
for clarity, and as to not go over the recommended 80-character line limitcoords[c(1, 1, 3, 3, 1)]
- [x] function documentation is very minimal for various functions (e.g.,
all_pathways
,genes_in_pathway
, ...); could you expand these a little? e.g., rather than stating that "a vector of pathway IDs" is returned, also write what type of pathways these are/where they come from, and perhaps list the corresponding resource/reference?
I have improved the function documentation as well as the vignette.
- [ ] consider adding a package help/.man package (
BioCartaImage
), e.g., briefly summarizing the package's key functionality, hyperlinking functions etc.
I think since all Bioc packages have vignettes (unlike CRAN packages), for me the first step is always to go to the vignettes. BioCartaImage is a small package, if I create a package-level rd file, it basically just copies text from the vignette, which I think for this package is not necessary.
- [x] please provide source information/a description of how the
data/
were generated; I think the corresponding scripts are already correctly placed ininst/scripts/
It is added in the vignette.
vignettes/
r BiocStyle::Biocpkg("BiocStyle")
for formatting with html_document
as rendering target, e.g.:output:
BiocStyle::html_document:
toc: true
toc_depth: 2
- [x] could you clarify: the print method of
biocarta_pathway
objectp
says "35 nodes, 16 genes", however, I am gettinglength(unique(p$genes))
-13? I see that the show method accessed the genes in the corresponding pathway, however, I find this confusing confusing still; perhaps the show method could display both (what's in the pathway and what's in the object, if that makes sense?)
It was indeed confusing. It should be not called "genes" because p$genes
are actually nodes or proteins in the pathway where the mapping to "real genes" is not always one-to-one. In the new version, I have changed the name "genes" to "bc" (bc
means internal biocarta ID for nodes/proteins). In this way, length(p$bc)
is the number of nodes in the pathway. Note here I did not apply unique()
because the term "nodes" is under the context of a network and multiple existences of the same node contribute to the topology of the network. Whereas when I measure the number of genes, I take the unique number of genes that are mapped to the pathway. Also explained in the vignette.
- [x] the output of
str(p)
is fairly messy and hard to interpret (especially for less programmatically inclined users); instead, might it be worth considering describing relevant elements (what's in$genes
,coords
, etc.)?
Now it is explained in the documentation of get_pathway()
as well as in the vignette.
grid.pathway()
outputs produce a lot of white space; would it be possible
to somehow 'prettify' this, e.g., using code chunk options/adjusting certain parameters?
(*I am not referring to the viewport, but rather the space introduced by knitr
defaults)This is on purpose. grid.pathway()
is not for making a high-level plot. It is for providing a graphic component for a larger plot, like grid.points()
. There are some examples where you see a lot of white spaces around. That is for demonstrating drawing the pathway in a sub-viewport which might be 1/4 of the parent viewport.
In the new version, I explicitly added the borders of the whole figure to show the white areas are parts of the figures that are demonstrated in the examples.
Thanks for the prompt response and addressing things! Couple more comments, with 1-2 being majorish, the other briefly clarifying earlier comments...
Regarding data/
source information ("It is added in the vignette."), this is not sufficient. Please also document the data/
in a way that is programmatically accessible (via ?
). Also, I don't find it intuitive where to find the corresponding documentation. E.g., searching for .RData
object BC2ENRTEZ, PATHWAY2BC etc. I don't get any matches in the vignette. The data is already being briefly described in terms for data structure (dimensions and variables) - what is missing is provenance information.
Some of the documentation I mentioned is still too minimal for my liking... E.g., it would be nice if ?genes_in_pathway
and ?get_pathway_image
(or any other function having a pathway
argument) included how valid pathways from different resources are retrieved; otherwise, how would one know (without typos or going online) what are valid values for pathway
? To link to useful information across .man pages, hyperlinks via \code{\link{...}}
or the #' @seealso
roxygen tag might be useful, as you are already doing in other help pages.
Similar to the above, the point of a ?BiocCartaImage
help page is not to replace the vignette, but to have users be able to have programmatic access (from within R) to a brief summary of the package, quick-links to key functions etc. - it is not a requirement, so you're call, just wanted to clarify why we recommend this :)
Regarding =
vs. <-
: Just to point out that this has less to do with what is "preferred", i.e., it is not a difference in syntax but semantics. Specifically, these operators can - in certain contexts - behave differently (!); hence we recommend refraining from using =
for assignment in all cases, although this is only a NOTE (for now).
Received a valid push on git.bioconductor.org; starting a build for commit id: 112aefbaeb44cd59880b04074f4892afcc925766
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on the Bioconductor Build System.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details.
The following are build products from R CMD build on the Bioconductor Build System: macOS 12.6.5 Monterey: BioCartaImage_0.99.5.tar.gz Linux (Ubuntu 22.04.2 LTS): BioCartaImage_0.99.5.tar.gz
Links above active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/BioCartaImage
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: daee920ce52cd3ed0293ca761204feb6dff811b6
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on the Bioconductor Build System.
Congratulations! The package built without errors or warnings on all platforms.
Please see the build report for more details.
The following are build products from R CMD build on the Bioconductor Build System: Linux (Ubuntu 22.04.2 LTS): BioCartaImage_0.99.6.tar.gz macOS 12.6.5 Monterey: BioCartaImage_0.99.6.tar.gz
Links above active for 21 days.
Remember: if you submitted your package after July 7th, 2020,
when making changes to your repository push to
git@git.bioconductor.org:packages/BioCartaImage
to trigger a new build.
A quick tutorial for setting up remotes and pushing to upstream can be found here.
Thanks for your reply!
Regarding data/ source information ("It is added in the vignette."), this is not sufficient. Please also document the data/ in a way that is programmatically accessible (via ?). Also, I don't find it intuitive where to find the corresponding documentation. E.g., searching for .RData object BC2ENRTEZ, PATHWAY2BC etc. I don't get any matches in the vignette. The data is already being briefly described in terms for data structure (dimensions and variables) - what is missing is provenance information.
I have added more text describing the source of these datasets in the documentations.
"searching for .RData object BC2ENRTEZ, PATHWAY2BC etc. I don't get any matches in the vignette". They should be able to be visited by ?BC2ENRTEZ
.
Some of the documentation I mentioned is still too minimal for my liking... E.g., it would be nice if ?genes_in_pathway and ?get_pathway_image (or any other function having a pathway argument) included how valid pathways from different resources are retrieved; otherwise, how would one know (without typos or going online) what are valid values for pathway? To link to useful information across .man pages, hyperlinks via \code{\link{...}} or the #' @seealso roxygen tag might be useful, as you are already doing in other help pages.
I added more text describing the valid format of pathways. I have checked all functions that accept pathways as input.
Similar to the above, the point of a ?BiocCartaImage help page is not to replace the vignette, but to have users be able to have programmatic access (from within R) to a brief summary of the package, quick-links to key functions etc. - it is not a requirement, so you're call, just wanted to clarify why we recommend this :)
Yes, I agree that is useful and necessary. I have added it in the package.
Regarding = vs. <-: Just to point out that this has less to do with what is "preferred", i.e., it is not a difference in syntax but semantics. Specifically, these operators can - in certain contexts - behave differently (!); hence we recommend refraining from using = for assignment in all cases, although this is only a NOTE (for now).
I know the difference between "=" and "<-" very clearly. But personal for me as an old programmer, I don't see the difference is big enough for me to change my coding style.
Thanks for the updates, it's all looking well. However, the package .man pages is currently not directly found (at least not how I tried viewing it at first, i.e., ?BioCartaImage
fails). You should add an @aliases
tag to fix this, specifically:
# in package.R:
#' @aliases BioCartaImage
I'll accept now as a package-help page is not a requirement. But would suggest adding this, since you already spent time putting together that piece of documentation.
Your package has been accepted. It will be added to the Bioconductor nightly builds.
Thank you for contributing to Bioconductor!
Reviewers for Bioconductor packages are volunteers from the Bioconductor community. If you are interested in becoming a Bioconductor package reviewer, please see Reviewers Expectations.
cannot build unless issue is open and has the 'pre-review' label or '2. review in progress' label, or is closed and has the 'TESTING' label.
cannot build unless issue is open and has the 'pre-review' label or '2. review in progress' label, or is closed and has the 'TESTING' label.
Thanks! I will add it.
The default branch of your GitHub repository has been added to Bioconductor's git repository as branch devel.
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/jokergoo.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("BioCartaImage")
. The package 'landing page' will be created at
https://bioconductor.org/packages/BioCartaImage
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.
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.