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

update bbknn arguments and docstring #1868

Merged
merged 3 commits into from
Jun 18, 2021
Merged

Conversation

ktpolanski
Copy link
Contributor

Updated sc.external.pp.bbknn() to match current arguments and docstring.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #1868 (7526663) into master (4dd8de9) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #1868   +/-   ##
=======================================
  Coverage   71.22%   71.22%           
=======================================
  Files          92       92           
  Lines       11181    11181           
=======================================
  Hits         7964     7964           
  Misses       3217     3217           
Impacted Files Coverage Δ
scanpy/external/pp/_bbknn.py 58.33% <100.00%> (ø)

@ktpolanski
Copy link
Contributor Author

It seems this is of some relevance to users, as shown by it showing up twice independently over the course of the past week.

For some reason, my tweaks have killed off ReadTheDocs, and I can't check why as upon pressing the "details" button I get the 404 equivalent :) I am now importing types so I can correctly define metric as also including types.FunctionType. This is probably what is causing whatever hiccup is happening.

@giovp , some assistance with getting the ball rolling on this? The changes are a result of me expanding BBKNN with pynndescent on your recommendation.

@pinin4fjords
Copy link
Contributor

@ktpolanski following from https://scanpy.readthedocs.io/en/stable/dev/code.html#code-style and building the docs locally gives me (among some errors that were not relevant):

...
/path/to/scanpy/scanpy/external/pp/_bbknn.py:docstring of scanpy.external.pp._bbknn.bbknn:28: WARNING: py:class reference target not found: function
/path/to/scanpy/scanpy/external/pp/_bbknn.py:docstring of scanpy.external.pp._bbknn.bbknn:28: WARNING: py:class reference target not found: function
...

Maybe that gives you some pointers?

@ktpolanski
Copy link
Contributor Author

ktpolanski commented Jun 15, 2021

I ran pip3 install scanpy[doc], as instructed by that page, and the doc seems to have built fine locally.

image

The only fishy thing is the function is not clickable, while the other two are, but it built.

Also, line 28 is trim: Optional[int] = None,, which was there previously already. It's the metric change that's causing this - your argument renaming PR did not hiccup.

@pinin4fjords
Copy link
Contributor

Hmm, I'm stumped. @ivirshup ?

Copy link
Member

@ivirshup ivirshup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only fishy thing is the function is not clickable, while the other two are, but it built

I think this is it. The error I see is:

sphinx.errors.SphinxWarning: /home/docs/checkouts/readthedocs.org/user_builds/icb-scanpy/checkouts/1868/scanpy/external/pp/_bbknn.py:docstring of scanpy.external.pp._bbknn.bbknn:28:py:class reference target not found: function

Warning, treated as error:
/home/docs/checkouts/readthedocs.org/user_builds/icb-scanpy/checkouts/1868/scanpy/external/pp/_bbknn.py:docstring of scanpy.external.pp._bbknn.bbknn:28:py:class reference target not found: function

I don't think the 'types.FunctionType' needs to be quoted, but we've used collections.abc.Callable in other places without issue.

@@ -14,13 +15,20 @@
def bbknn(
adata: AnnData,
batch_key: str = 'batch',
use_rep: str = 'X_pca',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could the representation be chosen with keywords like obsm, layers? You can use the helper functions sc.get._get_obs_rep and sc .get._set_obs_rep.

@ktpolanski
Copy link
Contributor Author

I can't get it to work. I have unquoted types.FunctionType, and function remains unclickable. I have tried to switch to an collections.abc.Callable, and instead of anything helping the entire argument support vanishes:

image

Please help?

@ivirshup
Copy link
Member

I think that should do it. I believe the types module isn't actually meant to be used for type annotations, that's what the typing module is for (which is always confusing to me).

and instead of anything helping the entire argument support vanishes:

I got this behaviour too, but running make clean and building from scratch got things working again. ¯_(ツ)_/¯

@ktpolanski
Copy link
Contributor Author

Thanks a lot for the help, assuming things are in order then that's it for me.

@ivirshup
Copy link
Member

Thanks for the update!

@ivirshup ivirshup merged commit 598842f into scverse:master Jun 18, 2021
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.

3 participants