ellispatrick commented 4 years ago

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

Repository: https://github.com/ellispatrick/spicyR

Confirm the following by editing each check box to '[x]'

[x ] I understand that by submitting my package to Bioconductor, the package source and all review commentary are visible to the general public.
[ x] I have read the Bioconductor Package Submission instructions. My package is consistent with the Bioconductor Package Guidelines.
[x] 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.
[x] My package addresses statistical or bioinformatic issues related to the analysis and comprehension of high throughput genomic data.
[x ] 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:

[ x] The 'devel' branch for new packages and features.
[x ] The stable 'release' branch, made available every six months, for bug fixes.
[x ] 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 commented 4 years ago

Hi @ellispatrick

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: spicyR
Type: Package
Title: Spatial analysis of in situ cytometry data
Version: 0.99.1
Author: Nicolas Canete and Ellis Patrick
Maintainer: Ellis Patrick <ellis.patrick@sydney.edu.au>
Description: spicyR provides a series of functions to aid in the analysis of both 
    immunofluorescence and mass cytometry imaging data as well as other assays that 
    can deeply phenotype individual cells and their spatial location. 
License: GPL (>=2)
biocViews: 
    SingleCell, CellBasedAssays
Encoding: UTF-8
LazyData: true
VignetteBuilder: knitr
Imports: ggplot2, class, concaveman, grid, BiocParallel, spatstat, lmerTest, BiocGenerics, S4Vectors, lme4, methods, mgcv, pheatmap,
     purrr, rlang, grDevices, IRanges
Suggests: BiocStyle, knitr, rmarkdown
Depends: R (>= 4.0.0)
RoxygenNote: 7.0.2

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.

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: "TIMEOUT, 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:

d25f173 version bump

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

2013791 Shortened examples

bioc-issue-bot commented 4 years ago

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

1b38732 Shortened examples.

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:

bd8267a Version bump. Updated examples to make things quic...

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

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

0005751 Made example data smaller 0032bed Removed R dependency

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.

bioc-issue-bot commented 4 years ago

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

fd76938 Added R dependency 3.5

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.

bioc-issue-bot commented 4 years ago

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

d1a7ea7 version bump with R v4 dependency

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.

ellispatrick commented 4 years ago

Hi @LiNk-NY there seems to be some conflicting advice on slack with how to deal with the R dependency warning.

LiNk-NY commented 4 years ago

Hi Ellis, @ellispatrick

Thank you for your submission to Bioconductor. I have taken a look at your package. Please see the short review below. If you have any questions, feel free to respond on this thread.

WRT the R dependency warning, please remove R (>= 4.0.0) from the DESCRIPTION file. We have R version limits to each release of Bioconductor already set in place so this is redundant.

Best regards, Marcel

spicyR #1447

R CMD check

The data should be small in order to demonstrate functionality

* checking installed package size ... NOTE
  installed size is  5.7Mb
  sub-directories of 1Mb or more:
    data      2.6Mb
    extdata   2.7Mb

Make sure all variables are accounted for:

* checking R code for possible problems ... NOTE
signifPlot: warning in pheatmap(pVal, col = pal, breaks = breaks,
  cluster_rows = FALSE, cluster_cols = FALSE): partial argument match
  of 'col' to 'color'
.show_spicy: no visible binding for global variable ‘p.adjust’
hatchingPlot: no visible binding for global variable ‘x’
hatchingPlot: no visible binding for global variable ‘y’
makeWindow : <anonymous>: no visible global function definition for
  ‘rnorm’
signifPlot: no visible global function definition for ‘p.adjust’
spatialLM: no visible global function definition for ‘predict’
spatialLM: no visible global function definition for ‘lm’
spatialMEM: no visible global function definition for ‘predict’
top,spicy: no visible global function definition for ‘p.adjust’
top,spicy: no visible global function definition for ‘Which’
Undefined global functions or variables:
  Which lm p.adjust predict rnorm x y
Consider adding
  importFrom("stats", "lm", "p.adjust", "predict", "rnorm")
to your NAMESPACE file.

DESCRIPTION

It looks good.
It's recommended to use an Authors@R field instead of the Author & Maintainer combo.
Use LazyData: true if you have small datasets that don't take a long time to load.
Include a BugReports: field in the DESCRIPTION with a link to the GitHub repo

NAMESPACE

(Optional) Bioconductor convention has a class names's first letter capitalized
Some names seem pretty general / vague: top, location

vignettes/

For readability and easy maintenance, keep an 80 column width limit to text.
Include an installation and sessionInfo() section
It's good to have broken up topics by vignette :+1:

R

Given the number of Bioconductor classes available, is there a class that could accommodate the needs of spicy? A list seems too simple.
Although it's the same, you could use setReplaceMethod instead of location<- for readability and easier identification
Avoid the use of @. There should be methods for data extraction, otherwise you can create accessor functions for those data
Register an S3 method as.data.frame for your class don't create an S4 generic. Preferably, create an as(x, "data.frame") coercion method.
Avoid using cat and use message instead. Use cat only for show methods
Where possible, use name-based subsetting instead of index-based for robustness
There are two plot.segmentedCells. Should there only be one?
If you have default names in cellData, why not use those as the default argumetns for the rest of the function segmentedCells and avoid creating new columns?
Minor: You can use grepl instead of length(grep(...)) == 0

bioc-issue-bot commented 4 years ago

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

c8f90a3 Added install and sessionInfo to vignettes ecd31b8 Changed names of morphology, intensity and locatio... e0bf8a2 Removed @ Set defaults for SegmentedCells replaced... c43d152 Added SetReplaceMethod Removed duplicate segmented...

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.

bioc-issue-bot commented 4 years ago

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

4a2a7f0 Make some lines shorter

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.

bioc-issue-bot commented 4 years ago

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

8f8df26 Reverted vignette to fix evals error

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.

ellispatrick commented 4 years ago

Hi @LiNk-NY Thanks for all of suggestions. They were helpful. I believe we have address nearly all of them.

R CMD check [x] The data should be small in order to demonstrate functionality

checking installed package size ... NOTE installed size is 5.7Mb sub-directories of 1Mb or more: data 2.6Mb extdata 2.7Mb

[x] Make sure all variables are accounted for

checking R code for possible problems ... NOTE signifPlot: warning in pheatmap(pVal, col = pal, breaks = breaks, cluster_rows = FALSE, cluster_cols = FALSE): partial argument match of 'col' to 'color' .show_spicy: no visible binding for global variable ‘p.adjust’ hatchingPlot: no visible binding for global variable ‘x’ hatchingPlot: no visible binding for global variable ‘y’ makeWindow : : no visible global function definition for ‘rnorm’ signifPlot: no visible global function definition for ‘p.adjust’ spatialLM: no visible global function definition for ‘predict’ spatialLM: no visible global function definition for ‘lm’ spatialMEM: no visible global function definition for ‘predict’ top,spicy: no visible global function definition for ‘p.adjust’ top,spicy: no visible global function definition for ‘Which’ Undefined global functions or variables: Which lm p.adjust predict rnorm x y Consider adding importFrom("stats", "lm", "p.adjust", "predict", "rnorm") to your NAMESPACE file. DESCRIPTION [x] It looks good. [x] It's recommended to use an Authors@R field instead of the Author & Maintainer combo. [x] Use LazyData: true if you have small datasets that don't take a long time to load.

I get a size warning when I do this. I've skipped it.

[x] Include a BugReports: field in the DESCRIPTION with a link to the GitHub repo NAMESPACE [x] (Optional) Bioconductor convention has a class names's first letter capitalized Some names seem pretty general / vague: top, location

I have changed top to topPairs, change location to cellSummary, intensity to cellMarks, morphology to cellMorph and pheno to imagePheno to make them sound less vague

vignettes/ [x] For readability and easy maintenance, keep an 80 column width limit to text.

It is still not a strict 80 but much closer

[x] Include an installation and sessionInfo() section [x] It's good to have broken up topics by vignette 👍

R [x] Given the number of Bioconductor classes available, is there a class that could accommodate the needs of spicy? A list seems too simple.

I haven't been able to find any as we want to store the pairwise comparisons. I have renamed the class SpicyResults for more clarity.

[x] Although it's the same, you could use setReplaceMethod instead of location<- for readability and easier identification

[x] Avoid the use of @. There should be methods for data extraction, otherwise you can create accessor functions for those data

Replaced @listData with slot(x,"listData")

[x] Register an S3 method as.data.frame for your class don't create an S4 generic. Preferably, create an as(x, "data.frame") coercion method.

[x] Avoid using cat and use message instead. Use cat only for show methods

[x] Where possible, use name-based subsetting instead of index-based for robustness

[x] There are two plot.segmentedCells. Should there only be one? If you have default names in cellData, why not use those as the default argumetns for the rest of the function segmentedCells and avoid creating new columns? [x] Minor: You can use grepl instead of length(grep(...)) == 0

LiNk-NY commented 4 years ago

Hi Ellis, @ellispatrick Thanks for making those changes.

You may want to export the S3 as.data.frame.SegmentedCells method so that it be available to users.
Add a Depends: field in the DESCRIPTION file with R (>= 4.0.0) due to having serialized data in the package.
Minor: You may want to have an un-exported extractor function for listData say something like:
```
.getData <- function(x)
getElement(x, "listData")
```
to make the code a bit more clean.

It's looking good!

bioc-issue-bot commented 4 years ago

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

3c4af8d Exported as.data.frame S3 Added a .putData() funct... 0ac7784 Merge branch 'master' of https://github.com/ellisp...

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.

ellispatrick commented 4 years ago

Thanks again @LiNk-NY

I can definetely see how all of these things are going to help me in the future, onwards and upwards :)

I exported as.data.frame and added back in the R dependency. I also wrote .putData() function to write to listData

LiNk-NY commented 4 years ago

@ellispatrick Thanks for making those changes!

It's almost good to go.

Make sure you are updating your documentation as you make changes. Ensure that you have the latest version of roxygen2 (>= 7.1.0) and that your file names match the capitalization of the class. For example, segmentedCells-class.R should be SegmentedCells-class.R for consistency.

bioc-issue-bot commented 4 years ago

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

b2259c5 Updated Roxygen to 7.1.0 Fixed name of plot-Segmen...

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.

ellispatrick commented 4 years ago

Thanks @LiNk-NY !

Done :)

LiNk-NY commented 4 years ago

Hi Ellis, @ellispatrick

The file names issue is not major. Thank you for your contribution!

Your package has been accepted.

Best regards, Marcel

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/ellispatrick.keys is not empty), then no further steps are required. Otherwise, do the following:

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("spicyR"). The package 'landing page' will be created at

https://bioconductor.org/packages/spicyR

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.

Bioconductor / Contributions

spicyR #1447

spicyR #1447

R CMD check

DESCRIPTION

NAMESPACE

vignettes/

R