stemangiola / tidyseurat

Seurat meets tidyverse. The best of both worlds.
https://stemangiola.github.io/tidyseurat/
154 stars 12 forks source link

Add slice_ functions #68

Closed wvictor14 closed 1 year ago

wvictor14 commented 1 year ago

Hi I found this issue on tidyomics and thought it would be good experience to try

Let me know how it looks and if there's any changes needed

This adds the slice_ family:

stemangiola commented 1 year ago

@wvictor14 have a look at the github action; it is failing. Check maybe on your local CHECK and BiocCheck because here I have to "allow" github action each time, unfortunately. After you become a contributor, github action will be runnable by you in the future.

stemangiola commented 1 year ago

@wvictor14 please add your authorship details here https://docs.google.com/spreadsheets/d/19XqhN3xAMekCJ-esAolzoWT6fttruSEermjIsrOFcoo/edit?usp=sharing

wvictor14 commented 1 year ago

@stemangiola I fixed the errors for CRAN check. There are two bioccheck errors that I am not sure best to address:

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

Also I realized that the option of inputting a tibble to order_by argument for slice_min and slice_max doesn't work with the select |> slice approach. So I added an internal function return_args that returns the variables as symbols of such cases, that can supply the necessary columns to select https://github.com/stemangiola/tidyseurat/pull/68/commits/7cca3f8116b4ce137c807db953d5de19f0a3d412

stemangiola commented 1 year ago

There are two bioccheck errors that I am not sure best to address

No problem, that could be another "challenge"

Also I realized that the option of inputting a tibble to order_by argument for slice_min and slice_max doesn't work with the select |> slice approach

Could you please give me minimal example ?

wvictor14 commented 1 year ago

Also I realized that the option of inputting a tibble to order_by argument for slice_min and slice_max doesn't work with the select |> slice approach

Could you please give me minimal example ?

This is what I mean

# in action
pbmc_small |> slice_min(tibble::tibble(nFeature_RNA, nCount_RNA), n = 2)

# return_args extracts the variables out of an expression
order_by <- expr(tibble::tibble(nFeature_RNA, nCount_RNA))
order_by_vars <- return_args(!!order_by)

# let's you input into select
pbmc_small |> select(!!!order_by_vars) |>

  # and then can apply slice_min
  slice_min(!!order_by, n = 6)

# A tibble: 6 × 2
  nFeature_RNA nCount_RNA
         <int>      <dbl>
1           26         51
2           29        157
3           29        172
4           30        150
5           30        202
6           31         62

Not sure if there is a more straightforward way, or how to make it more robust. What do you think?

stemangiola commented 1 year ago

Also I realized that the option of inputting a tibble to order_by argument for slice_min and slice_max doesn't work with the select |> slice approach

Could you please give me minimal example ?

This is what I mean

# in action
pbmc_small |> slice_min(tibble::tibble(nFeature_RNA, nCount_RNA), n = 2)

# return_args extracts the variables out of an expression
order_by <- expr(tibble::tibble(nFeature_RNA, nCount_RNA))
order_by_vars <- return_args(!!order_by)

# let's you input into select
pbmc_small |> select(!!!order_by_vars) |>

  # and then can apply slice_min
  slice_min(!!order_by, n = 6)

# A tibble: 6 × 2
  nFeature_RNA nCount_RNA
         <int>      <dbl>
1           26         51
2           29        157
3           29        172
4           30        150
5           30        202
6           31         62

Not sure if there is a more straightforward way, or how to make it more robust. What do you think?

Great! I left a last comment to address (Above) and then this PR is on its way!

wvictor14 commented 1 year ago

A detail. In the tidy transcriptomics framework, we adopt the standard of, no abbreviations and no acronyms.

Would you mind changing return_args into a self-explanatory function name, e.g. return_argumentsof... or anything else that would make sense.

This also applies to any variable, e.g. args_expr, args_vars, ...

I think I got them all!

stemangiola commented 1 year ago

Thanks a lot @wvictor14 for your PR! Well done.

Now that you are an expert, if you want to translate your PR to the other tidy adapters tidySingleCellExperiment and/or tidySummarizedExperiment feel free, that would be welcome!

No expectations of course.