Closed wilsoncai1992 closed 6 years ago
Hi @wilsoncai1992
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: adaptest
Title: Data-adaptive test statistics for multiple testing in high dimensions
Version: 0.8.2
Authors@R: c(
person("Weixin", "Cai", email = "wcai@berkeley.edu",
role = c("aut", "cre", "cph"),
comment = c(ORCID = "0000-0003-2680-3066")),
person("Nima", "Hejazi", email = "nhejazi@berkeley.edu",
role = "aut",
comment = c(ORCID = "0000-0002-7127-2789")),
person("Alan", "Hubbard", email = "hubbard@berkeley",
role = "ctb",
comment = c(ORCID = "0000-0002-3769-0127"))
)
Description: Data-adaptive test statistics represent a general methodology for
performing multiple testing on effects sizes under high-dimensional settings,
using the approach of data-adaptive statistical target parameters and
inference.
Depends:
R (>= 3.0.0)
License: GPL-2
URL: https://github.com/wilsoncai1992/adaptest
BugReports: https://github.com/wilsoncai1992/adaptest/issues
Encoding: UTF-8
LazyData: true
Imports:
tmle,
origami (>= 0.8.2),
SuperLearner,
calibrate,
magrittr,
Matrix,
future,
R2HTML
Suggests:
testthat,
rmarkdown,
knitr,
earth,
gam,
nnls
Remotes:
github::jeremyrcoyle/origami
VignetteBuilder: knitr
RoxygenNote: 6.0.1.9000
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.
Please update your repository so that 'master' contains only the components corresponding to the package (e.g., move paper/ etc to a new branch).
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.
thanks @mtmorgan , I will remove them
For the build error, it is due to my using a bleeding-edge version of origami
. my co-author, nima hejazi, will work on bumping the origami
version
Hi Wilson, @wilsoncai1992
It seems to me like your package does not provide any integrative features to the Bioconductor environment.
Please see the link below for a brief overview of potential classes to provide functionality for. http://bioconductor.org/developers/how-to/commonMethodsAndClasses/
Regards, Marcel
Thank you Marcel @LiNk-NY for bringing this up! In fact a wrapper facing SummarizedExperiment::SummarizedExperiment()
is almost ready and will be soon pushed to master
. Once that wrapper is ready, I will ping you for proceeding?
Yes, please do. Thanks.
Hi Wilson, @wilsoncai1992
Any updates on implementing these changes?
Regards, Marcel
hi Marcel @LiNk-NY ! We are actively implementing the changes on branches of adaptest
. We will update by next Mon.
Hi Wilson, @wilsoncai1992 Thanks for the update! M.
Received a valid push; starting a build. Commits are:
d0ad442 add bioc wrapper sketches c4484eb initial sketch of bioc wrappers c30a545 Merge branch 'bioc' of github.com:wilsoncai1992/ad... 4b54b69 add bioc reqs 5d397c5 biocviews and news 6cdadd4 bump R fbc9d54 misc 65eac69 update docs; add data.table 2cb7e19 4 spaces c7f6f1a wrap lines e5733fd wrap vignette 975f4a9 wrap line f723d4e some changes 304f956 unbreak the breakage 4492160 manual merge c3776fb Merge pull request #36 from nhejazi/bioc Bioc upd... ad0bc0b indent fix 6fae101 minor updates 7104724 Merge branch 'bioc' of github.com:wilsoncai1992/ad... 48bc8d0 temp c4f0573 something c2a13a4 temp ab4948a more temp 5f7ae6a passing build 37e463d more examples 5dc4c37 more values and example 24dbb1e more examples 98de416 bump version 20d31f8 sumexp additions 53d1652 manual merge fcf0d09 minor fixes for bioc wrappers 788b13c fix imports f7b4ef5 Merge pull request #37 from nhejazi/bioc more Bio...
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 Marcel @LiNk-NY ! The integrative features to the Bioconductor environment has been completed in our latest PR: https://github.com/wilsoncai1992/adaptest/pull/39
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.
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 Wilson, @wilsoncai1992
Can you please respond the the individual items in the review? Please state how you addressed these points.
Best regards, Marcel
Hi Wilson, @wilsoncai1992 Just a reminder to post your response to the review here. Best regards, Marcel
Dear Marcel @LiNk-NY , thanks for reminder! I am actively preparing a response and will post them before the end of the week! Thanks again for your patience! Best regards, Wilson
Dear Marcel @LiNk-NY , I confirm that all issues that all BIOC requirements have been met, and that I would love to request a full review on the package. Thank you!
Hi Wilson, @wilsoncai1992
It seems to me like your package does not provide any integrative features to the Bioconductor environment.
Please see the link below for a brief overview of potential classes to provide functionality for. http://bioconductor.org/developers/how-to/commonMethodsAndClasses/
Regards, Marcel
Dear Marcel, now there is a BIOC integrating wrapper called bioadaptest
in the package. An example of using the wrapper can be found in the vignette! The wrapper is facing the SummarizedExperiment::SummarizedExperiment()
object class.
Received a valid push; starting a build. Commits are:
7c3bdea 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.
Received a valid push; starting a build. Commits are:
d3a7b3c bioc require version #
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.
Received a valid push; starting a build. Commits are:
59e4ce6 bioc require higher R
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.
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. Dear @LiNk-NY Marcel, glad to inform you that all builds are passing
Dear Marcel @LiNk-NY just want to make sure we do not miss the contribution deadline for BioC 3.7?
Hi Wilson, @wilsoncai1992
We have not missed it. Please see: http://www.bioconductor.org/developers/release-schedule/ I will post the review soon.
Regards, Marcel
Hi Wilson, Thank you for your submission. I have reviewed your package. Please see below.
If you have any questions, don't hesitate to ask.
Best regards, Marcel
roxygen2
data_adapt
class is based on a list which is not the best structure
to use as you can't really enforce any validity checks to the components of
the class as easily as you would with an S4 class.SummarizedExperiment
which is good :+1:1:n
constructionsls <- vector("list", ncol(adaptY_composition))
grep
to find colnames in cv_param_est
. Let the user provide
the colname directly from the argument.sapply
calls with the more robust vapply
SummarizedExperiment
class
for your main function, it should just work with it as part of the integration
process. In other words, it should make use of the SummarizedExperiment
API
rather than the assay(x); t(x)
and rownames(x) <- colnames(x) <- NULL
version. @
accessor functions. See:
http://bioconductor.org/developers/how-to/efficient-code/Y
and A
can mean anything.BiocStyle
out.Thank you @LiNk-NY Marcel! My team will address all your comments in the next few days
Received a valid push; starting a build. Commits are:
9bf90a3 fix sapply / vector allocation; add author details... 5ef2398 req dplyr, not magrittr 1214c3b fix leftover magrittr call; internalize adaptest_o... 1bc90d5 catch non-SummarizedExperiment case in adaptest 54e6a45 add accessor; adjust rank_ttest; minor other chang... 7f37a9e Merge pull request #43 from nhejazi/bioc Updates ...
Hello @LiNk-NY ---
I am @wilsoncai1992's co-author on this package. I'm just writing to let you know that we have addressed nearly all of the review items you kindly pointed out in your recent review. About half of the items were address in adaptest
/41 while the remainder were addressed in adaptest
/43. Here are a few specifics on how major review items have been resolved (all addressed items were dealt with as part of adaptest
/41 and adaptest
/43) and an exception that we would like to the current review:
SummarizedExperiment
class for your main function, it should just work with it as part of the integration process. In other words, it should make use of the SummarizedExperiment
API rather than the assay(x)
; t(x)
and rownames(x) <- colnames(x) <- NULL
version."
adaptest
function process RangedSummarizedExperiment
objects differently from data.frame
or Matrix
, making the core analysis function more extensible in the sense that it now deals with base-R and Bioconductor classes in very similar ways (#43)@
accessor functions. See: http://bioconductor.org/developers/how-to/efficient-code/"
get_results_adaptmle
accessor function in #43. Please note that this is an internal function (and marked as such), with the main goal being to simply add data analytic results to the adapTMLE
class (which we sub-class from SummarizedExperiment
) in a clean, efficient, and read-able manner.data_adapt
class is based on a list
which is not the best structure to use as you can't really enforce any validity checks to the components of the class as easily as you would with an S4 class."
list
as the core structure (despite, having authored other Bioconductor packages and agreeing that this is v. poor practice) for across-community compatibility of the statistical routine exposed by this software. It is our aim to make this new statistical technique easy to use for the Bioconductor community via the bioadaptest
wrapper function while still making the core software easy to inspect for those well-versed in the methodology produced by our statistical community but yet unfamiliar with Bioconductor and related best practices (though we do hope to slowly remove this part of the code base and replace with S4, or maybe R6, as the package picks up users). Would it be acceptable to delay this particular item until a future release? With some input from you, we could definitely make this happen -- very easily and quickly, certainly in time for BioC 3.8 later this year (~Nov., I think).Please do let us know if there are any further concerns, as we would be quite happy to resolve them prior to the BioC 3.7 deadline of 9 April. Looking forward to your feedback.
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.
Received a valid push; starting a build. Commits are:
f1f43d6 address minor BiocCheck items
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 Wilson and Nima, @wilsoncai1992 and @nhejazi
Thank you for making those changes! It's looking better.
I've noticed that you're using the magrittr pipe %>%
.
I think you can do without the dependency since you're only using it for ~6 lines in the code.
I saw that you haven't made any changes to some argument names that are hard to figure out.
Namely, arguments Y
and A
would be better if they were named biomarkers
, treatment
or
something to that effect.
Other than that, it looks good.
Best regards, Marcel
Hi Wilson and Nima, @wilsoncai1992 and @nhejazi
Thank you for making those changes! It's looking better.
I've noticed that you're using the magrittr pipe %>%. I think you can do without the dependency since you're only using it for ~6 lines in the code.
I saw that you haven't made any changes to some argument names that are hard to figure out. Namely, arguments Y and A would be better if they were named biomarkers, treatment or something to that effect.
Other than that, it looks good.
Best regards, Marcel
Thank you @LiNk-NY Marcel! your latest comments will be quickly addressed.
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 @LiNk-NY ---
Thanks for the quick follow-up on this. Finding that unnecessary dplyr
dependency was great --- looks like we were only using it in a script that belonged on a development branch and importing it needlessly in a few other places. It's been completely removed as of the latest commits that built on the Bioconductor machines.
With respect to the argument names (e.g., A, Y), I assume you're referring to the adaptest
function. For this particular function, we intentionally did not change the names away from (W, A, Y), since this is the notation most commonly used in the Targeted Learning (TL) literature, on which the technique that adaptest
implements is based. In order to keep the package friendly/accessible to those in the TL community, we are trying to be consistent with notation (e.g., matching the drtmle package). As mentioned above, those in the Bioconductor community should not use this function and instead use the bioadaptest
function, which has argument names that ought to be easily interpretable. Maintaining this distinction ensures that those in the TL community who want to employ this data-adaptive testing strategy outside of the context of computational biology are able to do so (without thinking in terms of different notation) while having no effect on our intended audience in the Bioconductor community. Hope this clears up our perspective.
Please feel free to let us know if there's any more that we can do to make adaptest
Bioconductor ready/friendly. Finally, thank you very much for the time spent on this thorough review.
Great! Thanks for that. Your package has been accepted. Thank you for your submission to Bioconductor!
Best regards, Marcel
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!
Great! Thanks for that. Your package has been accepted. Thank you for your submission to Bioconductor!
Best regards, Marcel
Thank you @LiNk-NY Marcel! This is great news!
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/wilsoncai1992.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 biocLite(\"adaptest\")
. The package 'landing page' will be created at
https://bioconductor.org/packages/adaptest
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.