Skip to content

Commit ae83c3c

Browse files
fix(#1532): edge weight encodes cardinality, not master-part (#1533)
Line weight is binary and encodes cardinality only: thick when the foreign key constitutes the child's entire primary key (1:1), thin when the child has primary-key attributes beyond those the FK contributes (multi-valued) — newly declared or inherited from another FK. penwidth already followed this via multi; remove the misleading master-part conflation in the layout weight and drive it from the same predicate so the two never diverge. Rename-safe: multi compares the child's referencing columns to the child primary key (both child-column space), so a renamed FK that is the child's whole PK is correctly 1:1/thick. Adds a guardrail test (1:1, multi, master-part, renamed-1:1).
1 parent 210276b commit ae83c3c

2 files changed

Lines changed: 136 additions & 6 deletions

File tree

‎src/datajoint/diagram.py‎

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1540,19 +1540,24 @@ def make_dot(self):
15401540
# pydot edge — to_pydot stringifies the edge data, so booleans arrive
15411541
# as "True"/"False". This is parallel-edge-safe: each FK between the
15421542
# same pair of tables is its own pydot edge.
1543-
src = edge.get_source()
1544-
dest = edge.get_destination()
15451543
primary = str(edge.get("primary")) == "True"
15461544
multi = str(edge.get("multi")) == "True"
15471545
aliased = str(edge.get("aliased")) == "True"
15481546
# Renamed FK → distinct color; others → the usual translucent black.
15491547
edge.set_color("#FF8800" if aliased else "#00000040")
15501548
edge.set_style("solid" if primary else "dashed")
1551-
dest_node_type = graph.nodes[dest].get("node_type")
1552-
master_part = dest_node_type is Part and dest.startswith(src + ".")
1553-
edge.set_weight(3 if master_part else 1)
1554-
edge.set_arrowhead("none")
1549+
# Line weight encodes cardinality, and only cardinality. `multi` is
1550+
# True when the child has primary-key attributes beyond those this
1551+
# foreign key contributes — whether newly declared or inherited from
1552+
# another foreign key — i.e. a one-to-many dependency, drawn thin.
1553+
# When the foreign key constitutes the child's *entire* primary key
1554+
# the dependency is 1:1, drawn thick. Master-part is NOT a weight: a
1555+
# part almost always adds a key attribute, so its edge is thin under
1556+
# this same rule. penwidth is the visible thickness; the layout
1557+
# `weight` hint follows the same predicate so the two never diverge.
15551558
edge.set_penwidth(0.75 if multi else 2)
1559+
edge.set_weight(1 if multi else 3)
1560+
edge.set_arrowhead("none")
15561561

15571562
# Group nodes into schema clusters (always on)
15581563
if schema_map:
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
"""
2+
Guards the diagram edge-weight (cardinality) rule (#1532).
3+
4+
Line weight encodes cardinality only, and it is binary:
5+
- **thick** (penwidth 2): the foreign key constitutes the child's *entire*
6+
primary key -> a 1:1 dependency.
7+
- **thin** (penwidth 0.75): the child has primary-key attributes beyond those
8+
the foreign key contributes (newly declared, or inherited from another foreign
9+
key) -> a one-to-many dependency.
10+
11+
Master-part is NOT a weight: a part almost always adds a key attribute, so its
12+
edge is thin under this same rule. This test pins that, since the historical
13+
documentation inverted it ("thick = master-part").
14+
"""
15+
16+
import time
17+
18+
import pytest
19+
20+
import datajoint as dj
21+
22+
THICK = 2.0
23+
THIN = 0.75
24+
25+
26+
@pytest.fixture(scope="function")
27+
def schema_by_backend(connection_by_backend, db_creds_by_backend):
28+
backend = db_creds_by_backend["backend"]
29+
test_id = str(int(time.time() * 1000))[-8:]
30+
schema_name = f"djtest_edgewt_{backend}_{test_id}"[:64]
31+
if connection_by_backend.is_connected:
32+
try:
33+
connection_by_backend.query(
34+
f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(schema_name)}"
35+
)
36+
except Exception:
37+
pass
38+
schema = dj.Schema(schema_name, connection=connection_by_backend)
39+
yield schema
40+
if connection_by_backend.is_connected:
41+
try:
42+
connection_by_backend.query(
43+
f"DROP DATABASE IF EXISTS {connection_by_backend.adapter.quote_identifier(schema_name)}"
44+
)
45+
except Exception:
46+
pass
47+
48+
49+
def _penwidth_by_dest(dot):
50+
"""Map each edge's destination-node tail -> penwidth (float)."""
51+
out = {}
52+
for edge in dot.get_edges():
53+
dest = edge.get_destination().strip('"').lower()
54+
try:
55+
pw = float(edge.get_penwidth())
56+
except (TypeError, ValueError):
57+
pw = None
58+
out.setdefault(dest, []).append((edge.get_source().strip('"').lower(), pw))
59+
return out
60+
61+
62+
def _penwidth_for(edges_by_dest, dest_name):
63+
matches = edges_by_dest.get(dest_name, [])
64+
assert matches, f"no edge found into node {dest_name!r}; nodes: {list(edges_by_dest)}"
65+
return matches
66+
67+
68+
def test_edge_weight_encodes_cardinality(schema_by_backend):
69+
if not dj.diagram.diagram_active:
70+
pytest.skip("networkx/pydot not available")
71+
72+
@schema_by_backend
73+
class Parent(dj.Manual):
74+
definition = """
75+
parent_id : int32
76+
"""
77+
78+
class Part(dj.Part):
79+
definition = """
80+
-> master
81+
part_id : int32
82+
"""
83+
84+
@schema_by_backend
85+
class OneToOne(dj.Manual):
86+
definition = """
87+
-> Parent
88+
"""
89+
90+
@schema_by_backend
91+
class OneToMany(dj.Manual):
92+
definition = """
93+
-> Parent
94+
sub_id : int32
95+
"""
96+
97+
@schema_by_backend
98+
class RenamedOneToOne(dj.Manual):
99+
# A renamed foreign key can still be 1:1: the renamed column is
100+
# RenamedOneToOne's entire primary key, so the dependency is 1:1 -> thick.
101+
# The rule must compare child columns to the child PK, not parent-PK
102+
# names to child-PK names (which renaming would break).
103+
definition = """
104+
-> Parent.proj(alt_parent_id='parent_id')
105+
"""
106+
107+
dot = dj.Diagram(schema_by_backend).make_dot()
108+
edges = _penwidth_by_dest(dot)
109+
110+
# 1:1 — the FK is OneToOne's entire primary key -> thick.
111+
assert all(
112+
pw == THICK for _, pw in _penwidth_for(edges, "onetoone")
113+
), f"1:1 dependency must be thick ({THICK}); edges={edges}"
114+
# multi-valued — OneToMany adds `sub_id` -> thin.
115+
assert all(
116+
pw == THIN for _, pw in _penwidth_for(edges, "onetomany")
117+
), f"multi-valued dependency must be thin ({THIN}); edges={edges}"
118+
# master -> part — the part adds `part_id` -> thin (NOT thick).
119+
assert all(
120+
pw == THIN for _, pw in _penwidth_for(edges, "parent.part")
121+
), f"master-part edge must be thin ({THIN}); it is not a 1:1 dependency; edges={edges}"
122+
# renamed FK that is the child's whole primary key — still 1:1 -> thick.
123+
assert all(
124+
pw == THICK for _, pw in _penwidth_for(edges, "renamedonetoone")
125+
), f"a renamed 1:1 foreign key must be thick ({THICK}); the rule must be rename-safe; edges={edges}"

0 commit comments

Comments
 (0)