An EventGraph class with tests

[vineetbansal]

Introduced an EventGraph class with tests to illustrate basic functionality. EventGraph is a pathpyG Graph so in addition to being constructed from a TemporalGraph, it is capable of being constructed directly from a torch_geometric.data.Data object.

Essentially the usage might be:

  tg = TemporalGraph.from_edge_list([
      ("a", "b", 1),
      ("b", "c", 2),
      ("b", "d", 5),
      ("c", "e", 3),
  ])

  eg = EventGraph.from_temporal_graph(tg, delta=2)

  print(eg)                    # EventGraph (delta=2) ... one line per event, like a->b@1
  print(eg.num_events)         # 4  (one per temporal edge)
  print(eg.num_fo_nodes)       # 5  (a, b, c, d, e)

  print(eg.event_endpoints(0)) # ('a', 'b', 1)
  print(eg[0])                 # same thing
  print(eg.continuations(0))   # [(1, 1)]  -> event 1, with a time gap of 1

  dist, pred = eg.shortest_paths()
  print(dist[eg.fo_mapping.to_idx("a"), eg.fo_mapping.to_idx("e")])  # 3.0

  tg_back = eg.to_temporal_graph()
  
  # This is not part of this PR, but can be added easily.
  model = MultiOrderModel.from_event_graph(event_graph, max_order=2)

A couple of things to discuss:

• If an EventGraph is constructed from a TemporalGraph and the TemporalGraph is later changed by the caller, should the EventGraph change as well? Right now we don't - eg._temporal_graph = TemporalGraph(g.data.clone(), mapping=g.mapping). This seems to me like the right thing to do.

• eg.shortest_paths works, but creates a TemporalGraph (if it doesn't exist) to pass on to temporal_shortest_paths. This seems wasteful if EventGraph was constructed from Data and doesn't need all the TemporalGraph machinery. Perhaps temporal_shortest_paths can be tweaked to work with a pre-computed edge-index (as would be the case if EventGraph is constructed directly from torch_geometric.data.Data).

  • Decision - change signature to temporal_shortest_paths(g: TemporalGraph | None, eg: "EventGraph" | None, delta: int). Add methods to EventGraph/TemporalGraph that call this function under the hood.

Some changes in other files were for my understanding of the code by adding comments, and are unrelated to the PR.

[vineetbansal]

Some TODOs that came out of discussion:

• Make lift_order_temporal a method in EventGraph.
• Change signature of temporal_shortest_paths to temporal_shortest_paths(g: TemporalGraph | None, eg: "EventGraph" | None, delta: int). Add methods to EventGraph/TemporalGraph that call this function under the hood.
• Add a reduce_delta method that (smartly) constructs a new EventGraph object. Useful for delta sweeps in MultiOrderModel.
• Avoid using the term continuations + make private.
• Add a mapping attribute (mapping labels like a->b@1 to event ids).

[M-Lampert]

Thanks for keeping track of everything we discussed in the meeting. Some additional suggestions:

• I think we should rename lift_order_temporal to create_event_graph_index or something similar that explains better what the method actually does.
• About continuations: You probably got it from the following line, right?

  pathpyG/src/pathpyG/algorithms/temporal.py

  Line 36 in 8b1c2f4

  # lift order: find possible continuations for edges in each time stamp

  I think we use this functionality only in the event graph construction, and there we cannot make use of this function since we do not have the EventGraph object yet. So, I am not sure if we should add this function at all. Once we are at a stable version, removing it would mean a breaking change. Instead, later adding it would not be a problem if we need it at some point.
• Adding the mapping attribute would make the event_endpoints function almost redundant. Not sure if we should keep the function then.
• Regarding num_fo_nodes, I think I would prefer not having abbreviations in the function names since not all people might now that it refers to "first-order". Additionally, in Graph we use Graph.n as the property to get the number of nodes. We should make this consistent. Maybe n_first_order?

I think these are all things that we need to agree on now since changing them later would mean breaking changes. So, maybe you can also add what you think @hackl and @IngoScholtes

FYI: Just merged the pytest CI fix to main, so after rebasing you shouldn't run into problems anymore.

[vineetbansal]

@M-Lampert - I've moved the lift_order_temporal function to be a @staticmethod build_edge_index in EventGraph so it looks like:

build_edge_index(g: TemporalGraph, delta: float | int = 1):

and returns the exact same thing as before (the ho_index). This hopefully addresses the concern that it needs to live in the EventGraph namespace but be flexible enough to work with any temporal graph or delta passed in.

If not, let me know - happy to move around.

For the purposes of fixing the documentation related to this, let me know if simply renaming lift_order_temporal to EventGraph.build_edge_index but still calling it a "function" (it is functional), instead of a "method" (which might be confusing for the target audience?) in the docs would be acceptable.

Of course this is to get the docs consistent with the PR and get the CI to pass wrt this PR - it might move again in the (near) future.

[M-Lampert]

Just replacing every lift_order_temporal with EventGraph.build_edge_index in the docs and still calling it a "function" seems fine to me.