search

Bioconductor / Contributions

Contribute Packages to Bioconductor
134 stars 33 forks source link

(inactive) cellassign #1178

Closed kieranrcampbell closed 4 years ago

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

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: cellassign
Version: 0.99.2
Title: What the Package Does (One Line, Title Case)
Description: What the package does (one paragraph).
Authors@R: c(
  person("Allen", "Zhang", email = "alzhang@bccrc.ca", role = c("aut")),
  person("Kieran", "Campbell", email = "kieranrcampbell@gmail.com", role = c("aut", "cre"))
  )
License: Apache License (>= 2.0)
Encoding: UTF-8
Depends: R (>= 3.6)
Imports:
    methods,
    stats,
    glue,
    tensorflow,
    SummarizedExperiment,
    scran
Suggests: 
    knitr,
    SingleCellExperiment,
    rmarkdown,
    BiocStyle,
    dplyr,
    pheatmap,
    testthat,
    limma,
    org.Hs.eg.db,
    edgeR,
    matrixStats,
    plyr,
    magrittr
biocViews:
  Software, 
  Transcriptomics, 
  GeneExpression, 
  RNASeq,
  SingleCell
LazyData: true
ByteCompile: true
Roxygen: list(markdown = TRUE)
RoxygenNote: 6.1.1
VignetteBuilder: knitr

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.

kieranrcampbell commented 5 years ago

Forgot informative package title and description - updated in latest commit

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.

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:

a0bb00b Remove cellassign.Rproj

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:

c73d14b Install tensorflow if not already installed

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:

6bab37d Remove warning message

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.

kieranrcampbell commented 5 years ago

Hi @LiNk-NY

This error appears to be due to a 64 bit Python version not being on the build servers:

Please install 64-bit Python 3.5 or 3.6 to continue, supported versions include:

Can you advise us how to proceed?

Many thanks

LiNk-NY commented 5 years ago

Hi Kieran, @kieranrcampbell

Our SPB maintainer, Lori @lshep, can best answer that. It seems like the other builders have that version so we should support it. For now, ignore the error until we can get it working.

Thanks, Marcel

lshep commented 5 years ago

I am discussing the issue with the other system maintainer @hpages and will respond when we have further information. But yes please ignore for now.

LiNk-NY commented 5 years ago

Hi Kieran, @kieranrcampbell Thank you for your submission. Please see the review below.

If you have any questions, feel free to reach out to me here. Best, Marcel

cellassign #1178

DESCRIPTION

NAMESPACE

vignettes

data

R

kieranrcampbell commented 5 years ago

Hi Marcel @LiNk-NY

Thanks for your thorough and thoughtful review! We will work through these point-by-point and get back to you with any queries.

Kieran

bioc-issue-bot commented 5 years ago

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

6c727e3 Move from $ accessors to accessor functions 2849513 remove glue from imports 1b537ca Make lines shorter 62e08bd Bump version number to trigger bioc rebuild

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: "ABNORMAL". 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:

00bb415 Bump version number to trigger bioc rebuild

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: "ABNORMAL". 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.

kieranrcampbell commented 5 years ago

Hi @LiNk-NY

Looks like the build is abnormal as the build system isn't pulling any data (package size -1Kb) - any ideas? Thanks!

bioc-issue-bot commented 5 years ago

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

a5fb2ee Check if abnormal fixed this morning by bumping ve...

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: "ABNORMAL". 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 5 years ago

The ERROR is real. Your DESCRIPTION file is malformed. Please fix the new lines in the description field. 

Description: CellAssign assigns cells measured with scRNA-seq to 
both known and de novo cell types based on the declaring certain 
genes as markers for different cell types. The result is the 
probability that each cell is of a given cell type, or of an 
"other" cell type that doesn't match any markers provided.
bioc-issue-bot commented 5 years ago

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

f32019d DESCRIPTION malformed 3b268e9 Bump version number

kieranrcampbell commented 5 years ago

The ERROR is real. Your DESCRIPTION file is malformed. Please fix the new lines in the description field.

Description: CellAssign assigns cells measured with scRNA-seq to 
both known and de novo cell types based on the declaring certain 
genes as markers for different cell types. The result is the 
probability that each cell is of a given cell type, or of an 
"other" cell type that doesn't match any markers provided.

Thanks @lshep !

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.

kieranrcampbell commented 5 years ago

Slightly confused by this - the status here is OK but the build report shows errors on Windows and Linux, but with no output under the specified errors

lshep commented 5 years ago

Odd.... I'm going to kick off a manual build to see if that helps the other two...

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.

kieranrcampbell commented 5 years ago

@LiNk-NY as part of the review suggestions we have removed the automatic installation of tensorflow on package load, but this leads to build failure on Windows where it doesn't appear tensorflow is installed. We could opt to install on first use as per your suggestion but this would require verboseInstall = TRUE rather than FALSE as you suggested. Can you please advise on preference ? Thanks

lshep commented 5 years ago

tensorflow will not be able to be installed on our windows machine at this time. Our windows build machine has a lower version of python running than required for tensorflow installation - We plan on updating this in the near future. For now please ignore the ERROR on windows.

kieranrcampbell commented 5 years ago

Hi @LiNk-NY

We have addressed the majority of review points. cellassign now builds fine with the exception of errors on Windows, as @lshep discusses above. There are warnings on linux, but these appear to be deprecation warnings linked to the underlying numpy library. Below is what has been addressed out of the review, with follow up questions / comments about what has not:

DESCRIPTION

  • [X] Looks good, please keep the Description field limited to 80 width.

vignettes

  • Looks clean and concise.
  • [X] It would be good to stick to the 80 column width limit for readability and maintainership. This doesn't affect the ouput but makes it easier to go back and modify / read.
  • [X] Please show how the tf object was created
  • [X] Ensure that your installation instructions use BiocManager::install

data

  • example_rho seems like it could fit within the rowData of the SingleCellExperiment

We would prefer not to do this as typically a SingleCellExperiment will have 20k+ genes only of which ~ 20 will be markers, so encouraging storing of the marker info in the SingleCellExperiment encourages users to pass the entire SingleCellExperiment as input (a common mistake we have had collaborators do previously)

  • It may be better to generate the data from code in the examples than to provide a separate data file (exaxmple_sce and example_cellassign_fit)

  • Provide a unified object for representation such as SummarizedExperiment or SingleCellExperiment as a result of cellassign

Can the rationale behind this request be further clarified? It seems common in Bioconductor packages to have the option of the object to return, as is implemented in e.g. the scran packages "findMarkers" which has a sf.out option for whether to return an SCE or just the sizeFactors. Similarly here, we have a return_SCE option for whether to return the SingleCellExperiment or cellassign fit directly.

  • Is the holik data necessary for the package? It would be better to focus on the method implementation and provide a small example of how the method would run

This data is necessary for the vignette on constructing the marker matrix. The alternative would be to host documentation for a bioconductor package outside bioconductor.

R

  • [X] The cellassign function should return a class of the same name
  • [X] You should create accessor functions instead of using $ to access the list
  • Consider returning results as a SingleCellExperiment only or at least separating functions by their return type so that they are "pure". It's not recommended to have mixed return values / classes within a function

Please see question above.

  • Ideally you'd have an 'endomorphic' operation where the input class matches the output class
  • [X] Avoid glue and its dependencies by using sprintf()
  • Formatting code to fit within 80 column widths makes it easier to maintain and read

We have tried to do this where possible

Many thanks

Kieran

LiNk-NY commented 5 years ago

Thanks Kieran @kieranrcampbell For your thorough response. I will have a look at your package today. Best, Marcel

kieranrcampbell commented 5 years ago

Hi @LiNk-NY

Just wondering if there's any update on this? Thanks

LiNk-NY commented 5 years ago

Hi Kieran, @kieranrcampbell Apologies for the delay. Please push another version bump.

Below you will find the responses.

We would prefer not to do this as typically a SingleCellExperiment will have 20k+ genes only of which ~ 20 will be markers, so encouraging storing of the marker info in the SingleCellExperiment encourages users to pass the entire SingleCellExperiment as input (a common mistake we have had collaborators do previously)

It sounds like this could be a logical vector that you can keep track of in the object. I'm not sure to what level users will interact with the output. If you are doing most of the manipulation, it would be a good place to put it so to provide a more unified representation.

Can the rationale behind this request be further clarified? It seems common in Bioconductor packages to have the option of the object to return, as is implemented in e.g. the scran packages "findMarkers" which has a sf.out option for whether to return an SCE or just the sizeFactors. Similarly here, we have a return_SCE option for whether to return the SingleCellExperiment or cellassign fit directly.

Endomorphic operations are the usual route. Class methods can be used to extract particular data. It is based on the principle of function "purity" (meaning having a consistent return value).

This data is necessary for the vignette on constructing the marker matrix. The alternative would be to host documentation for a bioconductor package outside bioconductor.

This is still not clear to me why mock / abbreviated data can't be used. It is best to keep data within the package (especially for vignette examples).

Best regards, Marcel

bioc-issue-bot commented 5 years ago

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

7409202 Putative upgrade to tensorflow version 2; let's se...

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.

LiNk-NY commented 5 years ago

Hi Kieran! @kieranrcampbell Any ideas on what the Mac error is about? We can get your package into the next release if it is resolved soon. Best regards, Marcel

kieranrcampbell commented 5 years ago

Hi @LiNk-NY

We recently upgraded cellassign to use Tensorflow 2.0 which is a pretty big breaking change (in terms of tensorflow). The error

AttributeError: 'module' object has no attribute 'v1'

makes me think the version of Tensorflow on the linux build server is still 1.X -- is it possible to check this? (we call the v1 version to use tf 1 code within tf 2, all very confusing)

LiNk-NY commented 5 years ago

I am tagging Lori @lshep here. She can check whether this is the case. Thanks!

lshep commented 5 years ago

@LiNk-NY and @kieranrcampbell This is the case. It appears we are running 1.14.0 on the build system. If this a breaking change, we would wait until after the release in a week to do the system wide upgrade of tensorflow to avoid breaking packages that are using the existing version and now do not have enough time to apply changes to their package before the release. We will look at updating the version immediately after.

bioc-issue-bot commented 5 years ago

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

dab5367 Proper setting of random seeds in cellassign

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.

LiNk-NY commented 5 years ago

Hi Kieran, @kieranrcampbell

Setting a random.seed should always be done outside of the function call. Please remove the arguments in dab5367. If a user wants a reproducible result, the user will set the same seed before calling functions that make use of it (e.g., rnorm)

Best, Marcel

bioc-issue-bot commented 5 years ago

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

fcf161d Change seed setting

kieranrcampbell commented 5 years ago

Hi @LiNk-NY

Thanks, there was a bit of confusion since Tensorflow doesn't respect the R seed but I think I've found a workaround

Kieran

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 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, WARNINGS, 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:

a251f23 changes needed for basilisk integration

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.