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

fix sc.tl.umap for umap-0.5 #1589

Merged
merged 3 commits into from
Jan 19, 2021
Merged

fix sc.tl.umap for umap-0.5 #1589

merged 3 commits into from
Jan 19, 2021

Conversation

ivirshup
Copy link
Member

@Koncopd, this seems to work as a bare minimum for making sc.tl.umap work for umap-0.5. ingest still does not work, and that definitely looks complicated to fix.

What would you say to adding a line like

if umap.__version__ >= 0.5:
    raise ImportError("Ingest currently require umap-learn < 0.5")

We could then remove the pin, and free up user's environments.

@Koncopd
Copy link
Member

Koncopd commented Jan 15, 2021

I am investigating what could be done now to fix ingest. I really don't like requiring a different and older version of umap for ingest.

@ivirshup
Copy link
Member Author

Ideally yes, but it would be nice to minimize the loss of functionality in one environment.

I think a short term, sorta hacky, solution could be mimicking NNDescent.__getstate__/ NNDescent.__setstate__. Longer term I think we could try to get something in pynndescent like initialization from a prebuilt graph.

@Koncopd
Copy link
Member

Koncopd commented Jan 15, 2021

I'll try to fix this over the weekend.

@Koncopd
Copy link
Member

Koncopd commented Jan 18, 2021

@ivirshup i see you opened the issue about graph initialization, though i haven't checked yet the things that were added. Do we wait for a new release of pynndescent and use the new features or we try to fix ingest with the current version of pynndescent (i have some code already that partially works)?

upd: ah, i see init_graph should work without hacks now.

@ivirshup
Copy link
Member Author

Since we don't know when a release of pynndescent will go out, I think it's fine to keep this a little hacky for now. I think it can be less hacky than now doing something like this:

from_init = pynndescent.NNDescent(train, n_neighbors=15, init_graph=indices)
from_init._rp_forest = rp_forest
query_indices_init, query_distances_init = from_scratch.query(test)

Once a release of pynndescent comes out we can support doing it the proper way.

I'd say it's up to you whether you want to have the kinda hacky solution or not. I definitely don't want UMAP to be pinned to below 0.5 when we release 1.7 proper, and it would be good for ingest to work with UMAP 0.5.

The only downside I see to the kinda hacky solution as an intermediate is that you're fixing it twice. I don't think it'll be hard to go from this to the clean version however.


I haven't looked into what needs to happen for the UMAP embedding transfer stuff to work. Is that pretty straight forward?

@ivirshup
Copy link
Member Author

Are you happy with me to merge this as is, then you can open a PR for ingest support?

@Koncopd
Copy link
Member

Koncopd commented Jan 19, 2021

Yes, i am fine with it.

@ivirshup ivirshup merged commit 47d4f0d into scverse:master Jan 19, 2021
@ivirshup
Copy link
Member Author

@meeseeksdev backport 1.7.x

meeseeksmachine pushed a commit to meeseeksmachine/scanpy that referenced this pull request Jan 19, 2021
ivirshup added a commit that referenced this pull request Jan 19, 2021
@ivirshup ivirshup mentioned this pull request Feb 2, 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.

2 participants