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

Node2Vec Enhancements #566

Open
daxpryce opened this issue Oct 27, 2020 · 2 comments
Open

Node2Vec Enhancements #566

daxpryce opened this issue Oct 27, 2020 · 2 comments
Labels
enhancement New feature or request
Projects

Comments

@daxpryce
Copy link
Contributor

daxpryce commented Oct 27, 2020

Is your feature request related to a problem? Please describe.

n2v currently behaves very differently from the rest of the embed module

Describe the solution you'd like

While it is true that discrete fit and transform methods don't really make much sense for node2vec, it can at least implement fit_transform and warn or throw an error if fit or transform are called separately.

It also requires a networkx Graph, whereas we should be able to handle adjacency matrices in numpy and scipy sparse csr formats as well.

Lastly, none of the other embed "functions" (fit_transform results) return the labels array. It is going to be a requirement that our labels and our graphs match up at all times in all scenarios, up to and including nodes without any edges - that way we can rely on the natural ordering of graph to provide this label lookup vs. an explicitly returned tuple. The alternative makes for an even worse API from the adjacency matrix perspective, so this route is being chosen instead.

Additional context

The rest of the embed module is going to have corresponding functions that encapsulate the create AdjacencySpectralEmbed object with appropriate parameters, then call fit_transform on this graph steps into a single function, which will be more similar to the current node2vec_embed function we have.

@daxpryce daxpryce added the enhancement New feature or request label Oct 27, 2020
@daxpryce daxpryce added this to Needs Triage in Graspologic via automation Oct 27, 2020
@dokato
Copy link
Contributor

dokato commented Jun 9, 2022

hey @daxpryce, is this up for grabs? I've been working on various embedding methods recetnly, i.e. comparing ASE vs node2vec, so the difference in syntax is pretty annoying, happy to help with PR if needed.

@daxpryce
Copy link
Contributor Author

I think you absolutely can take this; there's some work coming down the pipeline that'll be impacted by whatever you do, but I don't know how far along it is or when we might expect it and I don't want to slow you down on this front. I definitely understand the ergonomics are jarring.

If you could move the current n2v function signature into the graspologic.pipeline.embed module and then work on a sklearn style impl in graspologic.embed, that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Graspologic
  
Needs Triage
Development

No branches or pull requests

2 participants