Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bioc review changes #109

Merged
merged 31 commits into from
Apr 22, 2023
Merged

Bioc review changes #109

merged 31 commits into from
Apr 22, 2023

Conversation

multimeric
Copy link
Collaborator

@multimeric multimeric commented Apr 12, 2023

Fixes suggested in Bioconductor/Contributions#2991

  • Added spelling package to Suggests, and include it in tests. Technically this doesn't catch the "Angency" typo that was suggested, because that was inside a name field in the Description, which spelling never checks. But this in theory proves the lack of spelling mistakes elsewhere
  • Rewrote vignette to use a dynamic metadata URL. This allows us to use a minimal metadata file, generated by the new function downsample_metadata(), which should ensure the vignette builds much faster now
  • Added ORCiDs for Stefano and myself
  • Changed get_SingleCellExperiment() to snake case: get_single_cell_experiment(), with deprecation pathway
  • Added sentence on data hosting to vignettes
  • Removed eval=FALSE from all vignettes where relevant
  • Added sessionInfo() to vignette
  • Consolidated assert_that() calls into one where possible (those with custom error messages can't be)
  • Use force(data) which is functionally identical but has a clearer intent
  • Deleted tissue_label.csv
  • Use bioc 3.16 in CI
  • Don't print the size of the metadata file unless it will be downloaded
  • Compare usage of .rds and .hdf5 savaing methods

@multimeric multimeric marked this pull request as ready for review April 21, 2023 06:58
@multimeric
Copy link
Collaborator Author

Also I need input on this comment

Provide a short explain with regards to how the data is hosted and from what
project it originates.

Copy link
Owner

@stemangiola stemangiola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @multimeric

  1. we should add titles to all figures
  2. the figures with fewer data do not look so impressive or informative; we should add the PNG (recalculated) version with all data below each plot. We should add the text above each full-data version to explain why we are showing this.
  3. I am not sure what the difference is between

image

and

image

If they are redundant, let's drop one of them

@stemangiola stemangiola merged commit cfeaed2 into master Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants