Closed IoannisVardaxis closed 6 years ago
Hi @IoannisVardaxis
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: MACPET
Type: Package
Title: Model based analysis for ChIA-PET data
Version: 0.99.0
Date: 2017-25-10
Author: Ioannis Vardaxis
Maintainer: Ioannis Vardaxis <ioannis.vardaxis@ntnu.no>
Description: The MACPET package can be used for binding site analysis for ChIA-PET data. MACPET reads ChIA-PET data in BAM or SAM format and separates the data into Self-ligated, Intra- and Inter-chromosomal PETs. Furthermore, MACPET breaks the genome into regions and applies 2D mixture models for identifying candidate peaks/binding sites using skewed generalized students-t distributions (SGT). It then uses a local poisson model for finding significant binding sites. MACPET is mainly written in C++, and it supports the BiocParallel package.
License: GPL-3
Encoding: UTF-8
LazyData: true
ByteCompile: true
RoxygenNote: 6.0.1
biocViews: Software, DNA3DStructure, PeakDetection, StatisticalMethod, Bayesian, Clustering, Classification, HiC
Depends: R (>= 3.4.2), InteractionSet (>= 1.4.0)
Imports: intervals (>= 0.15.1), plyr (>= 1.8.4), Rsamtools (>= 1.28.0), stats (>= 3.4.2), utils (>= 3.4.2), methods (>= 3.4.2), GenomicRanges (>= 1.28.6), S4Vectors (>= 0.14.7), IRanges (>= 2.10.5), GenomeInfoDb (>= 1.12.3), gtools (>= 3.5.0), GenomicAlignments (>= 1.12.2), BSgenome (>= 1.44.2), knitr (>= 1.17), Rcpp (>= 0.12.13), rtracklayer (>= 1.36.6), BiocParallel (>= 1.10.1)
Suggests: ggplot2 (>= 2.2.1), igraph (>= 1.1.2), rmarkdown (>= 1.6.0.9004), reshape2 (>= 1.4.2), BiocStyle (>= 2.4.1)
VignetteBuilder: knitr
RdMacros: Rdpack
LinkingTo: Rcpp
Collate:
'ALLData.R'
'AllClasses.R'
'AnalysisStatistics.R'
'ConvertToPSelf-methods.R'
'InputChecks.R'
'MACPET_pkg.R'
'PeakCallerUlt.R'
'PeaksToGRanges-methods.R'
'PeaksToNarrowPeak-methods.R'
'RcppExports.R'
'Stage_0_PETClassificationFunctions.R'
'Stage_1_PeakFinderFunctions_R.R'
'TagsToGInteractions-methods.R'
'exportPeaks-methods.R'
'plot-methods.R'
'summary-methods.R'
Your package has been approved for building. Your package is now submitted to our queue.
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: "WARNINGS, 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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171026041125.html
Hey,
Should I ask questions here on bioc-devel@r-project.org?
I am trying here first.
I have set up a push hook on your repository I think. I did what is on the instructions. (sorry i havent done this before and it takes time to understand.)
I have some questions for the errors I got, because I dont get any when I build or check the package on my MAC.
On the build report it says that "unknown macro '\insertRef'" somewhere at the beginning. I include a reference file for an article, how can I avoid this warning? I am not aware of any other macro to include references in roxygene.
A little further down I get an error " error: round is not a member of std", I didnt have any such error before in the algorithm and no problems with std for c++ in general. However I checked again now and for functions for std::log, std::round and std::ceil I get error that the "the call to function is ambigious". the header of my c++ script is:
using namespace Rcpp;
Should I change that somehow?
Finally, when I make all the changes locally at my MAC and then push the changes, will this automatically update the package and trigger a new check or do I need to do something more?
Thanks for all your held and time!
Hi bioc-devel
is better venue for this.
Thanks :)
Received a valid push; starting a build. Commits are:
037f544 Updated version from 0.99.0 to 0.99.1 because I fo...
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, 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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171027135637.html
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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171027154603.html
Received a valid push; starting a build. Commits are:
5e202cb Removing the Rdpack package and updating version t...
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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171027164238.html
Hey,
Now that the package is at stage 2. progress, should I do anything or just wait for a response?
HI @IoannisVardaxis I will review your package this week and post a review.
Ok!
Thanks:)
Ioannis Vardaxis Stipendiat NTNU Sendt fra min iPhone
HI @IoannisVardaxishttps://github.com/ioannisvardaxis I will review your package this week and post a review.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/Bioconductor/Contributions/issues/538#issuecomment-340539430, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AWOL_5t2l1hVIwkvqoi-X3c1nCS8AiEKks5sxhSogaJpZM4QG77p.
Hi @nturaga , have you checked the package?
Your review will be posted by tomorrow.
How is it going with the review?
R CMD Build: ok
R CMD INSTALL: ok
Quitting from lines 393-401 (MACPET.Rmd)
Error: processing vignette 'MACPET.Rmd' failed with diagnostics:
GenomePkg is not installed! See ??BSgenome::installed.genomes
Execution halted
The problem with this error, is it doesn't tell me what package is missing but instead says GenomePkg
.
The vignette also needs more detail on how to use the classes, and what use cases they can handle. right now, it looks mostly like an the classes and a few plots. Try to write up the vignette like you did the last section in Peak calling workflow
.
Explain the functions which are being called and "why". Explain the plots as well, i.e, your barplots and heatmaps. This will help new users.
Also think about producing the vignette so that the text wraps around or format the PDF to fit the width of the page. This will make it easier for users to follow.
Please use seq_len
or seq_along
instead of 1:length(x)
or 1:x
. Eg: Stage_1_PeakFinderFunctions_R.R l355
for(chri in 1:length(Peaks.Info_list))
I suggest a code wide search of wherever you using this pattern, and replace as needed.
Instead of if(class(x.self)=="PSelf"
AnalysisStatistics.R,l130. You can use
is(x.self, "PSelf)
You can do this across your package.
A few things regarding this block of code AnalysisStatistics.R: l181
#print:
cat(paste(rep("-",25),collapse=""))
cat("\n PETs Counts Summary \n")
cat(paste(rep("-",25),collapse=""))
Use message()
instead of cat()
so that diagnostic messages go to stderr rather than stdout, if that is your intention.
You don't need cat(paste(
, you can just use cat
. cat(rep("-",25),sep="")
will work just fine. Using paste
is not needed.
Same thing with return(cat("...
and also stop(paste(..
. There is no need to use paste
.
Do this throughout your code as there are multiple instances.
You use rm(list=ls())
, in PeakCallerUlt.R. This is not needed. R does this after every function be default.
It would be really helpful to review the code better if you used this package formatR
to format your code.
Maybe you meant "heatmap" summary-methods.R L181, L243
stop("hitmap has to be logical!")
Thanks! I will push the new version as soon as i am done with the changes. What do you mean with : " indent your C++ code better to make it more readable" on the last point? Also, I would like to add something to the sysdata.rda, and this will update some Rd files. Is it ok to do it know?
Received a valid push; starting a build. Commits are:
10907ea Version update to 0.99.4 54d137d Adding "is" function from method package 013cfec Changes on R codes and reformating using formatR p... 65a7a68 Adding some new genomes for the black list. d487d04 Documenting the new genomes in sysdata.Rda. Also r... 5d6c727 Removing commented code. afe057a Adding more documentation to vignette. Also fixing...
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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171115084344.html
Received a valid push; starting a build. Commits are:
68df52c Updating R version to 3.5, also the MACPET version...
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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171115085404.html
I made all the changes you suggested. For the vignette I wrote a little more, however I think everything is well explained. I can surely make more changes if you think that it is not enough.
Hi @nturaga , have you checked the latest push? I dont know if you have checked the c++ code yet, but I need to make some small changes to the code. Basically I need to add a small function, so the change is not big at all, but it is important.
Furthermore, the package takes as input a BAM or SAM file of the mapped sequences. The user himself has to map the fastq files and filter them using bowtie for example. I was thinking that since Bioconductor has packages for sequence mapping etc, maybe I could add them to the package to make it whole so the user does not need to refer to other algorithms prior using mine. Basically I will add another step to the whole analysis with a new R/c++ module for doing what i wrote above. Is it ok if I do it at this point?
Yes, go ahead. I can only re-check your package after Nov 28th.
Perfect, you can wait until I send the next push. It might be after Nov 28th.
Hi @IoannisVardaxis Please issue a version bump, so that your latest changes can build.
Received a valid push; starting a build. Commits are:
6098a1f Renaming State 0 to Stage 2 and Stage 1 to Stage 3 909ce06 Renaming to MACPETUlt 16be1ef Renaming the data files 94cd103 Small changes to those functions and their Rmb fil... 549c115 Deleting old Rd files 46ebd48 Now C++ codes, the Stage_3 was there before, just ... 86f354b Changes in the vignette fc68ad9 Introducing new data and just chaning names in pre... ef54f30 Small changes to Rd files for the data 2de1d51 New function b15ea72 Updated names for the previous stages 0 and 1 8c8204c New stages 0 and 1 for mapping and filtering 7fe9ed9 New name for the PeakCaller, main function 9ae875d Update in the description and Namespace
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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171212045825.html
Hey, I sent the new updates. I didnt change much on the old functions, but I made two new big scripts so the rest of the functions had to be updated a little. I also wrote more in the vignette, but I could write even more if you want.
A few things regarding this block of code AnalysisStatistics.R: l165, do you need the print statements?
Consider message
instead of cat
, everywhere.
Try using "futile.logger" instead of writing to a LogFile seperately. That will save you a lot of lines and you don't have to reinvent the wheel.
In ConvertToPE_BAM.R
This shouldn't happen in a function where you supress all warnings.
options(warn = -1)
invisible(Rsamtools::sortBam(file = BAM_file_2, destination = BAM_file_2.sorted,
byQname = FALSE))
options(warn = 0)
There really is no point in making these calls invisible
,
invisible(Rsamtools::filterBam(file = BAM_file_1, index = BAM_file_1.bai, destination = BAMfilt1,
param = SBparam, indexDestination = FALSE))
If you really want to supress warnings use suppressWarnings()
. The other bad practice is to adjust the global options. Instead of options(warn = -1); options(warn = 0)
(which changes as user specified ooptions(warn = 1)
) capture the old value and reset to that (oopt = options(warn = -1); options(oopt)
).
You do a good job of using seq_len, and seq_along eveywhere. You missed a few though, eg. Stage_3_PeakFinderFunctions.R: l 477
1:nrow(Peaks_Info_x)
For the AnalysisStatistics I would prefer having it as cat because the function works as a summary, if I use message then everything will be in orange letters which I dont want. But It is not important for me so I could change it if you want.
The reason I used options(warn=-1) is that suppressWarnings() did not work. I would also like to avoid it. Maybe it has something to do with the R-devel, i dont know.
I was making the calls invisible because they return a directory, which I dont really need. I will remove them.
@IoannisVardaxis When you are ready, please bump the version and let the package build again. I'll take that as a sign to review your package again.
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 following build report for more details:
http://bioconductor.org/spb_reports/MACPET_buildreport_20171219105237.html
I made the updates except from the "futile.logger" which I couldnt figure out how to create a log file and how to write in it actually.
Hi @IoannisVardaxis Did you take a look at this? https://cran.rstudio.com/web/packages/futile.logger/README.html
The section you want is flog.appender
To change the appenders assigned to a logger, use flog.appender():
# Change the 'quiet' logger to write to a file
flog.appender(appender.file('quiet.log'), 'quiet')
flog.warn("Goodbye, %s", "world", name='quiet')
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:
0e7c9e2 Version bump
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.
I made the changes with the "write" to "futile.logger". However I get an error when I push the new version:
It seems the error is in the lapply function all of a sudden. I dont get any error when I build the package on my MAC though. I tried to find what causes the error but I cant.
Received a valid push; starting a build. Commits are:
d568768 Version update and trying to fix the encoding erro...
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.
I have been trying to find and fix the error I get with no success. I think the problem is with the encoding, however I havent changed anything like that. The problem came after i changed the write function with the futile.logger function but I dont understand if it that which causes the error.
Hi @nturaga , Can you please help me with the error so that I can send a valid push?
Hi @IoannisVardaxis ,
This was an error on our end. We have issued a fix.
I would suggest a version bump for a new build tomorrow.
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.
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.