knncolle / BiocNeighbors

Clone of the Bioconductor repository for the BiocNeighbors package.
https://bioconductor.org/packages/devel/bioc/html/BiocNeighbors.html
6 stars 11 forks source link

Change in expected argument type `findKNN(subset=...)` #26

Closed MikeDMorgan closed 3 weeks ago

MikeDMorgan commented 3 weeks ago

Hi @LTLA - there seems to be an undocumented change to findKNN. Previously the subset argument would take a character vector input, but in Bioc 3.20 this leads to C++ type error:

Error: Not compatible with requested type: [type=character; target=integer].

13 . | stop(structure(list(message = "Not compatible with requested type: [type=character; target=integer].", call = NULL, cppstack = NULL), class = c("Rcpp::not_compatible", "C++Error", "error", "condition")))
-- | --

12. | generic_find_knn(X@ptr, num_neighbors = as.integer(k), force_variable_neighbors = is(k, "AsIs"), chosen = subset, num_threads = num.threads, last_distance_only = FALSE, report_index = !isFALSE(get.index), report_distance = !isFALSE(get.distance))
-- | --

11. | .local(X, k, get.index, get.distance, num.threads, subset, ..., BNPARAM = BNPARAM)
-- | --

10. | findKNN(ptr, k = k, get.index = get.index, get.distance = get.distance, num.threads = num.threads, subset = subset, ..., BPPARAM = BPPARAM)
-- | --

9. | findKNN(ptr, k = k, get.index = get.index, get.distance = get.distance, num.threads = num.threads, subset = subset, ..., BPPARAM = BPPARAM)
-- | --

8. eval(call, parent.frame())

7. eval(call, parent.frame())

6. | callGeneric(ptr, k = k, get.index = get.index, get.distance = get.distance, num.threads = num.threads, subset = subset, ..., BPPARAM = BPPARAM)
-- | --

5. | .local(X, k, get.index, get.distance, num.threads, subset, ..., BNPARAM = BNPARAM)
-- | --

4. findKNN(all_reduced_dims, k = nrow(nh_reduced_dims) + 1, subset = rownames(nh_reduced_dims))

3. findKNN(all_reduced_dims, k = nrow(nh_reduced_dims) + 1, subset = rownames(nh_reduced_dims)) at makeNhoods.R#188

2. .refined_sampling(random_vertices, X_reduced_dims, k) at makeNhoods.R#110

1. makeNhoods(traj_milo, prop = 0.1, k = 10, d = 30, refined = TRUE)

The obvious get around is for me to switch to using an integer/logical vector in miloR, but then that also means I need to add in a few extra checks on dimnames setting, etc.

Could either the documentation be updated to reflect this change in behaviour, or allow character vector input to subset?

Cheers me 'ol mucker.

LTLA commented 3 weeks ago

Oops. Should be fixed in the HEAD, pushed to BioC-devel as 1.99.3.

MikeDMorgan commented 3 weeks ago

Cheers, much obliged.