CATALYST: Cytometry dATa anALYSis Tools

[HelenaLC]

• Repository: https://github.com/HelenaLC/CATALYST

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][2] instructions. My package is consistent with the Bioconductor [Package Guidelines][1].

• [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][3] for issues that users may have, subscribing to the [bioc-devel][4] 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 [Subversion][5] (optionally [via GitHub][6]).

[bioc-issue-bot]

Hi @HelenaLC

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: CATALYST
Type: Package
Title: Cytometry dATa anALYSis Tools
Version: 0.99.0
Author: Helena Lucia Crowell <crowellh@student.ethz.ch>, Mark Robinson
    <mark.robinson@imls.uzh.ch>, Vito Zanotelli <vito.zanotelli@uzh.ch>
biocViews: MassSpectrometry, Preprocessing, StatisticalMethod, SingleCell
Maintainer: Helena Lucia Crowell <crowellh@student.ethz.ch>
Depends:
    R (>= 3.3)
Description: Tools for (pre)processing and analysis of cytometry data.
Imports:
    flowCore,
    ggplot2,
    graphics,
    grDevices,
    grid,
    gridExtra,
    matrixStats,
    methods,
    RColorBrewer,
    stats,
    utils
License: GPL (>=2)
LazyData: TRUE
VignetteBuilder: knitr
RoxygenNote: 6.0.1
Suggests: BiocStyle, knitr, rmarkdown

[bioc-issue-bot]

Your package has been approved for building. Your package is now submitted to our queue.

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]

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

http://bioconductor.org/spb_reports/CATALYST_buildreport_20170324184751.html

[mtmorgan]

@LiNk-NY will review this package, but I think the Description: field of the DESCRIPTION file should be expanded to provide users with a reasonable overview of the specific functionality this package provides, perhaps in contrast or relation to other similar packages.

[bioc-issue-bot]

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

684ab6f bump version

[bioc-issue-bot]

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

http://bioconductor.org/spb_reports/CATALYST_buildreport_20170326074717.html

[mtmorgan]

Thank you for your contribution. Please update your package in response to the following comments, providing a brief written summary when complete. Remember to bump your version to trigger a re-build.

DESCRIPTION

• Please provide additional detail about the unique functionality of your package in the Description: field. Like a paper abstract, so users can easily assess the merits and relevance of your contribution.

vignette

• looks good!

R

• Consider adopting consistent argument naming in setGeneric, sometimes it's x, sometimes object.

• For some generics, e.g., outFCS, it seems likely that not all arguments are required for dispatch, e.g., out_path= is likely always to be a character(1), or perhaps missing in which case there is an 'obvious' default. Use signature="x" to avoid unnecessarily complicated dispatch signatures.

• Optional. Stylistically, I find it clearer to write tryCatch() clauses in blocks, e.g., from applyCutoffs.R:89

  value <- tryCatch({
      solve(covMat) %*% covMat
  }, error=function(e) {
      e
  })

  and to avoid deeply nested clauses as in the if() statement

  value <- tryCatch({
      solve(covMat) %*% covMat
  }, error=function(e) {
      e
  })
  if (!any(class(value) == "error") {
      ...
  }

  the any(class(value) == "error") is better written as inherits(value, "any").

• In several places, if() statements are nested leading to highly indented code. It would be easier to read if the tests were placed in a single clause (maybe with the calculation of the test outside the clause) and thus indentation one layer less. E.g., compCytof.R:129

  test <- (length(add) != 0) && (any(ind <- old_ms %in% new_ms))
  if (test) {
      ...
  }

• Generally, one wants to avoid direct slot access, e.g., computeSpillmat.R:65, and provide a simple accessor (not necessarily a generic, or exported) to isolate the details of implementation from the desired interface.

• Carefully consider sequences generated by 1:n, which are surprising when n == 0; use seq_len(n) or seq_along(x) instead.

• concatFCS.R:47 it looks like cumsum() might be useful here (outside the loop)?

• The pattern at estTrim.R:60

  df <- rss <- NULL
  for (trim in trim_vals) {
      ...
      for (id in ids) 
          df <- rbind(df, data.frame(m=matrixStats::colMedians(
              comped[bc_ids(x) == id, ms != id]), trim=trim))

  involves 'copy-and-append' (also plotMahal.R:71), and is very inefficient. pre-allocate-and-fill to avoid excessive copying, or use lapply() to avoid need for direct memory management. See robust code.

• As a 'best practice', use vapply() rather than sapply() to (a) ensure functions return appropriate values and (b) protect against heterogenous results when the sapply() is over a zero-length vector (sapply(1:5, sqrt) returns a numeric vector, but sapply(numeric(0), sqrt) returns a list!).

• stop() often does not require a nested paste(), e.g., at outFCS.R:54

  stop("Only ", length(out_nms), " file names provided but ", 
       nrow(bc_key(x)), " needed.")

• cat() should only be used for show() or print() methods; otherwise, use message().

man

• Generally, documentation looks quite complete.

[bioc-issue-bot]

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

46e10fe bug fix 646bfd8 bug fix 4e36302 in sep_cutoffs replacement -> return named vector 82bbf4c + plot_estTrim(): separate plotting function for e... 7462bc6 change object -> x 893b33b change cat -> message bea5fac + Imports: plotly 01545d4 + importFrom plotly ggplotly cb452df test outside if statement 9dc8711 change object -> x and @ -> accessor 4029fef remove paste inside stop, change @ -> accessor and... 2479652 change 1:n_bcs -> seq_len(n_bcs) 74a69c4 change @ -> accessor 2b6e0c4 essential do-over - use lapply/sapply only - plot ... 3f95bce change object -> x and @ -> accessor ec3040b change 1:n_beads -> seq_len(n_beads) 4a2ac39 changed estTrim() and computeSpillmat() examples 7594615 test outside if statement ad8804b change 1:n -> seq_len(n) 4bc502b change 1:N -> seq_len(N) 9857483 change 1:n_beads -> seq_len(n_beads) 6352927 changed value and example 038bfdd changed example 6a26864 added examples to illustrate some functionality 80431aa changed @ -> accessor 3197e09 use cumsum() 62c9d2e change @ -> accessor, need to use object here a44f5bf reformat to shorten lines 82159f3 reformat 3de38e2 change order of if statement tests 9e6d529 reformat 72e8384 fix in method for signature x="character" ffa6fd6 update man a780ded update man 2715fe2 fix example fc3d798 line 71: initilize plot list of fixed length and f... e226516 bug fix 2bc7e4d bug fix 94dae45 update man 876a999 fix bug 55ced5c + description and version bump 4582cb8 changed warning c715750 sapply -> vapply 31ac244 sapply -> vapply d22884d sapply -> vapply 4764648 sapply -> vapply 70e9d6e sapply -> vapply 63ed6bd remove unnecessary sapply

[bioc-issue-bot]

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

http://bioconductor.org/spb_reports/CATALYST_buildreport_20170405063310.html

[HelenaLC]

Thank you for your swift feedback Martin! I'm fairly new at this and apologise for the unnecessary stylistic flaws.

DESCRIPTION:

• Please provide additional detail about the unique functionality of your package...
  • done.

R:

• Consider adopting consistent argument naming in setGeneric, sometimes it's x, sometimes object.
  • Changed all to x.
  • Note: Maybe it's worth considering to add exprs to the BiocGenerics package?
• For some generics, e.g., outFCS, it seems likely that not all arguments are required for dispatch...
  • apologies, but I don't quite follow. If I understand correctly, removing out_path from the generic will prevent checking signature "character" or "missing" for this argument. Should the method then be defined only for signature(x=...) and the testing for the validity of out_path be done inside the function, with an if clause?
• Optional. Stylistically, I find it clearer to write tryCatch()clauses in blocks...
  • done.
• In several places, if() statements are nested leading to highly indented code...
  • done.
• Generally, one wants to avoid direct slot access...
  • done.
• Carefully consider sequences generated by1:n...
  • done.
• concatFCS.R:47 it looks like cumsum() might be useful here (outside the loop)?
  • yes, thank you! Much nicer now I believe.
• The pattern at estTrim.R:60...
  • re-wrote estTrim() using lapply/vapply/sapply, no more loops
  • plot list in plotMahal() is pre-initialised now, with a fixed length, and filled
• As a 'best practice', use vapply() rather than sapply()...
  • done wherever the length of the output is consistent.
• stop() often does not require a nested paste()...
  • done.
• cat() should only be used for show() or print() methods; otherwise, use message().
  • done.

Additional changes:

• essential do-over of estTrim() as I got comments that it wasn't as informative as I believed it could be.
  • all computations done using lapply(), vapply(), sapply() - no more loops
  • output is now a re-styled and interactive ggplotly() plot
  • only dots corresponding to interactions computeSpillmat() actually considers are displayed

[mtmorgan]

Thanks for your own speedy updates!

For the generic signature, it seems that out_path will always be a character(1), and that there's a reasonable default (tempdir()), so something like

setGeneric("outFCS",
    function(x, out_path = tempdir(), ...) standardGeneric("outFCS"),
    signature = "x"
)

setMethod("outFCS", "dbFrame", function(x, out_path=tempdir()) {
    stopifnot(is.character(out_path), length(out_path) == 1L)
    ...
})

[bioc-issue-bot]

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

c53f647 changed outFCS() generic/method fc298fb sapply -> vapply bb78995 sapply -> vapply 00fc3b6 version bump

[bioc-issue-bot]

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

http://bioconductor.org/spb_reports/CATALYST_buildreport_20170406080836.html

[mtmorgan]

Thanks, I'm marking this package as accepted. It will be added to the Bioconductor svn repository and nightly builds, with additional instructions by email in a few days.