Hvg #330
Hvg #330
Conversation
scanpy/preprocessing/recipes.py
Outdated
| @@ -2,6 +2,7 @@ | |||
| """ | |||
|
|
|||
| from . import simple as pp | |||
| from ._deprecated.highly_variable_genes import filter_genes_dispersion | |||
There was a problem hiding this comment.
I guess the recipe_weinreb17 was relying on filter_genes_cv_deprecated, it's unfortunately not tested, but can you check and maybe add a small test? I fear this would break a lot of people's code that used the weinreb recipe.
There was a problem hiding this comment.
Oh, actually about this recipe, i forgot to import filter_genes_cv_deprecated from ._deprecated.highly_variable_genes. Fixed it.
For the recipes i just changed the point of import, no other things were changed.
There was a problem hiding this comment.
yes, I was referring to that omission :)
| @@ -0,0 +1,34 @@ | |||
| library(reticulate) | |||
There was a problem hiding this comment.
Ah! You're now requiring Seurat in the tests??? That's something that we don't want. No R scripts in the Scanpy tests... The previous version of filter_genes_dispersion was benchmarked and the tests should be produced with it; the new function should test against that output.
Seurat might definitely change their function in the future, and we don't want to get in trouble with that. We just want to be consistent within Scanpy.
There was a problem hiding this comment.
OK, got it; you just used seurat to produce the test data.
There was a problem hiding this comment.
That's of course fine; it's a bit redundant; but a nice-to-have. 😄
There was a problem hiding this comment.
The script can be removed of course, but i thought it would be nice to have in case the dataset changes, for example.
|
|
||
| FILE = Path(__file__).parent / Path('_scripts/seurat_hvg.csv') | ||
|
|
||
| def test_higly_variable_genes_compare_to_seurat(): |
There was a problem hiding this comment.
Could you add the same test for the deprecated filter_genes_dispersion? We want to be sure that nobody touches it again in the future...
Thank you!
|
The whole PR is pretty awesome already! I wrote some comments... Can you also add the function to the docs, cross reference the deprecated and the new function in the Notes section and add it to the release notes? |
|
Yes, i will. |
|
Thank you! |
|
I'm merging this now, as we're slowly running in danger of getting conflicts with other people working on this. Please move forward with the remaining issues in a new PR. Thank you! |
No description provided.