Skip to content

Conversation

@jarbus
Copy link
Contributor

@jarbus jarbus commented Nov 8, 2021

Still very much a work in progress, right now the example doesn't run because GraphSignals, Graphs GeometricFlux, have name collisions, and I'm also getting the error LoadError: UndefVarError: neighbors not defined even though we import Graphs in src/GeometricFlux.jl. I'm new to Julia package development so not entirely sure what's going on here. However, I tested the code by including all functionality in a script and all the functions work as intended.

@jarbus
Copy link
Contributor Author

jarbus commented Nov 10, 2021

@yuehhua

@yuehhua
Copy link
Member

yuehhua commented Nov 10, 2021

As for the neighbors, you can use Graphs.neighbors instead.

@jarbus
Copy link
Contributor Author

jarbus commented Nov 11, 2021

As for the neighbors, you can use Graphs.neighbors instead.

That would make sense, but even when perform such a replacement, I get

WARNING: both GeometricFlux and Graphs export "neighbors"; uses of it in module Main must be qualified

ERROR: UndefVarError: neighbors not defined

Any ideas @yuehhua?

@yuehhua
Copy link
Member

yuehhua commented Nov 11, 2021

You should call Graphs.neighbors instead of neighbors.

@jarbus
Copy link
Contributor Author

jarbus commented Nov 11, 2021

You should call Graphs.neighbors instead of neighbors.

I am! That's the thing. It's still having namespace issues even when I call Graphs.neighbors

https://github.com/jarbus/GeometricFlux.jl/blob/node2vec/src/graph_embedding/node2vec.jl#L108-L124

@jarbus
Copy link
Contributor Author

jarbus commented Nov 12, 2021

@yuehhua

@yuehhua
Copy link
Member

yuehhua commented Nov 12, 2021

Sorry, I'm busy these days. I checked calling Graphs.neighbors and have GeometricFlux imported in my side. It works well. Maybe I can check what's the difference between us.

@jarbus
Copy link
Contributor Author

jarbus commented Nov 12, 2021

Oh I figured it out! I was using the master branch instead of the node2vec branch somehow when adding the package.

@jarbus
Copy link
Contributor Author

jarbus commented Nov 12, 2021

@yuehhua I'm thinking about switching to using GraphSignals.FeaturedGraph, in order to get this to work seamlessly on Weighted Graphs and to add some consistency with the rest of the package.

Some notes:

  • GeometricFlux is currently incompatible with the newest version of GraphSignals
  • When installing GraphSignals, it was installing 0.2.2 as a dependency of GeometricFlux and v0.3.1 when I was installing with ]add GraphSignals. I had to specify the repo to install it.
  • Not a priority, but it would be nice to add has_edge, inneighbors outneighbors and neighbors to GraphSignals. I'm going to have to implement that functionality myself for conducting random walks over a FeaturedGraph.

@yuehhua
Copy link
Member

yuehhua commented Nov 17, 2021

@jarbus

GeometricFlux is currently incompatible with the newest version of GraphSignals

Yes, I am currently trying to make them compatible.

Not a priority, but it would be nice to add has_edge, inneighbors outneighbors and neighbors to GraphSignals.

So, you want FeaturedGraph to support has_edge, inneighbors, outneighbors and neighbors?

@jarbus
Copy link
Contributor Author

jarbus commented Nov 17, 2021

@yuehhua It's no longer needed, as I figured out a workaround using the FeaturedGraph's adjacency matrix, but for anything regarding graph traversal, functions like outneighbors and the like are really handy and make the code more readable.

@yuehhua
Copy link
Member

yuehhua commented Nov 25, 2021

I have rebased this branch to the new release. Please update to your local repo.

@jarbus
Copy link
Contributor Author

jarbus commented Dec 3, 2021

@yuehhua I'm trying to switch to a FeaturedGraph backend for node2vec, and think that it would be really helpful to include functions like edges and neighbors as part of the GraphSignals API, as it's quite inelegant to iterate over them using the adjacency matrix.

@yuehhua
Copy link
Member

yuehhua commented Dec 4, 2021

Does yuehhua/GraphSignals.jl#76 fit your need?

@jarbus
Copy link
Contributor Author

jarbus commented Dec 4, 2021

Does yuehhua/GraphSignals.jl#76 fit your need?

@yuehhua Unfortunately not. node2vec uses a sampling method called 'AliasSampling, which precomputes transition probabilities to allow for constant-time sampling, and also uses two parameters, pandq`, to control how far the random walk strays from the starting node, so I can't use just any sampling/random walk method. I've reimplemented neighbor and edge functionality but it would be nice to have it built-in to GraphSignals

@yuehhua
Copy link
Member

yuehhua commented Dec 5, 2021

OK, I will put edges and neighbors as part of the GraphSignals API in yuehhua/GraphSignals.jl#77.

@jarbus
Copy link
Contributor Author

jarbus commented Dec 6, 2021

@yuehhua Thanks for all of your help so far :) I have another question, which will hopefully be my last: The node2vec implementation to support both directed and weighted graphs, directed seems simple enough now that I can use neighbors(g, v;dir=:out), but is there an interface for edge weights?

@jarbus
Copy link
Contributor Author

jarbus commented Dec 7, 2021

Actually, I just pushed using the adjacency matrix of the SparseGraph, which looks like graph(fg).S[v,:] |> findnz to return neighbor ids and their weights. I'm able to run random walks on the cora dataset pretty quickly, and I have tests confirming that it works on the karate club network, so I think node2vec is almost ready for merging!

@yuehhua yuehhua marked this pull request as ready for review December 7, 2021 01:32
@yuehhua yuehhua changed the base branch from develop to master December 7, 2021 01:35
@yuehhua
Copy link
Member

yuehhua commented Dec 8, 2021

Please use yuehhua/GraphSignals.jl#78

@jarbus
Copy link
Contributor Author

jarbus commented Dec 9, 2021

We should be good to go. I benchmarked on Cora, takes about 0.7 seconds to conduct 27k walks, of length 80, seems about 15 times faster than the python implementation from the original paper: https://github.com/aditya-grover/node2vec

fix example and deps

reorder using

use bib for doc
@yuehhua yuehhua merged commit 16d43e5 into FluxML:master Dec 12, 2021
@yuehhua
Copy link
Member

yuehhua commented Dec 12, 2021

@jarbus Thank you for contributions. Closes #232

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