Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

peco #1248

Closed jhsiao999 closed 4 years ago

jhsiao999 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 @jhsiao999

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:

Encoding: UTF-8
Type: Package
Package: peco
Version: 0.99.0
Date: 2019-09-11
Title: A Supervised Approach for **P**r**e**dicting **c**ell Cycle
    Pr**o**gression using scRNA-seq data
Authors@R: c(
       person("Chiaowen Joyce","Hsiao", email="joyce.hsiao1@gmail.com", 
          role=c("aut","cre")),
       person("Matthew", "Stephens", email="stephens999@gmail.com", role="aut"),
       person("John", "Blischak", email = "jdblischak@gmail.com", role="ctb"),
       person("Peter", "Carbonetto", email = "peter.carbonetto@gmail.com",
    role="ctb"))
Maintainer: Chiaowen Joyce Hsiao <joyce.hsiao1@gmail.com>
Description: Our approach provides a way to assign continuous cell cycle phase 
    using scRNA-seq data, and consequently, allows to identify cyclic trend 
    of gene expression levels along the cell cycle. This package provides method
    and training data, which includes scRNA-seq data collected from 6 individual 
    cell lines of induced pluripotent stem cells (iPSCs), and also continuous cell 
    cycle phase derived from FUCCI fluorescence imaging data. 
URL: https://github.com/jhsiao999/peco
BugReports: https://github.com/jhsiao999/peco/issues
License: GPL (>= 3)
Depends: R (>= 3.5)
Imports:
    assertthat,
    Biobase,
    circular,
    conicfit,
    doParallel,
    foreach,
    genlasso,
    graphics,
    methods,
    parallel,
    stats
Suggests: 
    knitr,
    rmarkdown
Enhances:
    parallel
Remotes:
  glmgen/genlasso
biocViews:
  Sequencing, RNASeq, GeneExpression, Transcriptomics, SingleCell,
  Software, StatisticalMethod, Classification, Visualization
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

Please update your package to use the more modern SummarizedExperiment or, given the topic of your package, SingleCellExperiment

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.

jhsiao999 commented 5 years ago

@mtmorgan Thank you for the feedbacks. We now use SingleCellExperiment to store our data. All examples have been updated accordingly.

@bioc-issue-bot We've bumped the version to 0.99.3.

LiNk-NY commented 5 years ago

Hi ChiaoWen, @jhsiao999 Thank you for your submission. Please see the package review below. If you have any questions, feel free to post them as comments below.

Best, Marcel


peco #1248

Overall, the package seems to use lists often as function outputs. It is better to have 'endomorphic' operations that take a SingleCellExperiment as input and return one with results whenever possible.

DESCRIPTION

NAMESPACE

vignettes

Minor:

R

data.R

Minor:

LiNk-NY commented 4 years ago

Hi ChiaoWen, @jhsiao999 Please provide an item-by-item response to the review. Otherwise, I'd be forced to close the issue due to inactivity. Thank you. -Marcel

jhsiao999 commented 4 years ago

@LiNk-NY Hi Marcel, sorry for the delayed response. I've been working on incorporate the package as a Bioconductor single-cell analysis workflow, so that the main input/output object would be SingleCellExperiment. I will send a response to the comments later today.

Thanks for the reminder,

Joyce

jhsiao999 commented 4 years ago

@LiNk-NY Thank you for the useful suggestions. Please see my reply below. These changes have been incorporated into version v0.99.5. https://github.com/jhsiao999/peco

Hi ChiaoWen, @jhsiao999 Thank you for your submission. Please see the package review below. If you have any questions, feel free to post them as comments below.

Best, Marcel

peco #1248

Overall, the package seems to use lists often as function outputs. It is better to have 'endomorphic' operations that take a SingleCellExperiment as input and return one with results whenever possible. The main functions now output SingleCellExperiment object. These include cycle_npreg_outsample and data_transform_quantile.

DESCRIPTION

  • The Date field is not really needed.
  • Looks good

NAMESPACE

  • Avoid tagging functions with generic. The 'generic' property should be reflected in the implementation I am not sure how to address this comment. In the current version of NAMESPACE, none of the function is tagged with generic. Please let me know if this is still an issue.

vignettes

  • Please respect the 80 column width limit, it makes it easier to maintain and easier to review The code has been edited accordingly.

  • Avoid using get with data. Simply use data(sce_sub) Data are now loaded use data

  • Consider using a more informative dataset name Dataset names are changed to model_5genes_predict.rda, model_5genes_train.rda, sce_top101genes.rda and training_human.rda.

  • Use apply(x, 1L, ...) instead of do.call(rbind, lapply(seq_len(nrow(x)), ...)) Changes have been made accordingly.

  • Avoid converting logical to numeric index. Often the logical index is enough: which(coldata$chip_id != "NA18511") can be coldata$chip_id != "NA18511" and you can add ! in the front to get the opposite (instead of ==) Changes have been made accordingly.

Minor:

  • Simplify code by using 1:5 instead of c(1:5) Changes have been made accordingly.

R

  • Please use a standard 1 or 2 cores as default for functions that use doParallel Changes have been made accordingly.

  • Prioritize vectorized operations and try to avoid nested for/lapply loops Changes have been made accordingly.

  • Avoid using sapply and use vapply instead Changes have been made accordingly.

  • Use 'character' indices (e.g., x["coef"] vs x[2]) for subsetting data whenever possible to avoid mistakes when data changes Changes have been made accordingly.

data.R

  • Use descriptive names Changes have been made accordingly.

Minor:

  • Consider using seq_along instead of seq_len(length(...)) Changes have been made accordingly.

  • No need to double evaluate to TRUE in if (plot.it == TRUE); if (plot.it) should be enough Changes have been made accordingly.

bioc-issue-bot commented 4 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.

lshep commented 4 years ago

We are working on updating the SPB builders on R 4.0 and Bioc 3.11 the dependency issues is on our end and we are investigating

bioc-issue-bot commented 4 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.

lshep commented 4 years ago

The dependency issue on our end has been fixed. These results reflect real ERRORs that should be corrected. Cheers

LiNk-NY commented 4 years ago

Hi Joyce ChiaoWen, @jhsiao999 Please follow up on the issues reported on the SPB. Thank you. -M

jhsiao999 commented 4 years ago

Thanks @LiNk-NY! I'll look into this error and let you know if I have any questions.

Joyce

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999 Any updates on the status of the changes? Thanks.

jhsiao999 commented 4 years ago

Hi Marcel,

Apologies fo the delay. I am getting to them before the end of the year. Thanks for you patience!

Joyce

On Thu, Dec 19, 2019 at 2:39 PM Marcel Ramos notifications@github.com wrote:

Hi Joyce, @jhsiao999 https://github.com/jhsiao999 Any updates on the status of the changes? Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Bioconductor/Contributions/issues/1248?email_source=notifications&email_token=ABWB66S6KMJZMIPFCDCEYOTQZPEWDA5CNFSM4IWMSGVKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHKWKZQ#issuecomment-567633254, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABWB66SJ55YKXH2XUJEBRULQZPEWDANCNFSM4IWMSGVA .

jhsiao999 commented 4 years ago

Hi Marcel @LiNk-NY ,

The errors have been fixed in peco 0.99.6. Please let me know if there are any more issues. Thanks!

Joyce

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999 Please bump the package's version for another build in the SPB. Thank you. Best, Marcel

jhsiao999 commented 4 years ago

Hi Marcel @LiNk-NY ,

I just bumped the version to 0.99.7, made a new build, and pushed the build to the GithHub repo. Thanks!

Joyce

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999 Thank you for bumping the version. Can you check if the web hook is still active? I don't see a build of your package. CC: @lshep Lori, Is the SPB operational? Thanks.

Kayla-Morrell commented 4 years ago

Hey Marcel @LiNk-NY, I just checked the SPB and everything is running correctly on it.

jhsiao999 commented 4 years ago

Hi Marcel @LiNk-NY,

Do you see a build of the package now? I just checked the bioconductor webhook of the repo and it appears to be active (there is also a successful delivery).

P.S. What is the common practice of choosing webhook trigger event? Currently, I select "Just the Push Event". Thanks!

Joyce

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999

The build message should look like the comment above.

You have the right trigger selected. Try bumping the package z version (x.y.z) again.

@Kayla-Morrell Thanks for looking into it Kayla!

bioc-issue-bot commented 4 years ago

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

38e78ec trigger bioconductor build

jhsiao999 commented 4 years ago

Hi Marcel,

I bumped the version again. The push event seems to have triggered the above message.

Joyce

Hi Joyce, @jhsiao999

The build message should look like the comment above.

You have the right trigger selected. Try bumping the package z version (x.y.z) again.

@Kayla-Morrell Thanks for looking into it Kayla!

bioc-issue-bot commented 4 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 4 years ago

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

45dad56 bump to v0.99.9

bioc-issue-bot commented 4 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". 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.

jhsiao999 commented 4 years ago

@LiNk-NY What do you suggest I should do to handle the following warning? Thanks!

Joyce

* Checking R Version dependency...
    * WARNING: Update R version dependency from 3.5 to 4.0.
LiNk-NY commented 4 years ago

@jhsiao999 Because Bioconductor devel uses R-devel. It is recommended that you change the R version dependency to 4.0.0 in the DESCRIPTION file.

bioc-issue-bot commented 4 years ago

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

ee94ff4 change dependency to R4.0

bioc-issue-bot commented 4 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.

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999 I don't see your line-by-line response to the review. I will check the changes today. Best, Marcel

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999

I now noticed your line-by-line responses. In the future, please add more empty lines between original text and your responses for readability. (See below for an example)

Avoid tagging functions with generic. The 'generic' property should be reflected in the implementation

I am not sure how to address this comment. In the current version of NAMESPACE, none of the function is tagged with generic. Please let me know if this is still an issue.

Some functions such as fit_trendfilter_generic have the generic tag in the name. This should just be a regular function called fit_trendfilter unless you're creating a 'generic' for which you have and own a class to apply the method to (in S4 the implementation would involve setGeneric and setMethod).

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999 Any updates?

jhsiao999 commented 4 years ago

Hi Marcel @LiNk-NY,

I didn't have time to look into this yet. I'll fix it this weekend. Thank you!

Joyce

LiNk-NY commented 4 years ago

Thank you Joyce! @jhsiao999

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999
It's been a few weekends. Do you have updates to peco? Thanks!

bioc-issue-bot commented 4 years ago

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

9da591d support for matrix input c04a276 add matrix input option 53ff5eb debugging c5fae46 rename fit_trendfilter_generic to fit_trendfilter

bioc-issue-bot commented 4 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 4 years ago

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

2afba13 version bump and README.md update

bioc-issue-bot commented 4 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 4 years ago

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

7f38529 version bump to 0.99.13 and bug fixes

bioc-issue-bot commented 4 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.

bioc-issue-bot commented 4 years ago

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

1e79324 version bump to 0.99.14 and fixes to Travis-CI and...

bioc-issue-bot commented 4 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.

LiNk-NY commented 4 years ago

Hi Joyce, @jhsiao999

Thanks for making those changes. Do the packages need a version number dependency?

https://github.com/jhsiao999/peco/blob/1e79324378a8e13b0fd26be8401ce7f6f8f418e9/DESCRIPTION#L26-L37

Usually, you add a version number if there is a feature that is only implemented in a particular version or above. Is that the case for all the packages listed in the DESCRIPTION file with the exception of R?

jhsiao999 commented 4 years ago

Hi @LiNk-NY,

I cannot be sure of all the specific features that are implemented in a particular version of these pages. The version numbers are added in case of a possible feature that I am not aware of. I usually do this for my R packages to avoid possible versioning issues during software installation. This is probably more of a concern for newer packages such as scater, SingleCellExperiment and SummarizedExperiment.

What do you think about keep the version numbers of the newer packages? Is this acceptable?

Thanks, Joyce

jhsiao999 commented 4 years ago

Hi @LiNk-NY,

We have received requests about using our package for R3.6. The current version requires R>=4.0. Any suggestions on how to approach this? For example, could we create a new branch release compatible with R3.6? Would this trigger a new Bioc package built? Thanks!

Joyce