Skip to content

Commit 9035f23

Browse files
fix(diagram): don't duplicate shared edges when combining diagrams
On a MultiDiGraph, Diagram.__add__ used add_edges_from(arg.edges(data=True)), which assigns a fresh parallel-edge key per call — so every FK shared between two combined diagrams (they share the dependency graph) was duplicated, and a single foreign key rendered as multiple overlapping edges. Merge edges by their (u, v, key) instead, skipping ones already present. Adds tests/integration/test_erd.py::test_diagram_operators_no_duplicate_edges asserting +, +N, -N, *, add_parts, and from_sequence all preserve the dependency graph's edge multiplicity.
1 parent c560605 commit 9035f23

2 files changed

Lines changed: 37 additions & 2 deletions

File tree

‎src/datajoint/diagram.py‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,15 @@ def __add__(self, arg) -> "Diagram":
235235
"""
236236
result = Diagram(self) # copy
237237
try:
238-
# Merge nodes and edges from the other diagram
238+
# Merge nodes and edges from the other diagram. Add edges by their
239+
# multigraph key and skip ones already present: both operands derive
240+
# from the same dependency graph, so a shared FK has the same
241+
# (u, v, key) in each. Plain add_edges_from would mint a fresh key
242+
# per call and duplicate every shared edge (parallel-edge bug).
239243
result.add_nodes_from(arg.nodes(data=True))
240-
result.add_edges_from(arg.edges(data=True))
244+
for u, v, key, edata in arg.edges(keys=True, data=True):
245+
if not result.has_edge(u, v, key):
246+
result.add_edge(u, v, key=key, **edata)
241247
result.nodes_to_show.update(arg.nodes_to_show)
242248
# Merge contexts for class name lookups
243249
result.context = {**result.context, **arg.context}

‎tests/integration/test_erd.py‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,35 @@ def test_diagram_algebra(schema_simp):
4545
assert diag4.nodes_to_show == set(cls.full_table_name for cls in (B.C, E.F, E.G, E.H, E.M))
4646

4747

48+
def test_diagram_operators_no_duplicate_edges(schema_simp):
49+
"""No diagram operator may duplicate a foreign-key edge.
50+
51+
Regression for the ``nx.MultiDiGraph`` migration: because a plain
52+
``add_edges_from`` mints a fresh parallel-edge key on every call, combining
53+
diagrams (which share a dependency graph) doubled every shared FK, so a
54+
single foreign key rendered as multiple overlapping edges. Each operator's
55+
result must carry exactly the dependency graph's edge multiplicity for every
56+
pair of tables.
57+
"""
58+
deps = schema_simp.connection.dependencies
59+
deps.load()
60+
61+
def assert_no_dupes(diag, label):
62+
for u, v in {(u, v) for u, v in diag.edges()}:
63+
assert diag.number_of_edges(u, v) == deps.number_of_edges(u, v), (
64+
f"{label}: edge {u} -> {v} duplicated "
65+
f"({diag.number_of_edges(u, v)} vs {deps.number_of_edges(u, v)} in deps)"
66+
)
67+
68+
assert_no_dupes(dj.Diagram(schema_simp, context=LOCALS_SIMPLE), "Diagram(schema)")
69+
assert_no_dupes(dj.Diagram(A) + dj.Diagram(B) + dj.Diagram(D), "union +")
70+
assert_no_dupes(dj.Diagram(B) + 3, "downstream + N")
71+
assert_no_dupes(dj.Diagram(E) - 3, "upstream - N")
72+
assert_no_dupes((dj.Diagram(B) + 3) * (dj.Diagram(E) - 3), "intersection *")
73+
assert_no_dupes((dj.Diagram(B) + E).add_parts(), "add_parts")
74+
assert_no_dupes(dj.Diagram.from_sequence([A, B, D, E]), "from_sequence")
75+
76+
4877
def test_repr_svg(schema_adv):
4978
erd = dj.Diagram(schema_adv, context=dict())
5079
svg = erd._repr_svg_()

0 commit comments

Comments
 (0)