bbknn and leiden functions

[ktpolanski]

• created bbknn.py and leiden.py
• added Park18 and Traag18 references
• added bbknn import to api/pp.py and leiden import to api/tl.py

[ktpolanski]

Think the Travis thing failing is because of the new dependencies

[flying-sheep]

Sure is E ImportError: No module named 'leidenalg'

You should add a new extra in setup.py:

https://github.com/theislab/scanpy/blob/04056e4798873ecec71adcc51804825245a55295/setup.py#L26

then install it in .travis.yml (pip install -e .[louvain,leiden,test] or so) and add a test.

[flying-sheep]

The tests don’t fail, but you should still add the extra to setup.py

[ktpolanski]

The tests don’t fail, but you should still add the extra to setup.py

Is the fact that you don't list 'docs' in your pip thing (pip install -e .[louvain,leiden,test]) purposeful?

[flying-sheep]

Yes. We don’t build the docs, we just check the readme. so we only need docutils and not sphinx and so on.

[ktpolanski]

Ok, I've added the thing to setup.py, and did the pip command with all non-docs extras, but apparently the travis file didn't change.

[flying-sheep]

Currently there are no tests, so those packages aren't actually needed. Looks good to me!

[ktpolanski]

...so, what's the procedure with this? Do I need to do anything else?

[flying-sheep]

Almost done! Sorry for the delay

[flying-sheep]

This can be simplified I think. The whole file could just be:

try:
    from bbknn import bbknn
except ImportError:
    def bbknn(*args, **kwargs):
        raise ImportError('Please install BBKNN: `pip3 install bbknn`')

Except if we want control over the docstring? But as long as the upstream docstring is good, why bother?

Also the file is tab-indented while the rest of scanpy is 4-space-indented. Please adapt it.

[flying-sheep]

Please use """

[flying-sheep]

can you please add typing here instead of putting it in the docstring? we’re moving to typehints:

def leiden(
    adata: AnnData,
    key_added: str = 'leiden',
    ...
    adjacency: Optional[sparse.spmatrix] = None,
    ...

And in the docs just:

Parameters
----------
adata
    The annotated data matrix.
key_added
    ``adata.obs`` key under which to add the cluster labels.
...

[flying-sheep]

Please use PEP8 style # comment, not #comment

[ktpolanski]

I tried to do the things. I have no idea how to type the arguments, so there may be some trainwrecks there. Funny how it wasn't like that in the louvain function I based this on.

[ktpolanski]

So apparently this now fails on the typing, on a line you provided as an example. Great. Can I please revert to how it was?

[flying-sheep]

Hi! Sorry for frustrating you :( if you want I can fix and merge it manually. You're doing great work!

You simply need to import the things you're using in the annotations, then it'll work!

[ktpolanski]

This will introduce a new problem, won't it? I'm using some sort of thingy from leiden

[ktpolanski]

Ok, I'm stumped. Help please.

[flying-sheep]

just leave that one parameter’s annotation out for now, and I’ll pull it 😉

[ktpolanski]

Ok, did that. Sorry about the trouble, that was my first time trying to type stuff and I was a bit in the dark on this. Thanks for your patience.

[flying-sheep]

done. thank you!

[flying-sheep]

I had to fix a few issues, if you want you can check it out: 66e64b4

• I unified the parameter order with louvain

• I actually import it in sc.tl

• I added it to the docs here: https://scanpy.readthedocs.io/en/latest/api/#clustering-and-trajectory-inference

• I added a test

• I fixed the references (you had typos there: 2018 instead of 18 and a missing “L”)

• You did this:

  partition_kwargs['weights'] = None
  if use_weights:
      weights = np.array(g.es['weight']).astype(np.float64)
  # “weights” is never used then

  But I assume you meant this. Am I correct?

  if use_weights:
      partition_kwargs['weights'] = np.array(g.es['weight']).astype(np.float64)

[ktpolanski]

Good catch, sorry! I added the whole partition_kwargs dictionary late into the process and forgot to change that syntax.

In that case, could you also "hook up" pp.bbknn?

[flying-sheep]

Good call, will do!

[falexwolf]

Thank you, @flying-sheep for fielding all the questions! Thank you for the PR, @ktpolanski! Is there anything still unclear where I can be of help?

[flying-sheep]

Well, the bbknn docs look like this: https://scanpy.readthedocs.io/en/latest/api/index.html#batch-effect-correction

I went that way since I didn’t want to make it look like we coded it (with the docs hosted on our page and so on). Do you think that’s a good solution or would you like it to be done differently?

[ktpolanski]

I mirrored the scanpy docstring style to the best of my ability, but for some reason it turned out a little bit different visually than the ones you have. Hope that's not too much of a problem.

[flying-sheep]

Surely not! And since we’re linking it anyway, it doesn’t need to match ours.

I like that you’re using ``code`` instead of `code`, that looks much better than the way many many docstrings in scanpy do.

I suggest that you add some lines to your intersphinx mapping and use more links instead of code though, that makes the docs easier to use, since people can just click on things:

https://github.com/theislab/scanpy/blob/7b97d0d734970527230cf7f25ab15df874a143b3/docs/conf.py#L60-L69

I also added this hack to make links to AnnData and csr_matrix work:

https://github.com/theislab/scanpy/blob/7b97d0d734970527230cf7f25ab15df874a143b3/docs/conf.py#L236-L253

[falexwolf]

try:
    from bbknn import bbknn
except ImportError:
    def bbknn(*args, **kwargs):
        raise ImportError('Please install BBKNN: `pip3 install bbknn`')

I went that way since I didn’t want to make it look like we coded it (with the docs hosted on our page and so on). Do you think that’s a good solution or would you like it to be done differently?

This is great!

https://scanpy.readthedocs.io/en/latest/api/scanpy.api.pp.bbknn.html#scanpy.api.pp.bbknn

But I actually don't see any docs there, I don't know why it doesn't find the original docstring... We'd like to have the reference to @ktpolanski preprint in the docstring in the first line together with a short summary of what it does and how it does it, just as for any other function. As this directly uses the implementation from @ktpolanski, we'd also want an explicit statement about that.

pp.bbknn is just an alias for bbknn.bbknn(). Refer to it for the documentation.

... can be removed. That should be evident...

In the case of the tl.leiden wrapper, I'd also add an explicit statement that this wraps the leiden package of Traag (2018).

Otherwise, all of this is great!

[flying-sheep]

But I actually don't see any docs there, I don't know why it doesn't find the original docstring...

That’s because on readthedocs, bbknn isn’t installed, so it uses the dummy version.

We'd like to have the reference to @ktpolanski preprint in the docstring in the first line together with a short summary of what it does and how it does it, just as for any other function.

We could do that either by adding a docstring to the dummy version that links to https://bbknn.rtfd.io or by installing bbknn on rtd, and modifying the docstring programmatically. something like:

try:
    from bbknn import bbknn
    first_para, rest = bbknn.__doc__.split('\n\n', 1)
    bbknn.__doc__ =
        '{}\n\nFor a graphical explanation, visit '
        '`The bbknn project <https://github.com/Teichlab/bbknn>`__-\n\n{}'
        .format(first_para, rest)
except ImportError:
    ...