feat(math): add chip-firing domain with Laplacian and vertex firing (#1741) - #2017
feat(math): add chip-firing domain with Laplacian and vertex firing (#1741)#2017morluto wants to merge 1 commit into
Conversation
Create the chip_firing domain with two operations: - graph.chip_firing.laplacian.compute: compute the exact graph Laplacian L = D - A with degree vector and labelled axes. - graph.chip_firing.fire_vertex.compute: fire a vertex in a chip configuration, transferring one chip per edge to neighbors. Partially addresses #1741.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
morluto
left a comment
There was a problem hiding this comment.
Review verdict: one contract/correctness blocker
LabelledGraph documents and advertises a simple undirected graph, but it never rejects duplicate undirected edges. Both
edges=(("a", "b"), ("a", "b"))and
edges=(("a", "b"), ("b", "a"))are accepted and increment the adjacency entry twice. The returned Laplacian and firing operation therefore use multigraph multiplicities while the wire contract says simple graph. This also makes MAX_DEGREE ineffective: per-vertex degree is never checked, and repeated edges can exceed it.
Please canonicalize each undirected edge (for example by vertex index), reject repeated canonical pairs, and add tests for same-orientation and reversed-orientation duplicates. Alternatively, deliberately change the model to a multigraph contract and enforce the promised degree bound; the current hybrid is misleading.
Apart from that mismatch, I checked the two kernels: L = D - A, the firing update D' = D - L e_v, and total-chip conservation are correct for the represented undirected edge multiplicities.
Deep review summaryVerdict: REQUEST CHANGES — the wire contract says simple graph, but the implementation accepts and computes with multiedges.
edges=(("a", "b"), ("a", "b"))and edges=(("a", "b"), ("b", "a"))are accepted and increment the adjacency entry twice. The returned Laplacian and firing operation therefore use multigraph multiplicities while the public contract says simple graph. This also makes Canonicalize every undirected edge, reject repeated canonical pairs, and add tests for same-orientation and reversed-orientation duplicates. The alternative is to deliberately expose a multigraph contract and enforce the promised degree bound; the current hybrid is misleading. Apart from that mismatch, the kernels themselves are correct for the represented multiplicities: |
Summary
Create the
chip_firingdomain with two operations, partially addressing #1741.Operations
graph.chip_firing.laplacian.compute— Compute the exact graph Laplacian L = D - A with degree vector and labelled vertex axes.graph.chip_firing.fire_vertex.compute— Fire a vertex in a chip configuration: the fired vertex loses degree(v) chips and each neighbor gains one chip per edge.Tests
7 known-answer tests covering:
Continue this on Linzumi