Bioconductor / Contributions

Contribute Packages to Bioconductor
131 stars 33 forks source link

proBatch submission #958

Closed ChloeHJ closed 5 years ago

ChloeHJ commented 5 years ago

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

bioc-issue-bot commented 5 years ago

Hi @ChloeHJ

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: proBatch
Type: Package
Title: Tools for Batch Effects Diagnostics and Correction
Version: 1.0.0
Author: Jelena Cuklina <chuklina.jelena@gmail.com>, Chloe H. Lee <chloe.h.lee94@gmail.com>, Patrick Pedrioli <pedrioli@gmail.com>
Maintainer: The package maintainer Jelena Cuklina <chuklina.jelena@gmail.com>
Description: The proBatch package facilitates batch effects analysis and correction
    in high-thoughput experiments. Although the package has primarily been developed 
    for mass-spectrometry proteomics (DIA/SWATH), it should also be applicable 
    to most omic data with minor adaptations.
    The package contains functions for diagnostics (proteome/genome-wide and feature-level),
    correction (normalization and batch effects correction) and quality control.
    Diagnostics part of the package features unified color scheme for plotting, 
    that allows to produce publication-quality graphs.
    Correction functions are convenient wrappers for common normalization and 
    batch effects removal approaches such as quantile normalizetion and median centering. 
    Furthermore, the package includes non-linear fitting based approaches to deal 
    with complex, mass spectrometry-specific signal drifts.
    Quality control step, mostly based on correlation analysis, allows to assess whether 
    the correction improved the quality of the data.
    All steps of batch effects analysis and correction are illustrated in the vignette,
    using the subset of real-world large-scale dataset.
biocViews: BatchEffect, Normalization, Preprocessing, Software, MassSpectrometry,Proteomics, QualityControl
License: GPL-3
URL: https://github.com/symbioticMe/proBatch
BugReports: https://github.com/symbioticMe/proBatch/issues
Depends: R (>= 3.5.0)
Encoding: UTF-8
LazyData: true
Imports:
    Biobase,
    corrplot,
    dplyr,
    data.table,
    ggfortify,
    ggplot2,
    lazyeval,
    lubridate,
    magrittr,
    pheatmap,
    preprocessCore,
    purrr,
    pvca,
    RColorBrewer,
    readr, 
    reshape2, 
    rlang,
    scales,
    sva,
    tidyr,
    tidyverse,
    tibble,
    wesanderson,
    WGCNA
Suggests: knitr,
    rmarkdown
VignetteBuilder: knitr
RoxygenNote: 6.1.1
bioc-issue-bot commented 5 years ago

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.

mtmorgan commented 5 years ago

ouch, that long list of dependencies looks like you've created a very very fragile package; are they all necessary? I strongly recommend you revisit the need for each dependency, trimming the functionality of your package to make it more maintainable and robust.

bioc-issue-bot commented 5 years ago

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

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Please rethink and modify your package according to Martin's comments. (https://github.com/Bioconductor/Contributions/issues/958#issuecomment-449081049).

Besides, the building report has some ERRORS, including grammar mistakes, tarball size too big and package versioning. Please go through your package and remove any unnecessary files. New package should start from version 0.99.0 at first submission and bump version to 0.99.1 and go on for any changes you made during review process.

WARNINGS are also present with clear messages, e.g., R dependency version (should be >= 3.6), inconsistency about package imports between DESCRIPTION and NAMESPACE.

These are important, please make sure to clear all ERROR and WARNINGS so that a technical review will be given later. Comment back here with any questions or concerns.

Thank you for the contribution to Bioconductor!

Best, Qian

ChloeHJ commented 5 years ago

Dear revisions team,

Thank you for your feedbacks on the package, I made the submission by a branch called "BIoconductor_submission" instead of the master, but I am not sure if the revision has been done the branch or on the master, as many comments are already considered in the submitted branch. Would it be necessary to merge everything to master for an appropriate revision?

Thank you and best regards Chloe

On Thu, Dec 20, 2018 at 8:42 PM Qian Liu notifications@github.com wrote:

Hi @ChloeHJ https://github.com/ChloeHJ ,

Please rethink and modify your package according to Martin's comments. (#958 (comment) https://github.com/Bioconductor/Contributions/issues/958#issuecomment-449081049 ).

Besides, the building report has some ERRORS, including grammar mistakes, tarball size too big and package versioning. Please go through your package and remove any unnecessary files. New package should start from version 0.99.0 at first submission and bump version to 0.99.1 and go on for any changes you made during review process.

WARNINGS are also present with clear messages, e.g., R dependency version (should be >= 3.6), inconsistency about package imports between DESCRIPTION and NAMESPACE.

These are important, please make sure to clear all ERROR and WARNINGS so that a technical review will be given later. Comment back here with any questions or concerns.

Thank you for the contribution to Bioconductor!

Best, Qian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/958#issuecomment-449113731, or mute the thread https://github.com/notifications/unsubscribe-auth/ApdNc-TvGDDzLbaW7ZvPQQSO_pK3wzbuks5u6-gcgaJpZM4ZcVaz .

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

When you done setting up the web hook https://github.com/Bioconductor/Contributions/blob/master/CONTRIBUTING.md#adding-a-web-hook, it said

Subsequent pushes to the default master branch of your repository will now trigger builds (only if the package version has been bumped), and the build reports will be posted to the issue you created in this repository. Note that pushes to branches that are not master, or non-default branches, are ignored.

So you should merge all the new commits into master branch so that it triggers new builds. Don't forget the bump version when you need "Bioconductor/Contribution" to catch the updates.

Best, Qian

ChloeHJ commented 5 years ago

Thank you Qian, will do shortly :)

On Fri, Dec 21, 2018, 4:34 PM Qian Liu <notifications@github.com wrote:

Hi @ChloeHJ https://github.com/ChloeHJ ,

When you done setting up the web hook https://github.com/Bioconductor/Contributions/blob/master/CONTRIBUTING.md#adding-a-web-hook, it said

Subsequent pushes to the default master branch of your repository will now trigger builds (only if the package version has been bumped), and the build reports will be posted to the issue you created in this repository. Note that pushes to branches that are not master, or non-default branches, are ignored.

So you should merge all the new commits into master branch so that it triggers new builds. Don't forget the bump version when you need "Bioconductor/Contribution" to catch the updates.

Best, Qian

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/958#issuecomment-449419300, or mute the thread https://github.com/notifications/unsubscribe-auth/ApdNcw4FV4B0Es0XBpJ5NXv6ynYJgOaTks5u7P-KgaJpZM4ZcVaz .

ChloeHJ commented 5 years ago

All changes have been merged to the master (version 0.99.1). We resolved all ERRORs and WARNINGs from build, check and Bioccheck. Thank you for your time and kind feedback,

Best regards, Chloe

Liubuntu commented 5 years ago

HI @ChloeHJ ,

Seems that the Bioconductor/Contribution still did not catch those changes. I could see those commits in "master" branch, but you do need to change the repository in the first comment under this issue to the following: https://github.com/symbioticMe/proBatch/tree/master

After changing the repository, please remember to bump version again to 0.99.2 and see if the change has gone through. Thanks!

Best, Qian

ChloeHJ commented 5 years ago

Hi Qian,

The repository has been modified and version updated!

Thank you and best regards, Chloe

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Unfortunately the building machine didn't catch the commits by your new version bump... But it seems to be working fine on our end, would you like to check your webhook to see if that is set correctly? Thanks!

Best, Qian

ChloeHJ commented 5 years ago

Hi Qian,

I've changed the webhook and bumped the version to 0.99.3, could you please have a look?

Thank you and best regards, Chloe

ChloeHJ commented 5 years ago

Hi Qian,

Sorry, I misunderstood adding the webhook part, I think we didn't put the URL into payload URL in settings, will do shortly,

Thank you and best regards, Chloe

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Just a reminder that we would expect update on the issue (with updates/questions/commits/comments...) within 2 weeks. If no response within 2 weeks of last comment, the issue would be closed.

Thanks! Qian

ChloeHJ commented 5 years ago

Hi Qian,

We wanted to add the bioconductor link onto our webhook but the proBatch package is owned by my colleague and she is currently on a holiday and will only be back online in 3-5 days from today. Would there be a way to postpone the issue from being closed?

Thank you and best regards, Chloe

Liubuntu commented 5 years ago

HI @ChloeHJ ,

I'll keep the issue open for your colleague comeing back. Thank you for the explanation.

Best, Qian

ChloeHJ commented 5 years ago

Hi Qian,

The webhook was added to our github repository, could you please check if the building machine can catch the version bumps?

Thank you and best regards, Chloe

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

After adding the web hook following [this](), bump the package version in DESCRIPTION in master branch, the building machine would catch it and have some automatic comments under this issue showing it with a new building report if all is set correctly.

According here: https://github.com/symbioticMe/proBatch/commits/master, The lastest version bump was done on Jan 2nd. So you may want to bump version again to see if it is correct.

Qian

bioc-issue-bot commented 5 years ago

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

a2ddf5e bump version to 0.99.4

bioc-issue-bot commented 5 years ago

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.

bioc-issue-bot commented 5 years ago

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

657be0b changed maintainer of package

bioc-issue-bot commented 5 years ago

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.

ChloeHJ commented 5 years ago

Hi Qian,

I was wondering if the revision process is in progress and if there is anything you would like us to revise on,

Thank you and best regards, Chloe

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Yes, the webhook has been set correctly and the building machine could catch commits and version bump right now. You can expect an initial review /comments within 2 weeks since the last successful build.

Best, Qian

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Here is the initial review of your package. It is quite a big package with many functionalities and well written generally. Some issues to address for each section are summarized here. Please make effort to address all/most of them and comment back here with any questions. The vignette would be reviewed shortly and append later here. Thanks!

DESCRIPTION

NAMESPACE

R/


- similar in `proteome_wide_diagnostics.R` L49. Pre-assign argument values (`dendroLabels`, `cex.dendroLabels`...) with conditions and only call `WGCNA::plotDendroAndColors()` once to avoide duplicate code.

- when using checking conditions with `setequal`, use `if (!setequal())` instead of `if(setequal() == FALSE)` to avoid checking twice. Grep `setequal` in your R scripts, this seemed to appear in multiple cases.

- `correlation-based_diagnostics.R` L299,418, avoid functions inside functions, make them as internal functions with prefix of ".", and then call this internal function inside the main exported function for better organization and debugging.

- `fit_non_linear.R` L6: typo? Should be `measurement` instead of `measurment`.

- `normalize.R` L104: Use `normalize_data <- function(..., normalizeFunc = c("quantile", "MedianCentering")...) { normalizeFunc <- match.arg(normalizeFunc); ...` so the default would still be "quantile" and at the same time, it checks the provided values and return error if doesn't match to either "quantile" or "MedianCentering". Go through your scripts and apply to appropriate places.

- `correct_batch_effects.R` L71,86,183,230,233, Seems like you want to use `\code{...}` instead of "`...`". Try to go through other documentations for similar issues if you intend to encompass those words in help page. Some other places with similar situation: include: `correlation-based_diagnostics.R`, L251,254. `fit_non_linear.R` L14. `initial_assessment.R` documentations. `proteome_wide_diagnostics.R` L8,10,17.

- SUGGESTION: Prefix non-exported functions with a ‘.’ for easy reference. Refer [here](https://www.bioconductor.org/developers/how-to/coding-style/) for Biocoductor coding style.
- SUGGESTION: Use ` <- ` instead of " = " for consistent style for variable assignment all through the code.

## man/

- The documentation of functions `center_peptide_batch_medians` and `correct_batch_effects` are using the same `@rdname` and sharing one `man/correct_batch.Rd` file. So for the `@description` and `@return` fields, please specify which corresponds to which, so that the help page doesn't look confusing to uses.

- `man/correct_with_ComBat.Rd`, the "title" is too long. Since you already have everything in "description", you should make a short and descriptive title with 1 sentence. (in `R/correct_batch_effects.R`::L164, put the whole paragraph within `@description`, anda short title in `@name`)

- `man/plot_protein_corrplot.Rd`::L51. Shouldn't it be a ggplot object instead of "nothing"?
- Same issue as in `man/plot_sample_corr_heatmap.Rd`::L43.

Cheers, 
Qian
Liubuntu commented 5 years ago

Here are some additional comments. Please try to comment back here with any question, or commits, or updates within 2 weeks to keep this issue open. Thanks!

vignette

general

ChloeHJ commented 5 years ago

Thank you so much for your comments Qian, I will get back to you as soon as possible :)

On Thu, Jan 31, 2019, 10:07 PM Qian Liu notifications@github.com wrote:

Here are some additional comments. Please try to comment back here with any question, or commits, or updates within 2 weeks to keep this issue open. Thanks! vignette

  • Add affiliations for the authors?
  • L248,275. Do not eval the library(...) in the vignette.
  • The vignette looks really nice with explanations and details. Only that it looks a bit too long for users to quickly pick up the workflow with key functions. The current version is ok, but would also be nice to have a separate short one with an example data to go through the analysis. You can put both inside the vignette folder, and only include the Installation section once.

general

  • remove the NAMESPACE.rej file from top folder.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/958#issuecomment-459506628, or mute the thread https://github.com/notifications/unsubscribe-auth/ApdNczPSyjt2gTSfcPyge6ZkH3hqi8q3ks5vI1sJgaJpZM4ZcVaz .

bioc-issue-bot commented 5 years ago

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

9485c1d bump version

bioc-issue-bot commented 5 years ago

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.

ChloeHJ commented 5 years ago

Hi Qian,

I have made changes based on your comments but I had a question regarding the "imports". I tried to remove tidyverse from DESCRPTION/ NAMESPACE/ README and replace with smaller functions but the devtools::document() would automatically generate NAMESPACE including the tidyverse, and there will be an error when I go through check() with tidyverse removed. Could you please help me with these problems?

p.s. I've addressed most of your comments, except the 1) the imports, 2) shorter vignette (we will soon prepare a shorter vignette), and 3) "..." issue where @param was devoted to explain what parameters can be added to "..." and I thought no changes were necessary.

Thank you so much for your time and comments!

Best regards, Chloe

Liubuntu commented 5 years ago

Thanks for the quick update. I'll take a 2nd look and get back to you soon.

bioc-issue-bot commented 5 years ago

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

e883cc7 version bump

ChloeHJ commented 5 years ago

Hi Qian,

We just added a shorter version of vignette to our package. It would be great if you could kindly look at our vignette as well. Thank you so much for your reviews,

Best regards Chloe

bioc-issue-bot commented 5 years ago

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 commented 5 years ago

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

a3c3a76 version bump

bioc-issue-bot commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

cbd6656 version bump

bioc-issue-bot commented 5 years ago

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 build report for more details.

bioc-issue-bot commented 5 years ago

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

40d376f reduce size of gallery image

bioc-issue-bot commented 5 years ago

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

ChloeHJ commented 5 years ago

Hi Qian,

After adding the second vignette to our package, there have been several issues e.g, exceed size requirements, exceed 5 mins check requirements (below). I am not sure if the settings need to be modified to remove these warnings and errors, or if these errors and warnings would persist by having the shorter vignette to our package, we are also in the process of preparing an Applications note to be submitted to Bioinformatics, we can have a shorter illustrations in this manuscript and remove the shorter vignette from our package. It would be great if you could kindly have a look at the vignettes and let us know what you would recommend,

WARNING: R CMD check exceeded 5 min requirement

Thank you so much for your time and reviews,

Best regards, Chloe

bioc-issue-bot commented 5 years ago

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

225b72c correct typo fon vignette

bioc-issue-bot commented 5 years ago

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

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Here is the 2nd review of your package:

DESCRIPTION / NAMESPACE

R

man

test

Best, Qian

Liubuntu commented 5 years ago

One more important thing for developing a new R/Bioconductor package is the interoperability with other existing / established packages in the same area.

Interoperability is strongly encouraged in Bioconductor and would help with the usage of your own package. Consider interoperability with packages included in RforProteomics, and mzR which uses CDF (Common Data Format) to represent MS data in R without completely loading data into memory? Check here for other MS related R / Bioconductor packages: http://bioconductor.org/packages/release/BiocViews.html#___MassSpectrometry

Liubuntu commented 5 years ago

For the error of large size, and exceeding check time, try to make your example dataset (used in examples or vignettes or test functions) minimum but just big enough to show the functionalities. This is usually the case for making them fit into a Bioconductor package.

ChloeHJ commented 5 years ago

Hi Qian,

Thank you for your comments! I will look through them and get back to you soon,

Best regards, Chloe

ChloeHJ commented 5 years ago

Hi Qian,

Regards to the problem of large size, I was wondering if this was due to a wrong setting on the second vignette we produced because the size of data is 1.3Mb but the doc size goes 3.8Mb. Could you please have a closer look onto our second vignette (vignette_brief) and see if it's made correctly?

Thank you so much! Chloe

Liubuntu commented 5 years ago

Hi @ChloeHJ ,

Sorry for late reply. But I'll look into the short vignette get back to you shortly.

Thanks, Qian