Skip to content

Graph filter that only keeps currently valid edges #1969

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

Open
wants to merge 91 commits into
base: master
Choose a base branch
from

Conversation

ljeub-pometry
Copy link
Collaborator

@ljeub-pometry ljeub-pometry commented Mar 6, 2025

What changes were proposed in this pull request?

  • refactor of the time semantics to simplify history filtering
  • filtering layers/edges now filters nodes as well if all the edge updates that added them are filtered out and the node is not added explicitly via add_node.
  • changes latest_time semantics for the PersistentGraph to return the time of the last update for the node/edge/graph in the current view or the start of the window if there are no updates.
  • The earliest_time and latest_time within a filtered Event Graph will now reflect the updates within the graph view instead of just window bounds.
  • adds a ValidGraph filter that only keeps edges that are currently valid without removing their history
  • is_valid and is_active are no longer the same as active means there is an update during the period, valid means that the edges most recent update is an addition.
  • The default layer only exists if it has updates on it.

Why are the changes needed?

Does this PR introduce any user-facing change? If yes is this documented?

How was this patch tested?

Are there any further changes required?

@ljeub-pometry ljeub-pometry force-pushed the feature/IsValidFilter branch from 7f2d03d to dfbb7a8 Compare March 10, 2025 12:58
@ljeub-pometry ljeub-pometry force-pushed the feature/IsValidFilter branch from 14b8b49 to bab1997 Compare March 24, 2025 13:11
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark 'Rust Benchmark'.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 73303d6 Previous: a9e6f61 Ratio
lotr_graph/num_nodes 9 ns/iter (± 0) 2 ns/iter (± 0) 4.50
lotr_graph_window_100/iterate nodes 14502 ns/iter (± 47) 3128 ns/iter (± 17) 4.64
lotr_graph_window_10/iterate nodes 16687 ns/iter (± 77) 7620 ns/iter (± 18) 2.19
lotr_graph_window_10/iterate edges 80248 ns/iter (± 2019) 30649 ns/iter (± 180) 2.62
lotr_graph_subgraph_10pc/num_nodes 24 ns/iter (± 0) 4 ns/iter (± 0) 6
lotr_graph_subgraph_10pc_windowed/has_node_existing 127 ns/iter (± 10) 62 ns/iter (± 10) 2.05
lotr_graph_window_50_layered/num_edges 150188 ns/iter (± 1958) 29100 ns/iter (± 4308) 5.16
lotr_graph_window_50_layered/num_edges_temporal 1335495 ns/iter (± 8703) 135255 ns/iter (± 2693) 9.87
lotr_graph_window_50_layered/has_edge_existing 368 ns/iter (± 19) 82 ns/iter (± 7) 4.49
lotr_graph_window_50_layered/active edge 1396 ns/iter (± 121) 543 ns/iter (± 59) 2.57
lotr_graph_window_50_layered/num_nodes 42410 ns/iter (± 495) 18320 ns/iter (± 350) 2.31
lotr_graph_window_50_layered/has_node_existing 256 ns/iter (± 14) 67 ns/iter (± 11) 3.82
lotr_graph_window_50_layered/max_degree 467372 ns/iter (± 6190) 187407 ns/iter (± 744) 2.49
lotr_graph_window_50_layered/iterate nodes 52199 ns/iter (± 228) 12054 ns/iter (± 7) 4.33
lotr_graph_window_50_layered/iterate edges 291422 ns/iter (± 867) 48833 ns/iter (± 69) 5.97
lotr_graph_window_50_layered/max_neighbour_degree 539005 ns/iter (± 851) 251910 ns/iter (± 1850) 2.14
lotr_graph_window_50_layered_materialise/materialize 6082391 ns/iter (± 32470) 2692116 ns/iter (± 8354) 2.26
lotr_graph_persistent_window_50_layered/num_edges 140467 ns/iter (± 832) 56156 ns/iter (± 279) 2.50
lotr_graph_persistent_window_50_layered/num_edges_temporal 1506561 ns/iter (± 90010) 167588 ns/iter (± 1383) 8.99
lotr_graph_persistent_window_50_layered/has_edge_existing 371 ns/iter (± 43) 142 ns/iter (± 48) 2.61
lotr_graph_persistent_window_50_layered/active edge 1419 ns/iter (± 99) 496 ns/iter (± 73) 2.86
lotr_graph_persistent_window_50_layered/num_nodes 281776 ns/iter (± 6580) 14183 ns/iter (± 164) 19.87
lotr_graph_persistent_window_50_layered/has_node_existing 1682 ns/iter (± 173) 27 ns/iter (± 0) 62.30
lotr_graph_persistent_window_50_layered/max_id 340938 ns/iter (± 18929) 23770 ns/iter (± 55) 14.34
lotr_graph_persistent_window_50_layered/max_degree 822055 ns/iter (± 57010) 252203 ns/iter (± 1269) 3.26
lotr_graph_persistent_window_50_layered/iterate nodes 573267 ns/iter (± 1492) 8390 ns/iter (± 7) 68.33
lotr_graph_persistent_window_50_layered/iterate edges 272285 ns/iter (± 245) 75169 ns/iter (± 406) 3.62
lotr_graph_persistent_window_50_layered_materialise/materialize 11253510 ns/iter (± 70161) 3792085 ns/iter (± 120234) 2.97

This comment was automatically generated by workflow using github-action-benchmark.

@CLAassistant
Copy link

CLAassistant commented Mar 24, 2025

CLA assistant check
All committers have signed the CLA.

@ljeub-pometry ljeub-pometry marked this pull request as ready for review April 24, 2025 10:53
Copy link
Collaborator

@miratepuffin miratepuffin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part way done

@@ -148,7 +148,7 @@ where
.iter()
.flat_map(|e| e.explode())
.merge_by(inc.iter().flat_map(|e| e.explode()), |e1, e2| {
e1.time_and_index() < e2.time_and_index()
e1.time_and_index().unwrap() < e2.time_and_index().unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miratepuffin Make a ticket for exploded edges being converted into different types

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.

3 participants