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

Allow del adata.X, adata.X = None #463

Merged
merged 15 commits into from
Dec 1, 2020
Merged

Allow del adata.X, adata.X = None #463

merged 15 commits into from
Dec 1, 2020

Conversation

rcannood
Copy link
Contributor

@rcannood rcannood commented Nov 23, 2020

Feature proposal for #464.

Need to test:

  • Getter (test_base.py > test_x_is_none())
  • Setter (test_base.py > test_x_is_none())
  • Deleter (test_base.py > test_x_is_none())
  • Initializer (test_base.py > test_x_is_none())
  • Views
    • Check that .X = None doesn't change other view behavior
    • a[idx].X doesn't do anything weird
    • copying views still works
  • _subset_inplace
  • Copying
  • Backed mode
  • Concatenation
  • Transpose
  • Methods which default to values from X, e.g. obs_vector, to_df.
  • IO (seems to work, needs testing)
    • Maybe specialized writers like to_csvs will need some attention here?
  • Raw (pretty similar set of things to be checked)
    • Initializer
    • Backed
    • Copies
    • Concatenation
    • IO

@ivirshup
Copy link
Member

I don't think we currently allow X to be None (#331).

I have come across a case recently where this would be useful, though I think it would take some doing to make the entire codebase work again if we allowed this. X being an array with shape is pretty baked into the data model at this point.

Maybe this could be opened as a feature request issue, and you could pitch your use case?

@rcannood
Copy link
Contributor Author

Thanks for your input.

X being None is already part of the unit tests: test_base.py#L46-L47. Also, if X would not be allowed to be None, why is there a check in the code I changed? ;)

A possible use case I have is when working with read-only h5ad files: if your input is immutable, it may be interesting to exclude X from the output file to reduce the file size.

@ivirshup
Copy link
Member

At one point it was allowed, but I can say for certain that a lot of code has been written since then that does not expect X to be None.

For example:

a = ad.AnnData(X=None, shape=(100, 200))

a.copy()
Traceback
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
<ipython-input-12-02bb7baf10a6> in <module>
----> 1 a.copy()

~/github/anndata/anndata/_core/anndata.py in copy(self, filename)
   1428                 X = _subset(self._adata_ref.X, (self._oidx, self._vidx)).copy()
   1429             else:
-> 1430                 X = self.X.copy()
   1431             # TODO: Figure out what case this is:
   1432             if X is not None:

AttributeError: 'NoneType' object has no attribute 'copy'

At one point I think I saw _X being None as an indicator for if the object was in backed mode.

If this is something we were to support more, it would need a fair amount of testing.

@rcannood rcannood changed the title ad.X = None not reachable Feature request: ad.X = None Nov 24, 2020
@rcannood
Copy link
Contributor Author

Alright :) I hereby turn this PR into a feature request. I'd like X to be optional. I mean, it already is, but apparently more thorough testing is needed to verify that it works properly.

What is the opinion of the major anndata developers? 👍 or 👎?

If we agree X should be able to be None, we can use this PR to add tests and fix the code where necessary.

@ivirshup
Copy link
Member

Oops, I linked the wrong issue before. #330 is where this has come up before. Also I'm going to open this discussion as a new issue so it's more visible.

@rcannood rcannood marked this pull request as draft November 26, 2020 07:46
@rcannood rcannood marked this pull request as ready for review November 27, 2020 08:06
@rcannood
Copy link
Contributor Author

Plus it's much easier to review and merge small PRs. Let's keep that PR's scope to just fixing up the .X property.

@ivirshup Alright, shall we merge this PR in order to resolve the first box in #467?

anndata/_core/anndata.py Outdated Show resolved Hide resolved
anndata/_core/anndata.py Outdated Show resolved Hide resolved
@ivirshup ivirshup changed the title Feature request: ad.X = None Allow del adata.X, adata.X = None Dec 1, 2020
@ivirshup
Copy link
Member

ivirshup commented Dec 1, 2020

Great, thanks for implementing this and making the suggestion!

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