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

Remove intermediate module #1

Closed
wants to merge 3 commits into from
Closed

Conversation

flying-sheep
Copy link
Member

@flying-sheep flying-sheep commented Nov 21, 2017

I hope the way I did it has two effects:

  1. File history is preserved (by deleting __init__.py before renaming anndata.py). Note that I mean blame (which works), not history (which is rather useless)

  2. Autocompletion still works well (by adding __all__). We should keep in mind Rethink completer limit to __all__ ipython/ipython#10649 here: Some people prefer seeing everything while others don’t.

    It’s configurable via c.IPCompleter.limit_to__all__ = True

@falexwolf
Copy link
Member

The thing with the history does not seem to work. Click on "history"

https://github.com/theislab/anndata/commits/6d5cfd6cbb2e929fac0100547ebb6dab9bf0001a/anndata/__init__.py

and you'll see that the changes to the structure of AnnData are not there.

Before merging: are we sure that AnnData and the helper functions around it will never become so complicated that we have to setup a more general file structure? If yes, we should maybe consider renaming to base.py instead of to __init__.py, which is the init of the whole package. I think it's nice if a package only makes those parts of the subpackges available to the user that the user is actually likely to need. In the case of the anndata package, this is currently only the AnnData class.

Still, what I have in mind is to move all the basic read-write, the basic plotting, and the basic mathematical operations from Scanpy to AnnData, as already mentioned in the README.

anndata would then have submodules readwrite, plotting and maths (or preprocessing). All of this is independent of single-cell stuff and simply practical, simple operations that you would like to have with data container. all of this also not a single (!) additional dependency and hence anndata would remain a small package that can be quickly installed to read anndata files, plot them and perform simple mathematical operations.

In the light of these plans, I would prefer to keep AnnData either in anndata or in base or something similar and use __init__ solely for the purpose of providing the user some toplevel functions.

Btw: one could also think of adding all these simple functions as attributes to AnnData, but this could be discussed.

@flying-sheep
Copy link
Member Author

flying-sheep commented Nov 22, 2017

The thing with the history does not seem to work.

uh, i think you skipped the third sentence i wrote.

If yes, we should maybe consider renaming to base.py instead of to init.py, which is the init of the whole package.

maybe we should think about if we can split it up right now. all the helper stuff in a module, BoundRecArray in another, and only leave some imports and class AnnData in __init__.py.

we can also simplify the code in AnnData, so __init__.py is smaller. either by putting more code into helpers, or by using a base class that contains all the implementation details, and leaves AnnData as just a skeleton with all the public methods.

e.g. we could put concatenate as a function:

def concatenate(*adatas, batch_key='batch', batch_categories=None):
    if len(adatas) == 0: raise ArgumentError('You need to pass at least one AnnData object to `concatenate`')
    if len(adatas) == 1: return adatas[0]
    ...

class AnnData:
    ...
    concatenate = concatenate

# adata1.concatenate(adata2)
# concatenate(adata1, adata2)

@falexwolf
Copy link
Member

Let's do as you proposed above! I'll close this stub pull request for now.

@falexwolf falexwolf closed this Nov 28, 2017
@falexwolf falexwolf deleted the remove-intermediate branch August 4, 2018 07:34
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