Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle non-csr/csc sparse matrices in anndata.write #847

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

scottgigante-immunai
Copy link

Attempting to save an AnnData object with non-CSR/CSC sparse matrices throws an error:

File "/usr/src/singlecellopenproblems/openproblems/api/utils.py", line 58, in write_h5ad
      adata.write_h5ad(filename)
    File "/usr/local/lib/python3.8/site-packages/anndata/_core/anndata.py", line 1918, in write_h5ad
      _write_h5ad(
    File "/usr/local/lib/python3.8/site-packages/anndata/_io/h5ad.py", line 85, in write_h5ad
      write_elem(f, "X", adata.X, dataset_kwargs=dataset_kwargs)
    File "/usr/local/lib/python3.8/site-packages/anndata/_io/utils.py", line 220, in func_wrapper
      raise type(e)(
  TypeError: No method has been defined for writing <class 'scipy.sparse._coo.coo_matrix'> elements to <class 'h5py._hl.group.Group'>
  
  Above error raised while writing key 'X' of <class 'h5py._hl.group.Group'> to /

This PR fixes that by registering an appropriate method which converts all sparse matrices without an explicitly defined conversion to CSR.

Attempting to save an AnnData object with non-CSR/CSC sparse matrices throws an error:

```
File "/usr/src/singlecellopenproblems/openproblems/api/utils.py", line 58, in write_h5ad
      adata.write_h5ad(filename)
    File "/usr/local/lib/python3.8/site-packages/anndata/_core/anndata.py", line 1918, in write_h5ad
      _write_h5ad(
    File "/usr/local/lib/python3.8/site-packages/anndata/_io/h5ad.py", line 85, in write_h5ad
      write_elem(f, "X", adata.X, dataset_kwargs=dataset_kwargs)
    File "/usr/local/lib/python3.8/site-packages/anndata/_io/utils.py", line 220, in func_wrapper
      raise type(e)(
  TypeError: No method has been defined for writing <class 'scipy.sparse._coo.coo_matrix'> elements to <class 'h5py._hl.group.Group'>
  
  Above error raised while writing key 'X' of <class 'h5py._hl.group.Group'> to /
```

This PR fixes that by registering an appropriate method which converts all sparse matrices without an explicitly defined conversion to CSR.
@codecov
Copy link

codecov bot commented Nov 8, 2022

Codecov Report

Merging #847 (c446a23) into master (bc9f886) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
+ Coverage   83.74%   83.79%   +0.04%     
==========================================
  Files          32       32              
  Lines        4399     4412      +13     
==========================================
+ Hits         3684     3697      +13     
  Misses        715      715              
Impacted Files Coverage Δ
anndata/_io/specs/methods.py 84.98% <100.00%> (+0.51%) ⬆️

@ivirshup
Copy link
Member

Hey. I think I'd be more inclined to say "that anndata is invalid and should have errored when you created it". I think many anndata operations would error with any of the sparse matrix types that don't do indexing right.

I would rather add broad support for them as they gain functionality, rather than later changing conversion behavior.

An alternative would be to do the conversion when the AnnData is constructed, but I think it's largely fine to leave this to the user.

@scottgigante-immunai
Copy link
Author

I'd be fine with either throwing an error or automatically converting inside AnnData.X.setter -- just doing it in construction may not work as it's not that uncommon to set adata.X = func(adata.X).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants