Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adaptest #628

Closed
8 tasks done
wilsoncai1992 opened this issue Jan 29, 2018 · 49 comments
Closed
8 tasks done

adaptest #628

wilsoncai1992 opened this issue Jan 29, 2018 · 49 comments
Assignees
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution OK

Comments

@wilsoncai1992
Copy link

wilsoncai1992 commented Jan 29, 2018

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]'

  • I understand that by submitting my package to Bioconductor,
    the package source and all review commentary are visible to the
    general public.

  • I have read the Bioconductor Package Submission
    instructions. My package is consistent with the Bioconductor
    Package Guidelines.

  • 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.

  • My package addresses statistical or bioinformatic issues related
    to the analysis and comprehension of high throughput genomic data.

  • 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:

  • The 'devel' branch for new packages and features.
  • The stable 'release' branch, made available every six
    months, for bug fixes.
  • Bioconductor version control using Git
    (optionally via GitHub).

For help with submitting your package, please subscribe and post questions
to the bioc-devel mailing list.

@bioc-issue-bot
Copy link
Collaborator

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

@bioc-issue-bot
Copy link
Collaborator

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.

@bioc-issue-bot bioc-issue-bot added 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place and removed 1. awaiting moderation labels Jan 29, 2018
@mtmorgan
Copy link
Contributor

Please update your repository so that 'master' contains only the components corresponding to the package (e.g., move paper/ etc to a new branch).

@bioc-issue-bot
Copy link
Collaborator

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.

@wilsoncai1992
Copy link
Author

thanks @mtmorgan , I will remove them

@wilsoncai1992
Copy link
Author

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

@LiNk-NY
Copy link

LiNk-NY commented Jan 29, 2018

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

@wilsoncai1992
Copy link
Author

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?

@LiNk-NY
Copy link

LiNk-NY commented Feb 8, 2018

Yes, please do. Thanks.

@LiNk-NY
Copy link

LiNk-NY commented Feb 20, 2018

Hi Wilson, @wilsoncai1992

Any updates on implementing these changes?

Regards,
Marcel

@wilsoncai1992
Copy link
Author

hi Marcel @LiNk-NY ! We are actively implementing the changes on branches of adaptest. We will update by next Mon.

@LiNk-NY
Copy link

LiNk-NY commented Feb 22, 2018

Hi Wilson, @wilsoncai1992
Thanks for the update!
M.

@bioc-issue-bot
Copy link
Collaborator

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...

@bioc-issue-bot
Copy link
Collaborator

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.

@wilsoncai1992
Copy link
Author

hi Marcel @LiNk-NY ! The integrative features to the Bioconductor environment has been completed in our latest PR:
wilsoncai1992/adaptest#39

@bioc-issue-bot
Copy link
Collaborator

Received a valid push; starting a build. Commits are:

fb64198 fix broken plotting index
f94ae06 passing all checks
4afb708 fix Bioc bindings for SumExp
f497965 Merge pull request #38 from nhejazi/bioc fix BioC...
52f045b Merge pull request #39 from wilsoncai1992/bioc Bi...

@bioc-issue-bot
Copy link
Collaborator

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.

@bioc-issue-bot
Copy link
Collaborator

Received a valid push; starting a build. Commits are:

ce0a111 bump
0a5bb72 catch n_top = 1

@bioc-issue-bot
Copy link
Collaborator

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.

@LiNk-NY
Copy link

LiNk-NY commented Mar 9, 2018

Hi Wilson, @wilsoncai1992

Can you please respond the the individual items in the review?
Please state how you addressed these points.

Best regards,
Marcel

@wilsoncai1992
Copy link
Author

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

@wilsoncai1992
Copy link
Author

Dear Marcel @LiNk-NY just want to make sure we do not miss the contribution deadline for BioC 3.7?

@LiNk-NY
Copy link

LiNk-NY commented Mar 20, 2018

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

@LiNk-NY
Copy link

LiNk-NY commented Mar 20, 2018

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


adaptest #628

DESCRIPTION

  • Looks good
  • Use the release version of roxygen2

NAMESPACE

  • You may want to separate the "shinyprint" functionality into a different
    package?

R

  • Avoid using cryptic argument names, use names that are descriptives and
    helpful for the end user
  • The 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.
  • You've added support for SummarizedExperiment which is good 👍
  • Avoid the use of 1:n constructions
  • Use the allocate and fill method, e.g.,
    ls <- vector("list", ncol(adaptY_composition))
  • Avoid using grep to find colnames in cv_param_est. Let the user provide
    the colname directly from the argument.
  • Replace sapply calls with the more robust vapply
  • You shouldn't need to "wrangle" the supported 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.
  • A proper API should not use @ accessor functions. See:
    http://bioconductor.org/developers/how-to/efficient-code/

data

  • Provide descriptive names for your data,Y and A can mean anything.

vignette

  • It looks like you commented BiocStyle out.
  • Please add an installation chunk. It doesn't have to run.
  • Looks good overall.

@wilsoncai1992
Copy link
Author

Thank you @LiNk-NY Marcel! My team will address all your comments in the next few days

@bioc-issue-bot
Copy link
Collaborator

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 ...

@nhejazi
Copy link

nhejazi commented Mar 26, 2018

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:

  • "You shouldn't need to "wrangle" the supported 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."
  • "A proper API should not use @ accessor functions. See: http://bioconductor.org/developers/how-to/efficient-code/"
  • "The 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."
    • currently, we use the standard S3-style ad-hoc classed 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.

@bioc-issue-bot
Copy link
Collaborator

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.

@bioc-issue-bot
Copy link
Collaborator

Received a valid push; starting a build. Commits are:

f1f43d6 address minor BiocCheck items

@bioc-issue-bot
Copy link
Collaborator

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.

@LiNk-NY
Copy link

LiNk-NY commented Mar 27, 2018

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

@wilsoncai1992
Copy link
Author

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.

@bioc-issue-bot
Copy link
Collaborator

Received a valid push; starting a build. Commits are:

e4bcffb rm devel files; fix docs; rm dplyr dep; run stylr
1c483f5 Merge pull request #44 from nhejazi/bioc Minor Bi...

@bioc-issue-bot
Copy link
Collaborator

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.

@nhejazi
Copy link

nhejazi commented Mar 28, 2018

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.

@LiNk-NY
Copy link

LiNk-NY commented Mar 30, 2018

Great! Thanks for that.
Your package has been accepted.
Thank you for your submission to Bioconductor!

Best regards,
Marcel

@LiNk-NY LiNk-NY added 3a. accepted will be ingested into Bioconductor daily builder for distribution and removed 2. review in progress assign a reviewer and a more thorough review of package code and documentation taking place labels Mar 30, 2018
@bioc-issue-bot
Copy link
Collaborator

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!

@wilsoncai1992
Copy link
Author

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!

@mtmorgan
Copy link
Contributor

mtmorgan commented Apr 2, 2018

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:

  1. Add an SSH key to your github account
  2. Submit your SSH key to Bioconductor

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.

@mtmorgan mtmorgan closed this as completed Apr 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3a. accepted will be ingested into Bioconductor daily builder for distribution OK
Projects
None yet
Development

No branches or pull requests

5 participants