stemangiola / tidyseurat

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

Update `slice_sample` in `dplyr_methods.R` #72

Closed noriakis closed 1 year ago

noriakis commented 1 year ago

Hello, this PR clears the warning in slice_sample. I would be grateful if you could review the code. Related to: https://github.com/stemangiola/tidySingleCellExperiment/pull/90

> data(pbmc_small)
> pbmc_small |> slice_sample(n=0)
Error: No cells found
noriakis commented 1 year ago

Thank you for your guidance! In this case, I added the unit test for slice_sample() expecting the error (expect_error). Would it be okay?

Edit: I apologize for the recurrent commit, but I thought I should have included expect_no_warning as well.

stemangiola commented 12 months ago

Hello @wvictor14 @noriakis In the Seurat master, the slice functions produce errors in the tests. Could you please have a look?

Thanks!

noriakis commented 12 months ago

Hello @stemangiola @wvictor14,

expect_equal(
    pbmc_small |> as_tibble() |> arrange(nFeature_RNA) |> head(n = 5) %>% pull(.cell),
    pbmc_small |> slice_min(nFeature_RNA, n = 5) |> colnames()
)

The errors seem to come from above tests in slice_min and slice_max, where the subset.Seurat by cells returns the object that is preserving the original order of the cells. The returned row numbers of cells inside the function are correct, so one option would be to adding sort() to both vectors which removes the errors.

expect_equal(
    pbmc_small |> as_tibble() |> arrange(nFeature_RNA) |> head(n = 5) %>% pull(.cell) |> sort(),
    pbmc_small |> slice_min(nFeature_RNA, n = 5) |> colnames() |> sort()
)

The arrange() function is currently deprecated, so maybe we could replace the test?

stemangiola commented 12 months ago

The arrange() function is currently deprecated, so maybe we could replace the test?

Agree, if you had the chance to do it, would be amazing.