Scanpy: sc.pp.neighbors throws KeyError: diffmap_evals due to obsm concatenation

[gokceneraslan]

Duplicating from https://github.com/theislab/anndata/pull/284:

@Koncopd @falexwolf

There is an issue with the obsm concatenation. When we run sc.tl.diffmap with different anndata objects, concatenate them and run sc.pp.neighbors on the concatenated new anndata, we get the following exception. The reason is that X_diffmap' is available in obsm but .uns['diffmap_evals'] is not.

---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<timed exec> in <module>

/opt/conda/lib/python3.7/site-packages/scanpy/neighbors/__init__.py in neighbors(adata, n_neighbors, n_pcs, use_rep, knn, random_state, method, metric, metric_kwds, copy)
    104     if adata.isview:  # we shouldn't need this here...
    105         adata._init_as_actual(adata.copy())
--> 106     neighbors = Neighbors(adata)
    107     neighbors.compute_neighbors(
    108         n_neighbors=n_neighbors, knn=knn, n_pcs=n_pcs, use_rep=use_rep,

/opt/conda/lib/python3.7/site-packages/scanpy/neighbors/__init__.py in __init__(self, adata, n_dcs)
    527                 self._number_connected_components = self._connected_components[0]
    528         if 'X_diffmap' in adata.obsm_keys():
--> 529             self._eigen_values = _backwards_compat_get_full_eval(adata)
    530             self._eigen_basis = _backwards_compat_get_full_X_diffmap(adata)
    531             if n_dcs is not None:

/opt/conda/lib/python3.7/site-packages/scanpy/neighbors/__init__.py in _backwards_compat_get_full_eval(adata)
    395         return np.r_[1, adata.uns['diffmap_evals']]
    396     else:
--> 397         return adata.uns['diffmap_evals']
    398 
    399 

KeyError: 'diffmap_evals'

Doesn't it make more sense to make obsm concatenation False by default, by the way? Should concatenating obsm be the default behaviour?

[ivirshup]

I think concatenating obsm makes sense for default behavior. But we should probably make it easy to skip elements when concatenating. We should probably also have options for reconciling entries in uns.

Would sc.pp.neighbors do anything different if diffmap had been run before? It doesn't look to me like it would. If this is the case, the bug here is that neighbors is checking expectations unrelated to what it's doing.

[gokceneraslan]

I think concatenating obsm makes sense for default behavior.

I'm not entirely sure. Less experienced users might concatenate things and plot a UMAP without running sc.tl.umap on the new concatenated object and see some super weird things. We should never assume that ALL users know how exactly sc.tl.umap, adata.obsm and AnnData.concatenate work. The decisions about the defaults are always more than what we want to see in scanpy for our own convenience.

Would sc.pp.neighbors do anything different if diffmap had been run before? It doesn't look to me like it would. If this is the case, the bug here is that neighbors is checking expectations unrelated to what it's doing.

Concatenating obsm without touching uns puts the object in an unstable state somehow from diffmap point of view, since it has obsm X_diffmap but not uns diff_evals :) I don't know why "neighbors" needs diffmap_evals...

[gokceneraslan]

I'm not entirely sure. Less experienced users might concatenate things and plot a UMAP without running sc.tl.umap on the new concatenated object and see some super weird things.

It'd be cool to print a warning in such cases somehow, that concatenated obsms are not compatible or so.

[LuckyMD]

I would tend to agree with @gokceneraslan on this. Concatenating obsm seems like a risky thing to do.

[Koncopd]

I'm not entirely sure. Less experienced users might concatenate things and plot a UMAP without running sc.tl.umap on the new concatenated object and see some super weird things.

I agree that this can be a problem.

[ivirshup]

Concatenating obsm without touching uns puts the object in an unstable state somehow from diffmap point of view

Sure, but this should only ever effect diffmap.

Arguably it also puts the object in an unstable state from a PCA point of view since there's no promise that observation loadings correspond to the variable loadings. I don't think users should have the expectation that meaning is preserved by concatenation, but I'm not sure if this is something people would believe.

I'm not entirely sure. Less experienced users might concatenate things and plot a UMAP without running sc.tl.umap on the new concatenated object and see some super weird things

Have users reported that this is confusing?

It'd be cool to print a warning in such cases somehow, that concatenated obsms are not compatible or so.

I think a note in the docstring for concatenation should be sufficient. My expectation is that it's much more common for our users to be familiar with what similar methods (like np.concatenate and pd.concat) do, and to have the right expectations about this.

Bioconductor's SummarizedExperiment classes also do not warn about this, and concatenate along their reducedDims.

[LuckyMD]

Have users reported that this is confusing?

I think concatenating .obsm by default was a recent change, no? I vaguely recall reading about it in an issue recently.

[Koncopd]

Yes, it is a very recent change.

[dangeles]

Hi all! I've been using scanpy and ran into a similar problem. I was wondering if there's an easy / appropriate work-around, other than simply deleting obsm[X_diffmap]?

Thank you all for developing scanpy, it's a really wonderful piece of software.