Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

ctgGEM #1334

Closed bicbioeng1 closed 4 years ago

bicbioeng1 commented 4 years ago

Update the following URL to point to the GitHub repository of the package you wish to submit to Bioconductor

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 4 years ago

Hi @bicbioeng1

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: ctgGEM
Type: Package
Title: Generating Tree Hierarchy Visualizations from Gene Expression Data
Version: 0.99.0
Author: Mark Block and Carrie Minette
Maintainer: USD Biomedical Engineering <bicbioeng@gmail.com>
Description: Package to generate tree hierarchy visualizations from gene 
    expression data using a variety of methodologies.  
VignetteBuilder: knitr
License: GPL(>=2)
Encoding: UTF-8
LazyData: true
biocViews: GeneExpression, Visualization, Sequencing, SingleCell, Clustering, 
    RNASeq, ImmunoOncology, DifferentialExpression, MultipleComparison, 
    QualityControl, DataImport
RoxygenNote: 6.1.1
Roxygen: list(markdown = TRUE)
Depends: monocle
Imports:
    Biobase,
    BiocGenerics,
    ggplot2,
    graphics,
    grDevices,
    igraph,
    Matrix,
    methods,
    utils
Suggests:
    BiocStyle,
    biomaRt,
    cellTree,
    destiny,
    HSMMSingleCell,
    irlba,
    knitr,
    sincell,
    TSCAN,
    VGAM
Collate: 
    'ctgGEMset-class.R'
    'ctgGEMset-methods.R'
    'generate_tree.R'
    'makeCellTree.R'
    'makeDestiny.R'
    'makeMonocle.R'
    'makeSincell.R'
    'makeTSCAN.R'
    'plotOriginalTree.R'
    'tree2igraph.R'

Add SSH keys to your GitHub account. SSH keys will are used to control access to accepted Bioconductor packages. See these instructions to add SSH keys to your GitHub account.

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

Please update your package to use the more modern SummarizedExperiment rather than ExpressionSet, or perhaps given the nature of your package TreeSummarizedExperiment

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.

mtmorgan commented 4 years ago

Please let me know when you will provide an update to your package.

bicbioeng1 commented 4 years ago

Dear Mr. Morgan,

I apologize for the delay, some issues on our end prevented us from resolving the package problems sooner. We should have our updates in early this week.

If you don't mind me asking: is Bioconductor moving away from the use of the ExpressionSet class? We're fine switching over to using SummarizedExperiment classes, but the ExpressionSet has proven to be a very useful data structure for handling our gene expression data, so out of curiosity we would like to know why the former is preferred in this case. Thank you, and have a great day!

On Thu, Jan 23, 2020 at 8:00 AM Martin Morgan notifications@github.com wrote:

Please let me know when you will provide an update to your package.

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

mtmorgan commented 4 years ago

Yes, SummarizedExperiment was introduced more than 4 years ago as a replacement for ExpressionSet.

bicbioeng1 commented 4 years ago

Mr. Morgan,

We should have the updated code in today, I'll let you know when it happens.

Quick question: while we have changed our core structure to use the SummarizedExperiment class, would it be reasonable to leave the ExpressionSet implementation for our destiny-related methods? The destiny package makes extensive use of the ExpressionSet class in their documentation and their methods seem to favor ExpressionSet objects. Thanks!

On Mon, Jan 27, 2020 at 9:34 AM Martin Morgan notifications@github.com wrote:

Yes, SummarizedExperiment was introduced more than 4 years ago as a replacement for ExpressionSet.

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

bioc-issue-bot commented 4 years ago

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

b5ad9f0 fixed R build issues and changed class structure t...

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.

mtmorgan commented 4 years ago

yes supporting both SummarizedExperiment and ExpressionSet is appropriate.

mtmorgan commented 4 years ago

is your package ready for review?

bicbioeng1 commented 4 years ago

My apologies, the package is technically ready but I can't seem to figure out how to fix the warning generated during the build process. The issue is in the html code for the vignette in the inst/doc subdirectory after the package is built, and I can't seem to find anyone else reporting this problem. At this point I should probably raise the question to the public and see if anyone has any insight because I'm not sure where the warning is coming from. I can't reproduce it locally...

On Wed, Feb 5, 2020 at 4:23 PM Martin Morgan notifications@github.com wrote:

is your package ready for review?

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

mtmorgan commented 4 years ago

The warning I see is

* Checking coding practice...
    * WARNING: Avoid class() == or class() != ; use is() or !is()
      Found in files:
        inst/doc/ctgGEM-Vignette.html (line 906)

which as you say is in an HTML file; this looks like a red herring to me. I'll review your package and look into the cause of this...

mtmorgan commented 4 years ago

Please review the following comments, update your package, and post a comment with a brief summary of your response. The issue about the warning from HTML files seems like a false positive and BiocCheck will be updated.

DESCRIPTION

vignettes

R (comments on specific lines of code may apply to other parts of the pacakge)

man

bicbioeng1 commented 4 years ago

Thank you, will get those updates in as soon as possible!

On Mon, Feb 10, 2020 at 12:25 PM Martin Morgan notifications@github.com wrote:

Please review the following comments, update your package, and post a comment with a brief summary of your response. The issue about the warning from HTML files seems like a false positive and BiocCheck will be updated.

DESCRIPTION

  • please provide a more comprehensive Description: field, like a paper abstract, outling what specific analysis your package enables.

vignettes

  • good

R (comments on specific lines of code may apply to other parts of the pacakge)

-

ctgGEMset-class.R: 84 The convention is to name the 'constructor' after the class, so ctgGEMset() rather than newctgGEMset().

ctgGEMset-class.R:87 stop() prefixes "Error:" to the message, so no need to include "Error:" in the message

ctgGEMset-methods.R: 81 break stop() messages into separate character vectors to avoid unusual indentation; compare

stop("foo

  • bar") Error: foo bar stop("foo ",
  • "bar") Error: foo bar

-

ctgGEMset-methods.R:106 No need to paste() separately from stop(); stop() calls an implicit paste0() on it's arguments

ctgGEMset-methods:217 I don't think it's a good idea to implement exprs() / pData() etc on SummarizedExperiment or in general to provide a compatiblity layer between SummarizedExperiment and ExpressionSet -- ExpressionSet should be viewed as 'legacy' and supported for backward compatibility only. For instance, colData() of a SummarizedExperiment can contain data that are not well-represented as data.frame(), e.g., DataFrame(x = GRanges()). It should not be / is not necessary to use accessors when accessing these objects, e.g., colData(object)@listData should be simply colData(object).

generate_tree.R:46 functions should not write to the current working directory, but instead provide the option for user to provide a directory, defaulting to tempfile() or perhaps tempdir(). The command dir.exists() is more transparent than file_test("-d", ...)

makeMonocle.R:74 use format(Sys.time(), "%Y-%m-%d_...") instead of this transformation; see ?format.POSIXct for the format string.

makeMonocle.R:174 use message() rather than print() to communicate messages to users.

man

  • good

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

bioc-issue-bot commented 4 years ago

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

ab78148 updates for Bioconductor

bicbioeng1 commented 4 years ago

Here is a list of the most recent updates:

On Tue, Feb 11, 2020 at 10:35 PM USD Biomedical Engineering < bicbioeng@gmail.com> wrote:

Thank you, will get those updates in as soon as possible!

On Mon, Feb 10, 2020 at 12:25 PM Martin Morgan notifications@github.com wrote:

Please review the following comments, update your package, and post a comment with a brief summary of your response. The issue about the warning from HTML files seems like a false positive and BiocCheck will be updated.

DESCRIPTION

  • please provide a more comprehensive Description: field, like a paper abstract, outling what specific analysis your package enables.

vignettes

  • good

R (comments on specific lines of code may apply to other parts of the pacakge)

-

ctgGEMset-class.R: 84 The convention is to name the 'constructor' after the class, so ctgGEMset() rather than newctgGEMset().

ctgGEMset-class.R:87 stop() prefixes "Error:" to the message, so no need to include "Error:" in the message

ctgGEMset-methods.R: 81 break stop() messages into separate character vectors to avoid unusual indentation; compare

stop("foo

  • bar") Error: foo bar stop("foo ",
  • "bar") Error: foo bar

-

ctgGEMset-methods.R:106 No need to paste() separately from stop(); stop() calls an implicit paste0() on it's arguments

ctgGEMset-methods:217 I don't think it's a good idea to implement exprs() / pData() etc on SummarizedExperiment or in general to provide a compatiblity layer between SummarizedExperiment and ExpressionSet -- ExpressionSet should be viewed as 'legacy' and supported for backward compatibility only. For instance, colData() of a SummarizedExperiment can contain data that are not well-represented as data.frame(), e.g., DataFrame(x = GRanges()). It should not be / is not necessary to use accessors when accessing these objects, e.g., colData(object)@listData should be simply colData(object).

generate_tree.R:46 functions should not write to the current working directory, but instead provide the option for user to provide a directory, defaulting to tempfile() or perhaps tempdir(). The command dir.exists() is more transparent than file_test("-d", ...)

makeMonocle.R:74 use format(Sys.time(), "%Y-%m-%d_...") instead of this transformation; see ?format.POSIXct for the format string.

makeMonocle.R:174 use message() rather than print() to communicate messages to users.

man

  • good

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

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:

9655eeb removed excess files

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

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!

mtmorgan commented 4 years ago

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/bicbioeng1.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 BiocManager::install("ctgGEM"). The package 'landing page' will be created at

https://bioconductor.org/packages/ctgGEM

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.