Bioconductor submission pre-review

[lazappi]

This is a pre-review of the package to make the submission process smoother.
We should try to address as many of these as possible before submitting.
It might make sense to create sub-issues for some of these that are bigger tasks and/or need discussion.
Try to keep this checklist up to date as we fix things.

Review

Key: 🚨 Required ⚠️ Recommended 🟢 Optional ❓ Question

My comments/suggestions/explanations are in italics

General package development

• 🚨 Fix the NOTEs from R CMD build

  WARNING: Added dependency on R >= 4.1.0 because package code uses the
   pipe |> or function shorthand \(...) syntax added in R 4.1.0.
   File(s) using such syntax:
     ‘Seurat.R’ ‘SingleCellExperiment.R’
  checking for unstated dependencies in vignettes ... NOTE
    'library' or 'require' calls not declared from:
      ‘dplyr’ ‘rprojroot’ ‘stringr’ ‘tibble’ ‘tidyr’
  

• ⚠️ Fix NOTEs and WARNINGs from BiocCheck::BiocCheck(new-package = TRUE)

  ℹ NOTE: Update R version dependency from 4.0.0 to 4.5.0
  ! WARNING: No Bioconductor dependencies detected. Note that some infrastructure packages may not have Bioconductor dependencies. For more information, reach out to the Bioconductor community and/or consider a CRAN submission.
  ℹ NOTE: 'sessionInfo' not found in vignette(s)
  ℹ NOTE: Avoid sapply(); use vapply()
  ℹ NOTE: Avoid 'cat' and 'print' outside of 'show' methods
  ! WARNING: .Deprecated / .Defunct usage (found 5 times)
  ℹ NOTE: Use accessors; don't access S4 class slots via '@' in examples/vignettes.
  ℹ NOTE: Avoid 'suppressWarnings'/'*Messages' if possible (found 3 times)
  ℹ NOTE: The recommended function length is 50 lines or less. There are 14 functions greater than 50 lines.
  ℹ NOTE: Usage of dontrun / donttest tags found in man page examples. 7% of man pages use at least one of these tags.
  ℹ NOTE: Use donttest instead of dontrun.
  ℹ NOTE: Consider shorter lines; 273 lines (4%) are > 80 characters long.
  ℹ NOTE: Consider multiples of 4 spaces for line indents; 2342 lines (31%) are not.
  ℹ NOTE: Cannot determine whether maintainer is subscribed to the Bioc-Devel mailing list (requires admin credentials). Subscribe here: https://stat.ethz.ch/mailman/listinfo/bioc-devel
  

• 🚨 Fix ERROR from BiocCheck::BiocCheckGitClone()

  ✖ ERROR: System files found that should not be Git tracked.
  • anndataR.Rproj
  ℹ NOTE: (Optional) CITATION file not found. Only include a CITATION file if there is a preprint or publication for this Bioconductor package.
  

DESCRIPTION file

• 🚨 Should depend on the version of R used for the next release (R>=4.5.0)

• ⚠️ Currently there are no Bioconductor packages in the Imports field

  This should be ok because we make extensive use of {SingleCellExperiment} through Suggests. We could be asked to use {rhdf5} rather than {hdf5r} though.

• 🚨 Dependencies used in vignettes should be added to Suggests

NAMESPACE file

• 🚨 Function names use camelCase or snake_case and do not include .

  We do have a mix of camelCase and snake_case but only for class names so I think that should be fine

The NEWS file

• 🚨 The NEWS file should be kept up to date

  For other packages I keep a summary of changes for each release (or submission in this case) and a more detailed changelog somewhere else. There are a few ways to do this if we want to.

Documentation

README

• 🚨 The README file should include Bioconductor installation instructions

Vignette

• 🚨 The main vignette should have an Introduction section

  • 🚨 Include motivation for inclusion in Bioconductor
  • 🚨 Includes comparison to similar packages (if applicable)

• 🚨 The main vignette should have an Installation section

  • 🚨 Include Bioconductor installation instructions

• 🚨 Describe the core functionality of the package

  I think we already do a great job of this, just making a note that we should check/update the vignettes before submitting

• 🚨 Some vignettes need to add a sessionInfo() chunk

• ⚠️ The vignettes/ directory should contain only vignette file(s), bibtex files and necessary static images

  There is some code here to create the architecture diagram. I think it should be fine but making a note that a reviewer might bring it up.

Code

R

• 🚨 Use vapply instead of sapply

• 🚨 Use accessors instead of @ or slot() (where possible)

  There was a note about this so we should check, probably in the Seurat stuff

• 🚨 Use message(), warning(), stop() instead of cat()

  There is a note about this but I think it is in the print methods for the R6 classes. It's probably a false positive but we should check.

• 🟢 Split up long functions where it makes sense to do so

  There are a few functions that are a bit long, I don't think it will be an issue but we should check to see if it makes sense to restructure anything

• 🟢 Consider adding validity checks for function arguments

  We have some of these but we could look at adding more and/or standardising them

• ⚠️ Check if for loops can be replaced

  Most of these make sense but there could be a couple we can replace

• ⚠️ Remove deprecated code

  There is a warning about deprecated code. We can probably argue for why this should be kept for the next release cycle.

Additional files

• 🚨 The .Rproj file should be removed from Git

[LouiseDck]

Checked:

🚨 Use message(), warning(), stop() instead of cat()

It is indeed used for the R6 object print messages, and in the development status and known_issues vignettes, which seems fine to me. (It is not used for error or warning messages)

[LouiseDck]

Since the package has been updated a lot since this pre-review, I ran R CMD build, R CMD check, BiocCheck::BiocCheckGitClone() and BiocCheck::BiocCheck('new-package'=TRUE), as mentioned in the Bioconductor package instructions.

Additionally, the guidelines specify:

The package should require less than 10 minutes to run R CMD check --no-build-vignettes

On my system this takes about 8 minutes.

R CMD build

No warnings or errors

R CMD check

• 1 ERROR

  * checking for file ‘./DESCRIPTION’ ... ERROR
  Required fields missing or empty:
    ‘Author’ ‘Maintainer’
  

  I'm not sure why it's not picking up this information, according to the guidelines I believe we specify this correctly?

BiocCheckGitClone

• 1 NOTE

  ℹ NOTE: (Optional) CITATION file not found. Only include a CITATION file if
  there is a preprint or publication for this Bioconductor package. Note that
  Bioconductor packages are not required to have a CITATION file but it is useful
  both for users and for tracking Bioconductor project-wide metrics. When
  including a CITATION file, add the publication using the 'doi' argument of
  'bibentry()'.
  

  Not (yet) relevant

BiocCheck ✖ 3 ERRORS | ⚠ 3 WARNINGS | ℹ 10 NOTES

• ✖ ERROR: New package 'y' version not 99 (i.e., x.99.z); Package version: 0.2.0

• ✖ ERROR: At least 80% of man pages documenting exported objects must have runnable examples.

  The following pages do not:
  from_Seurat.Rd
  from_SingleCellExperiment.Rd
  to_Seurat.Rd
  to_SingleCellExperiment.Rd
  

  These are the deprecated functions, and the functions that should be used all have runnable examples (as_Seurat, as_AnnData, as_SingleCellExperiment)

• ✖ ERROR: Unable to find your email in the Support Site: HTTP 404 Not Found.

• ! WARNING: No Bioconductor dependencies detected. Note that some infrastructure packages may not have Bioconductor dependencies. For more information, reach out to the Bioconductor community and/or consider a CRAN submission.

  extensive use of {SingleCellExperiment} and {rhdf5} through Suggests.

• ! WARNING: Empty or missing \value sections found in man page(s).

  Found in files:
  man/AbstractAnnData.Rd
  man/AnnData-usage.Rd
  man/HDF5AnnData.Rd
  man/InMemoryAnnData.Rd
  

  I'm not quite sure what this refers to, does this refer to a missing @return in the documentation? If so, these are all the R6 constructors.

• ! WARNING: .Deprecated / .Defunct usage (found 7 times)

  We can probably argue for why this should be kept for the next release cycle. --> What is the argument? Breaking changes and existing users?

• ℹ NOTE: Update R version dependency from 4.1.0 to 4.5.0

• ℹ NOTE: Vignette(s) found with missing chunk labels

• ℹ NOTE: Avoid 'cat' and 'print' outside of 'show' methods

  print methods for the R6 class

• ℹ NOTE: Avoid 'suppressWarnings'/'*Messages' if possible (found 3 times)

  suppressWarnings() in R/as_Seurat.R (line 495, column 16)
  suppressWarnings() in R/as_Seurat.R (line 537, column 15)
  suppressWarnings() in R/as_Seurat.R (line 620, column 13)
  

• ℹ NOTE: The recommended function length is 50 lines or less. There are 18 functions greater than 50 lines.

  as_Seurat() (R/as_Seurat.R): 172 lines
  from_Seurat() (R/from_Seurat.R): 168 lines
  as_SingleCellExperiment() (R/as_SingleCellExperiment.R): 147 lines
  initialize() (R/HDF5AnnData.R): 141 lines
  ...
  

  I've had another look, and while the functions are long, I do think they are well-structured. Initializing the AnnData objects, converting them, and generating the datasets, requires quite some code resulting in long functions, but these are structured logically. I really don't think 50 lines would be at all feasible for most of these (while keeping good structure and style).

• ℹ NOTE: Usage of dontrun / donttest tags found in man page examples. 6% of man pages use at least one of these tags. (generate_dataset.Rd)

  Found in files:
  generate_dataset.Rd
  

  I'm not entirely sure why we have this dontrun statement: they are by far not the longest examples to run. (read_h5ad and as_AnnData take quite a bit longer)

• ℹ NOTE: Use donttest instead of dontrun. (generate_dataset.Rd)

  Found in files:
  generate_dataset.Rd
  

• ℹ NOTE: Consider shorter lines; 151 lines (2%) are > 80 characters long.

  Currently the lintr specifies 120 characters. Is it necessary to change this?

• ℹ NOTE: Consider multiples of 4 spaces for line indents; 2333 lines (32%) are not.

  Is this something we should fix? If so, we should update the styling, as we currently use 2 spaces for line indents

• ℹ NOTE: Cannot determine whether maintainer is subscribed to the Bioc-Devel mailing list (requires admin credentials). Subscribe here: https://stat.ethz.ch/mailman/listinfo/bioc-devel

[lazappi]

No warnings or errors

R CMD check

* [ ]  1 ERROR
  ```
  * checking for file ‘./DESCRIPTION’ ... ERROR
  Required fields missing or empty:
    ‘Author’ ‘Maintainer’
  ```

  _I'm not sure why it's not picking up this information, according to [the guidelines](https://contributions.bioconductor.org/description.html) I believe we specify this correctly?_

I think it's probably because we use Authors@R instead of the separate Maintainer and Author fields. Should be ok to ignore.

  [x]  ✖ ERROR: At least 80% of man pages documenting exported objects must have runnable examples.
  ```
  The following pages do not:
  from_Seurat.Rd
  from_SingleCellExperiment.Rd
  to_Seurat.Rd
  to_SingleCellExperiment.Rd
  ```       
    
  _These are the deprecated functions, and the functions that should be used all have runnable examples (`as_Seurat`, `as_AnnData`, `as_SingleCellExperiment`)_

I think we should probably remove the deprecated stuff before we submit (maybe should have raised the deprecation level in the last release)

  [ ]  ✖ ERROR: Unable to find your email in the Support Site: HTTP 404 Not Found.

Whoever is the maintainer needs to register on the support site with the same email

  [ ]  ! WARNING: Empty or missing \value sections found in man page(s).
  ```
  Found in files:
  man/AbstractAnnData.Rd
  man/AnnData-usage.Rd
  man/HDF5AnnData.Rd
  man/InMemoryAnnData.Rd
  ```

  _I'm not quite sure what this refers to, does this refer to a missing `@return` in the documentation? If so, these are all the R6 constructors._

Yep. I don't think the checks really handle R6 so we can ignore this.

  [ ]  ℹ NOTE: Avoid 'suppressWarnings'/'*Messages' if possible (found 3 times)
  ```
  suppressWarnings() in R/as_Seurat.R (line 495, column 16)
  suppressWarnings() in R/as_Seurat.R (line 537, column 15)
  suppressWarnings() in R/as_Seurat.R (line 620, column 13)
  ```  

Should check if we can avoid these but I think we added them deliberately

[rcannood]

I suppose this can be closed now?

[lazappi]

Yeah, I think so. I meant to do that earlier.

Bioconductor submission at Bioconductor/Contributions#3907.