Support Dask in highly_variable_genes

[flying-sheep]

• Depends on Test full Dask support for log1p, normalize_per_cell, filter_cells/filter_genes #2814

• Closes Dask support for pp.highly_variable_genes (default "seurat" flavor) #2777, closes Dask support for pp.highly_variable_genes ("cellranger" flavor) #2807
• Tests included or not required because:

• Release notes not necessary because:

TODO:

• batched

flavor=	"seurat"	"cell_ranger"
n_top_genes=n	‌	(SeriesGroupBy.agg does not accept Aggregate when paired with 'median' dask/dask#10853)
{min,max}_{disp,mean}	‌	‌

[codecov]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e7a1251) 74.07% compared to head (54388f8) 74.11%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2809      +/-   ##
==========================================
+ Coverage   74.07%   74.11%   +0.04%     
==========================================
  Files         115      115              
  Lines       12652    12685      +33     
==========================================
+ Hits         9372     9402      +30     
- Misses       3280     3283       +3     

Files	Coverage Δ
scanpy/_compat.py	50.00% <100.00%> (ø)
...preprocessing/_deprecated/highly_variable_genes.py	95.40% <100.00%> (+0.05%)	⬆️
scanpy/preprocessing/_distributed.py	82.60% <ø> (ø)
scanpy/preprocessing/_highly_variable_genes.py	95.58% <98.47%> (-0.28%)	⬇️

... and 1 file with indirect coverage changes

[flying-sheep]

this has been broken the whole time by operating on X instead of the selected layer?

[ivirshup]

I bet we didn't notice since the nonzero values are likely pretty similar, if not the same, for common use cases

[flying-sheep]

Hmm, I found several weirdnesses in hvg:

1. as mentioned above, this line has always ignored the layer parameter (fixed in this PR):

   scanpy/scanpy/preprocessing/_highly_variable_genes.py

   Line 496 in 86b85ee

   filt = filter_genes(adata_subset, min_cells=1, inplace=False)[0]

2. How does the binning in flavour='cell_ranger' work? (see n_bins not respected in highly_variable_genes(..., flavour='cell_ranger') #888, where I asked @Koncopd and @falexwolf how to address that)
3. This line has survived since the very first commit, but what does it do? it sorts the view in-place?

   scanpy/scanpy/preprocessing/_highly_variable_genes.py

   Lines 276 to 278 in 86b85ee

   	dispersion_norm[
   	::-1
   	].sort() # interestingly, np.argpartition is slightly slower

[ivirshup]

Apart from the comments, can we get a regression test for "cell_ranger" (e.g. generate results with an older version)? I don't think we have one in the test suite.

Sure! That’s a concrete thing I can do. I’ll do that on thursday, I did the rest of what you asked today

I don't think this has happened yet, could we add this?

---

There are two more lines which aren't covered, but I believe they should be unreachable (both just ValueError that the arg should be "cell_ranger" or "seurat") so it's fine.

---

I'm a little concerned about changing the return for inplace=False, in case anyone was relying on that. Not too concerned, but what do you think about tracking that for 2.0?

[ivirshup]

Sorry for the accidental push, trying to see if we can get inline code coverage on github:

• Test codecov code annotation #2847

[flying-sheep]

can we get a regression test for "cell_ranger" (e.g. generate results with an older version)? I don't think we have one in the test suite.

I’ll do that today if you can give me more info. Usually “regression test” refers to testing specific properties that were broken in a bug and subsequently fixed. What properties exactly are you looking for? Why cell_ranger and not seurat? Are you implying that we do that already for seurat?

/edit: done in #2851

There are two more lines which aren't covered, but I believe they should be unreachable (both just ValueError that the arg should be "cell_ranger" or "seurat") so it's fine.

yeah, lines like that are more defensive coding. I add them even to internal code to force us to look at everything instead of having a else: # flavor == "cell_ranger" branch or so

I'm a little concerned about changing the return for inplace=False, in case anyone was relying on that.

You mean the fact that the index makes the dataframe now actually useful? I can’t think of a way in which this breaks things in a way that isn’t immediately obvious and welcome. Of course, code can be infinitely weird, but can you think of a scenario?

[ivirshup]

You mean the fact that the index makes the dataframe now actually useful? I can’t think of a way in which this breaks things in a way that isn’t immediately obvious and welcome. Of course, code can be infinitely weird, but can you think of a scenario?

I get that it is more useful, but any code that was accessing it with .loc especially if it was relying on unique indices could run into a problem. It's minor, I'd be fine to leave it. Just we may need to revert.

[flying-sheep]

We have documented the columns returned by the data frame, not the index, so I’d say the documented behavior stayed the same