Skip to content

HigherOrderGraph / modified MultiOrderModel - #329

Open
vineetbansal wants to merge 11 commits into
devfrom
vb/hograph
Open

vineetbansal wants to merge 11 commits into
devfrom
vb/hograph

Conversation

@vineetbansal

Copy link
Copy Markdown
Collaborator

A HigherOrderGraph class of arbitrary order, constructable from temporal_graph/event_graph/path_data. It delegates most of the work to the MultiOrderModel class which does the iterations on the lifting (with cached=False). It is also constructable using from_aggregated, and has its own .lift method.

The MultiOrderModel class now has HigherOrderGraphs in its layers.

None of the tests for MultiOrderModel needed modifications and still pass, which is reassuring. Tests for HigherOrderModel and MultiOrderModel that assume HigherOrderModel in layers are coming next.

This PR assumes that the EventGraph branch is merged, as it builds on top of it.

Typical workflow using these new classes:

def data() -> pp.TemporalGraph:
    r"""

        a           d
          \        /
            c  (hub)
          /        \
        b           e

    """
    return pp.TemporalGraph.from_edge_list(
        [
            ("a", "c", 1), ("c", "d", 2),   # a -> c -> d
            ("b", "c", 3), ("c", "e", 4),   # b -> c -> e
            ("a", "c", 5), ("c", "d", 6),
            ("b", "c", 7), ("c", "e", 8),
        ]
    )

t = data()
DELTA = 1
eg = EventGraph.from_temporal_graph(t, delta=DELTA)
h1 = HigherOrderGraph.from_temporal_graph(t, order = 1)
assert h1.order == 1

h2 = HigherOrderGraph.from_event_graph(eg)
assert h2.order == 2

paths = PathData(IndexMap(list("abcde")))
paths.append_walk(("a", "c", "d"), weight=3)
paths.append_walk(("b", "c", "e"), weight=3)

h1b = HigherOrderGraph.from_path_data(paths, order = 1)
h1c = HigherOrderGraph.from_event_graph(eg, order = 2)

h5 = HigherOrderGraph.from_event_graph(eg, order=5)  # Create order 5 ho (but still has to go through 2->5 algorithmically)

print("\n=== HigherOrderGraph (order 2) ===")
print("order:", h2.order)                   # 2
print("nodes:", h2.nodes)                   # [('a','c'), ('b','c'), ('c','d'), ('c','e')]
print("edges:", h2.edges)                   # [(('a','c'),('c','d')), (('b','c'),('c','e'))]
print("weights:", h2.data.edge_weight)      # [2., 2.]

assert h2.order == 2
assert h2.n == 4                            # 8 events collapsed into 4 nodes
assert h2.n_first_order == 5
assert h2.first_order_mapping.to_id(0) == "a"

h3 = h2.lift()
assert isinstance(h3, HigherOrderGraph)
assert h3.order == 3
print("order-3 nodes:", h3.nodes)  # [('a', 'c', 'd'), ('b', 'c', 'e')]


MAX_ORDER = 2

# build MultiOrderModel from TemporalGraph
m = MultiOrderModel.from_temporal_graph(t, delta=DELTA, max_order=MAX_ORDER)

for k, layer in sorted(m.layers.items()):
    print(f"  layer {k}: order={layer.order}  n={layer.n}  m={layer.m}")
    #   layer 1: order=1  n=5  m=4
    #   layer 2: order=2  n=4  m=2
    assert isinstance(layer, HigherOrderGraph)
    assert layer.order == k
    assert layer.n_first_order == t.n

# build MultiOrderModel from EventGraph
m_via_eg = MultiOrderModel.from_event_graph(eg, max_order=MAX_ORDER)
assert m_via_eg.layers[2].edges == m.layers[2].edges

# build MultiOrderModel from PathData
paths = pp.PathData(pp.IndexMap(list("abcde")))
paths.append_walks(node_seqs=[("a", "c", "d"), ("b", "c", "e")], weights=[4, 4])
m_paths = MultiOrderModel.from_path_data(paths, max_order=MAX_ORDER)

@vineetbansal vineetbansal changed the title Vb/hograph HigherOrderGraph / modified MultiOrderModel Aug 13, 2026

@M-Lampert M-Lampert left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PR looks good already. I made some comments; let me know if you have any questions. I think some of the questions that I raised should be discussed together with the others in our next meeting. I said so in the comments as well.

Comment on lines +137 to +141
def _validate_order(order: int) -> None:
"""Reject orders for which no De Bruijn graph is defined."""
if order < 1:
logger.error("order must be at least 1, got %s", order)
raise ValueError(f"order must be at least 1, got {order}")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There could actually be a zeroth-order De Bruijn graph and it is currently an open question whether that is something that should be included in the MultiOrderModel or not. See #172
This is probably something that we should discuss together in the next meeting if this is something that we want the HigherOrderModel to be able to represent or not.

Comment thread src/pathpyG/core/higher_order_graph.py Outdated
return IndexMap([tuple(v.tolist()) for v in node_sequence])

@classmethod
def from_aggregated(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think that name is confusing since it requires an unaggregated graph as input. So the function should either be named aggregate or from_unaggregated.
We could also think about giving the unaggregated graph a more consistent name throughout the whole repository. Currently, it doesn't have a distinct name since it is just the representation for a step in between with the final result being the higher-order DeBruijn Graph. Some naming suggestions:

  • Higher-Order (path-)occurence graph
  • Higher-Order line graph
  • Lifted graph

If and how we name this intermediate unaggregated higher-order graph, is probably also something we should discuss in the next meeting.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

You're absolutely right - I'll rename this method aggregate for now.

Good point about the unaggregated graph naming too - I won't make any changes there for now but we can discuss it when we meet.

Comment on lines +361 to +382
def lift(self, aggr: str = "src") -> HigherOrderGraph:
"""Return the De Bruijn graph of order `k + 1` obtained by lifting this graph.

Nodes of the result are the edges of this graph, i.e. the paths of length `k + 1`
that exist in this graph's topology.

Args:
aggr: Aggregation used for the lifted edge weights. One of "src", "dst",
"max", "mul" or "add".

Returns:
HigherOrderGraph: A higher-order graph of order `k + 1`.
"""
edge_index = self.data.edge_index.as_tensor()
if "edge_weight" in self.data:
edge_weight = self.data.edge_weight
else:
edge_weight = torch.ones(edge_index.size(1), device=edge_index.device)

ho_index, node_sequence, ho_weight = lift_order_step(
edge_index, self.data.node_sequence, edge_weight=edge_weight, aggr=aggr
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this lift should use the pre-aggregation representation rather than self.data.edge_index.

At this point, edge_index and edge_weight have already been coalesced by from_aggregated(). Lifting them constructs paths implied by the aggregated De Bruijn topology, which can combine occurrences that were never observed consecutively in the source data and can propagate already-aggregated weights incorrectly.

inverse_idx already lets us reconstruct the pre-aggregation node sequences via:

node_sequence = self.data.node_sequence[self.data.inverse_idx]

If from_aggregated() also retains the occurrence-level edge index and weights (e.g. pre_aggregation_edge_index and pre_aggregation_edge_weight), then .lift() can call lift_order_step() on those tensors and aggregate only the result. That would make h.lift() consistent with the unaggregated iteration used by MultiOrderModel.

@vineetbansal vineetbansal Sep 3, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

As I was implementing this, I found that there is potential for some confusion here. Since HigherOrderGraph can be constructed from TemporalGraph or a general Graph or PathData, in other words, it can either have observations or aggregated nodes, and a pre-aggregation lift may not be possible. From the caller's perspective, it is not clear what lift() should do, since HigherOrderGraph doesn't tell the caller whether it still knows which observation continued which.

Perhaps it shouldn't exist at all?

The class that always has pre-aggregation data is MultiOrderModel, perhaps we provide a lift() or extend there, with an option of whether its a pre-aggregated lift or an aggregated lift? (it would have to start maintaining more state in hidden instance variables to support both I think).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Update from 09/10 - remove lift from HigherOrderGraph; add a new issue to add lift to MultiOrderModel.

Comment on lines +275 to +277
m.layers[1] = HigherOrderGraph.from_aggregated_graph(
g1, first_order_mapping=path_data.mapping, n_first_order=n_first_order
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we check if cached or max_order == 1: here?

Comment thread src/pathpyG/core/graph.py
Comment on lines 56 to 60
- `node_sequence`: Node sequence [tensor][torch.Tensor] of shape `(num_nodes, order)` where each entry
corresponds to the index of first-order nodes in the underlying graph and mapping. For first-order graphs,
the indices in the node sequence is identical to the indices in the edge index. For higher-order graphs,
the node sequence contains tuples of node indices representing higher-order nodes that correspond to paths in
the underlying first-order graph.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since we have a dedicated higher-order graph class now, we can remove the higher-order workarounds in Graph like the node_sequence attribute that is only needed for orders larger than 1.

Comment thread src/pathpyG/core/graph.py
Comment on lines 767 to 772
# For higher-order graphs, we need to update the inverse_idx attribute
if "inverse_idx" in d:
d.inverse_idx = mapping.to_idxs(
np.concatenate([m1.to_ids(d1.inverse_idx), m2.to_ids(d2.inverse_idx)]),
device=d.inverse_idx.device,
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is also something only necessary for higher-order graphs and can be removed now.

Comment thread src/pathpyG/core/graph.py
Comment on lines 661 to 668
@property
def order(self) -> int:
"""Return order of graph.

Returns:
int: order of the (De Bruijn) graph
"""
return self.data.node_sequence.size(1)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

From now on, Graph is exclusively used for order-1 graphs, so this property is not necessary anymore.

Comment thread src/pathpyG/core/higher_order_graph.py
@vineetbansal

vineetbansal commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Note from 09/10 meeting - okay to restrict Graph to order 1, and use HigherOrderGraph for the more general case, any order >= 0.

TODO: Vineet - add test cases for HigherOrderGraph as a start. @M-Lampert can then add test cases for order=0.

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