-
Notifications
You must be signed in to change notification settings - Fork 57
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
add fast_rp algorithm #1867
add fast_rp algorithm #1867
Conversation
Added Rust implementation; Rust test; Python integration; Python test
}); | ||
} | ||
|
||
// NOTE(Wyatt): the simple fast_rp test is more of a validation of idempotency than correctness (although the results are expected) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as this here - wanna create a fastRP 2.0 ticket to track this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the point of extra testing, perhaps some window tests + layer tests etc - this is probably a good idea for us to add within the larger test pipeline
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also yes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two minor nits, but looks good to merge
let weights = Arc::clone(&weights); | ||
let ss = vv.eval_graph.ss; | ||
// TODO: rewrite using iters? | ||
for neighbour in vv.neighbours() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this algorithm is treating the network as undirected, would a directed version make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The base implementation is only defined for undirected/bidirectional graphs, so this is correct as is. There is, however, a modification I intend to make in the future where different segments of the embedding vector encode different edge types and/or features.
…ordered AlgorithmResult; make tidy NOTE: make tidy applied formatting to: python/tests/graphql/test_nodes_property_filter.py, python/tests/graphql/test_graph_nodes_edges_property_filter.py, and python/python/raphtory/__init__.pyi
NOTE: when running black, some minor formatting changes applied to
python/python/raphtory/graphql/__init__.pyi
,python/python/raphtory/__init__.pyi
,python/tests/graphql/edit_graph/test_graphql.py
,python/tests/test_graphdb/test_graphdb.py
, andpython/python/raphtory/algorithms/__init__.pyi
(this last one was expected, since it was auto-generated by pyO3)What changes were proposed in this pull request?
Added FastRP to algorithms list. Added: Rust implementation; Rust test; Python integration; Python test
Why are the changes needed?
Always good to expand the algorithms library!
Does this PR introduce any user-facing change? If yes is this documented?
Yes. The Python and Rust functions include docstrings.
How was this patch tested?
The test includes a small sample graph with a seeded result which conforms to the expectations of the embedding algorithm.
Are there any further changes required?
(This goes for all algorithms!) Add expanded validation testing + scale testing.