Closed itsvenu closed 5 years ago
Hi @itsvenu
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: ALPS
Title: AnaLysis routines for ePigenomicS data
Version: 0.99.0
Author: Venu Thatikonda, Natalie Jäger
Maintainer: Venu Thatikonda <thatikonda92@gmail.com>
Description: ALPS is an R package that provides tools for the analysis and visualization, mainly aimed at epigenomics data.
License: MIT + file LICENSE
Encoding: UTF-8
LazyData: true
RoxygenNote: 6.1.1.9000
Depends: R (>= 3.6)
Imports:
assertthat,
ChIPseeker,
corrplot,
data.table,
dplyr,
GenomicRanges,
GGally,
genefilter,
ggplot2,
ggseqlogo,
Gviz,
magrittr,
org.Hs.eg.db,
pals,
plyr,
reshape2,
rtracklayer,
stats,
stringr,
tibble,
tidyr,
TxDb.Hsapiens.UCSC.hg19.knownGene,
TxDb.Hsapiens.UCSC.hg38.knownGene
URL: https://github.com/itsvenu/ALPS
BugReports: https://github.com/itsvenu/ALPS/issues
Suggests:
knitr,
rmarkdown,
ComplexHeatmap,
circlize,
testthat
biocViews: Epigenetics, Sequencing, ChIPSeq, ATACSeq, Visualization
VignetteBuilder: knitr
A reviewer has been assigned to your package. Learn what to expect during the review process.
IMPORTANT: Please read the instructions for setting up a push hook on your repository, or further changes to your repository will NOT trigger a new build.
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details.
Hi @nturaga, the error seems to be from vignette which requires another package: https://github.com/itsvenu/ALPSdata. This package contains example data (>5MB). That's why I didn't provide it with ALPS
package. The data is coming from a published study.
R CMD build, check and BiocCheck are clean if I uncomment the code to install ALPSdata
. Should I push these modifications with a version bump or is it fine to commit changes with the same version.
Do you suggest I should also submit ALPSdata package to bioconductor? or should I host files somewhere and include code to download them within vignette?
Thank you.
If you want to build the package again on the Single package builder, you have to bump the version.
You should submit ALPSdata as an Experiment hub package. https://bioconductor.org/packages/devel/bioc/vignettes/ExperimentHub/inst/doc/CreateAnExperimentHubPackage.html.
Your software package cannot exceed 5MB in size.
Best,
Nitesh
Received a valid push; starting a build. Commits are:
2a61338 fix & bump v0.99.1
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, 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.
Received a valid push; starting a build. Commits are:
c0422f8 v0.99.2
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.
Hi @nturaga, I don't really get what the error in current build is related to? I ran build, check, bioccheck in windows, linux, macOS from my side. All are clear. Is it possible to provide more information about the error from your side, so that I can look into it?
Thanks.
Hi, don't worry about that one CHECK
issue on windows.
I would rerun the build report with a version bump and see if it happens again.
Best,
Nitesh
Received a valid push; starting a build. Commits are:
a11ef72 v0.99.4
Dear Package contributor,
This is the automated single package builder at bioconductor.org.
Your package has been built on Linux, Mac, and Windows.
On one or more platforms, the build results were: "skipped, ERROR". This may mean there is a problem with the package that you need to fix. Or it may mean that there is a problem with the build system itself.
Please see the build report for more details.
Received a valid push; starting a build. Commits are:
333b1ef v0.99.5
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.
hi @nturaga, I bumped the version number but the ERROR in windows is still present. It's very smooth when I run on windows system from my side. Also, there are other examples which utilizes the same code snippet, they didn't throw any error.
Is there any chance you can take a look into more details why is it happening on your windows server? So that I can move onto next steps?
Thank you!
Hi @nturaga, any update on this issue? Thanks.
So we have determined the windows is the 32bit specific ERROR as per the discussion on the mailing list. Please review despite the windows ERROR @itsvenu its up to you if you want to add the .BBSOption file for unsupporting 32bit for the time being or wait it out to see what rtracklayer says after submitting an issue report about it.
Hi @lshep, I will include the .BBSOption file for this release. As such I don't need to modify any code in the package, unless rtracklayer changes a lot to support win32 system. Once the issue is fixed, I will remove the BBS file in the next release. This would be best IMO. Should I add BBS file and update the package git repo?
Yes please git add the .BBSoptions
file sorry please use this exact caps and plural form for our system to recognize it with the line UnsupportedPlatforms: win32
this should prevent building on the 32 bit version of windows only. Then do a version bump in the DESCRIPTION to hopefully get a full build report. Thank you
My mistake on the wrong caps early please use .BBSoptions
Received a valid push; starting a build. Commits are:
441f7ab v0.99.6 & BBSoptions
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.
@lshep aw, finally seems like clean build report. Thanks for helping me out.
ok
ok
ok
[ ] There are no official guidelines but using %>% makes reading code that much harder from the reviewers stand point. The use of the pipe is not consistent in your package as well. I'm not suggesting removing them, but for a future package use them only if necessary. Using pipe operators within software will make it harder to debug as well if there is an error.
[ ] Avoid using slot accessors @
directly. Make a getter function if needed and use that in x@annotStat
.
[ ] Use BiocParallel package and find the appropriate function to fit
your needs, instead of parallel::mclapply
.
[ ] Is it worth declaring a new function with a confusing pipe operation
%||%
when you are using it exactly 2 times?
R/geom_flat_violin.R 45: data$width <- data$width %||% 46: params$width %||% (resolution(data$x, FALSE) * 0.9)
figures
folder in the man pages, but the figures
themselves don't seem to be used.[ ] Add the bioconductor way of installing packages with
BiocManager::install()
[ ] Don't supressMessages at library loading.
Once you fix these issues, i'll do a second review.
Received a valid push; starting a build. Commits are:
f2dda8a bioc-review v0.99.7
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.
Hi @nturaga, thanks for the review and comments. I addressed all the comments mentioned above
@
with getter function with attr
parallel
with BiocParallel
depends
in DESCRIPTION
fileman/figures
are used in the github README.md
manual file.README.md
manual file and is generated with README.Rmd
BiocManager::install()
supressMessages
Thanks.
Hello @itsvenu -
I'll be taking over the review of your package for the time being. Thank you for making the changes mentioned above. I do have some additional points that should be addressed before accepting the package. Please see my review points below. The required changes must be made while the suggested changes do not have to be but we strongly encourage them. Comment back here with updates that have been made and when the package is ready for a re-review.
[ ] REQUIRED: 'LazyData:' should be set to false. When this is true it can slow down the loading of packages with data.
[ ] REQUIRED: 'RoxygenNote:' should be 6.1.1 not 6.1.1.9000.
[ ] SUGGESTION: Consider adding automatically suggested biocViews:
The 'README-ALPS-overview' image is a very good overview of the package, well done!
pipe
@keywords internal
.[ ] REQUIRED: This is where DATARAW.R belongs. However, the code needs to be worked on so that other users can use the code and produce the same resuls you did. The paths are specific to your library which if someone else ran this code right now, it would error out.
[ ] REQUIRED: The scripts in this directory can vary. Most importantly if data was included in the inst/extdata/, a related script must be present in this directory documenting very clearly how the data was generated. It should include source urls and any important information regarding filtering or processing. It can be executible code, sudo code, or a text description. A user should be able to download and roughly reproduce the file or object that is present as data.
test-plot_correlation
test-plot_enrichment
test-plot_motif_logo
[ ] REQUIRED: It doesn't seem right to have your input data for most of your
functions be set to NULL. In some of the functions you use
assertthat::assert_that()
to kind of test if the data being read in is NULL
and would throw an error if it was, but in some you don't. A better approach
would be not to set a default value for the input data and then use
assertthat::assert_that()
to test if the input data is the format you require.
Something like assertthat::assert_that(is.data.frame(data))
.
[ ] REQUIRED: Is there a reason why you redefined %>%
and then export it?
You end up importing the pipe from magrittr, why not just use this in your code/
[ ] REQUIRED: Avoid 1:...; use seq_len()
or seq_along()
. Found in files:
[ ] SUGGESTION: Consider shorter lines, 176 lines are >80 characters long.
[ ] SUGGESTION: Consider multiples of 4 spaces for line indents, 327 lines are not.
Best, Kayla
Hi @Kayla-Morrell, thanks for the comments. I addressed above raised points. For some I added comments.
[x] REQUIRED: There is some reorganization of the repo that should be done. LICENSE.md file is not needed. Since the MIT license is distributed by R the only additional file that is needed in your repo is the LICENSE file.
[x] The README_files/ should be moved to an inst/figures/ directory. The man/figures/ directory should also go in the inst/figures/ directory.
[x] The directory data-raw/ should be removed and the file DATASET.R should be moved to an inst/script/ directory.
[x] REQUIRED: 'LazyData:' should be set to false. When this is true it can slow down the loading of packages with data.
I changed this to false
. However, from this resource, seems we better off keeping lazydata: true
, so the example datasets within the package does not occupy any memory, unless used. If a user wants to use only functions without running the examples given in the package's vignette, loading package doesn't occupy any memory. Or did I miss some additional details?
(Although, ALPS package doesn't have any such datasets. All examples need the data to be read from extdata
directory)
[x] REQUIRED: 'RoxygenNote:' should be 6.1.1 not 6.1.1.9000.
[x] SUGGESTION: Consider adding automatically suggested biocViews:
Transcription
[x] REQUIRED: There are many files in the inst/extdata/ directories that aren't utilized in the package (from what I could tell at least). Please make sure you are only including files that are used within the code, vignette, or examples.
All files are used in vignette's examples.
devtools::document()
is generating the Rd file with keyword
, although the utils-pipe.R
contains @keywords
. If I edit the pipe.Rd
file by hand to change keyword -> keywords, CMD build is raising a warning that keywords
is unknown. Could you please provide the source on which the keywords
suggestion is based on?
[x] REQUIRED: This is where DATARAW.R belongs. However, the code needs to be worked on so that other users can use the code and produce the same results you did. The paths are specific to your library which if someone else ran this code right now, it would error out.
[x] REQUIRED: The scripts in this directory can vary. Most importantly if data was included in the inst/extdata/, a related script must be present in this directory documenting very clearly how the data was generated. It should include source urls and any important information regarding filtering or processing. It can be executible code, sudo code, or a text description. A user should be able to download and roughly reproduce the file or object that is present as data.
test-plot_correlation
test-plot_enrichment
test-plot_motif_logo
Removed these test files.
Changed functions according to suggestions where appropriate.
Instead of importing pipe for every function, this thread suggests to define pipe in separate file and export it.
...
are not present in functions (not sure why BiocCheck throwing this NOTE)
get_variable_regions
multiBigwig_summary
plot_motif_logo
and in function plot_correlation
& plot_browser_tracks
, ...
was used in the context of ellipsis. I couldn't find any resource that ellipsis can be replaced by seq_len
or seq_along
(I doubt that these 2 functions can replace ellipsis?). Further discussion would be appreciated!
Thank you. Please let me know if there are additional comments.
Received a valid push; starting a build. Commits are:
ae83f51 bioc-review v0.99.8
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.
Hello @itsvenu,
Thank you for making the necessary changes. I have just a few more requirements as well as some responses to your comments. Please see the re-review below.
- [x] REQUIRED: 'LazyData:' should be set to false. When this is true it can slow down the loading of packages with data.
I changed this to
false
. However, from this resource, seems we better off keepinglazydata: true
, so the example datasets within the package does not occupy any memory, unless used. If a user wants to use only functions without running the examples given in the package's vignette, loading package doesn't occupy any memory. Or did I miss some additional details?
Bioconductor preferes to have this set to false, see notes about DESCRIPTION and data.
- [ ] REQUIRED: Almost got the internal keyword tag correct, it should be @Keywords internal.
devtools::document()
is generating the Rd file withkeyword
, although theutils-pipe.R
contains@keywords
. If I edit thepipe.Rd
file by hand to change keyword -> keywords, CMD build is raising a warning thatkeywords
is unknown. Could you please provide the source on which thekeywords
suggestion is based on?
Correct, this was my mistake. I didn't realize when you add @keywords
to the roxygen documentation it gets turned into \keyword
in the man page. This comment can be ignored.
- [ ] REQUIRED: Avoid 1:...; use seq_len() or seq_along(). Found in files: -get_variable_regions, line 55 -multiBigwig_summary, line 93 -plot_browser_tracks, line 89 -plot_correlation, line 58 -plot_motif_logo, line 45
...
are not present in functions (not sure why BiocCheck throwing this NOTE)
get_variable_regions
multiBigwig_summary
plot_motif_logo
and in function
plot_correlation
&plot_browser_tracks
,...
was used in the context of ellipsis. I couldn't find any resource that ellipsis can be replaced byseq_len
orseq_along
(I doubt that these 2 functions can replace ellipsis?). Further discussion would be appreciated!
Here are my proposed solutions...correct me if I'm wrong.
get_variable_regions
: idx <- order(-rv)[seq_long(num_regions)]
multiBigwig_summary
: for (i in seq_len(bwL)) {
plot_browser_tracks
: for (i in seq_len(all_sample_ids)) {
plot_motif_logo
: for (i in seq_len(x_rd2)) {
plot_correlation
doesn't have a fix and is fine as is.
Best, Kayla
Hi @Kayla-Morrell,
Thanks a lot for the quick response.
- [x] REQUIRED: Thank you for cleaning up the repo, however you did not move the README_files/ into an inst/figures/ directory. Also, the man/figures/ directory is still present and not in the inst/figures/ directory. We do not like to have anything besides man pages in the man/ directory. Please make this change.
README_files were used in github README.md file. The contents are same as package's vignette. I thought it's becoming redundant and completely removed this directory. I moved man/figures to inst/figures (sorry, I missed it somehow from previous commit).
- [x] REQUIRED: 'RoxygenNote:' should be 6.1.1 not 6.1.1.900, this change was not made. Please do so
This was automatically changed back to 6.1.1.900 when I ran devtools::document()
. Now fixed it.
Regarding ...
Thanks for suggesting the solutions. I implemented these changes and used either seq_along
or seq_len
depending on the context.
However, I can't use seq_*
in multiBigwig_summary
function. The bwL
is a BigWigFileList
class (from rtracklayer
) object and using seq_len
is producing an error (argument must be coercible to non-negative integer). Looks like it has to be for(i in 1:length(BwL))
.
Please let me know if there are additional comments/suggestions.
Thank you.
@itsvenu - Perfect, as soon as I see a commit and push, as well as a clean build I'll be happy to accept the package.
Received a valid push; starting a build. Commits are:
1655a8a bioc-review v0.99.9
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.
Your package has been accepted. It will be added to the Bioconductor Git repository and nightly builds. Additional information will be posed to this issue in the next several days.
Thank you for contributing to Bioconductor!
hi @Kayla-Morrell, great. Thanks a lot :)
The master branch of your GitHub repository has been added to Bioconductor's git repository.
To use the git.bioconductor.org repository, we need an 'ssh' key to associate with your github user name. If your GitHub account already has ssh public keys (https://github.com/itsvenu.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("ALPS")
. The package 'landing page' will be created at
https://bioconductor.org/packages/ALPS
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 that a minimum requirement for package acceptance is to pass R CMD check and R CMD BiocCheck with no ERROR or WARNINGS. Passing these checks does not result in automatic acceptance. The package will then undergo a formal review and recommendations for acceptance regarding other Bioconductor standards will be addressed.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x] I am committed to the long-term maintenance of my package. This includes monitoring the support site for issues that users may have, subscribing to the bioc-devel mailing list to stay aware of developments in the Bioconductor community, responding promptly to requests for updates from the Core team in response to changes in R or underlying software.
I am familiar with the essential aspects of Bioconductor software management, including:
For help with submitting your package, please subscribe and post questions to the bioc-devel mailing list.