-
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
Feature/node state ops #1854
Feature/node state ops #1854
Conversation
0ec7984
to
40b71c4
Compare
517783c
to
335b259
Compare
fn filtered<G: GraphViewOps<'graph>>(&self, graph: G) -> Self::Filtered<G>; | ||
} | ||
|
||
#[derive(Debug, Clone, Copy)] |
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.
I wonder if many of these could just be special cases of a more generic
struct NodeOpFn<O, F: Fn(&GraphStorage, VID) -> O>(F)
and could be just made from constructors like
fn id()-> ...
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.
If you do this you can no longer name the type which causes a bunch of problems, though maybe we can get away with fn
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 file alone makes me SERIOUSLY want to reconsider the way we use generics to filter the graph
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.
LGTM
What changes were proposed in this pull request?
Make lazy node state views support time ops and layer ops
Why are the changes needed?
so you can get a windowed degree for all nodes in the graph for example
Does this PR introduce any user-facing change? If yes is this documented?
Adds additional APIs that are documented
How was this patch tested?
Needs more tests
Are there any further changes required?