Constrain predicate groundings by node type (#236) - #239
Conversation
#235 established that an exact label match is not sufficient to ground a predicate, and that nothing in the repo could have caught it. `causally upstream of` IS the label of RO:0002411, so it passed the id↔label gate — but RO defines the relation over occurrents, and all 13 corpus edges carrying it connect material entities. The gate compares a CURIE to its label and never looks at the edge; predicate_id is an unbound string in the schema; nothing in qc inspects domain or range. mappings/predicate_grounding.tsv gains subject_types and object_types: `|`-separated CausalNodeTypeEnum names, or `*` for any. All 99 existing rows are `*`/`*` explicitly rather than by omission, so the file states its own permissiveness instead of relying on a missing column. ground_causal_predicates.py builds a node_id->node_type map per graph and refuses edges outside the declared types. They stay in the residual — a wrong grounding is worse than a missing one — and are reported under their own heading with the shape that failed, because folding them into the plain residual would read as "no CURIE known", which is the opposite of the truth. `causally upstream of` is restored to the mapping WITH a process constraint. It grounds zero edges, deliberately: the CURIE is right, the corpus usage is not, and recording that is more useful than leaving 13 edges silently residual with no explanation. Running it now prints exactly the three shapes the #235 review identified: causally upstream of (CHEMICAL->CHEMICAL) 6 causally upstream of (GENE_OR_PROTEIN->CHEMICAL) 6 causally upstream of (GENE_OR_PROTEIN->CAPACITY) 1 So the mechanism demonstrably blocks the exact mistake it was built for. Six new tests plus the nine existing ones updated for the wider signature: blocked outside the types, grounded inside them, object type alone can block, unconstrained rows still ground anything, a node with no declared type cannot satisfy a constraint, and both `*` and empty parse as "any". No corpus change: 0 newly grounded, residual unchanged at 1221. Whether to constrain the regulation relations — RO:0002213 is 13% process->process, RO:0002212 is 8% — is the separate curation decision #236 records, and now has a mechanism to express. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 3m 46s —— View job Review — PR #239The mechanism itself is right, and the RO:0002411 constraint is semantically sound (GO The problem is that the block is only visible on stderr, which does not survive the run. Every durable artifact still says "no CURIE known" — the thing the code comment at 🟡 The residual TSV does not distinguish blocked rows from unmapped ones
indistinguishable from the ~1200 genuinely unmapped labels. That file is the durable, committed, staleness-gated one, and it is a documented input contract: 🟡
|
![culturebot-reviewer[bot]](https://avatars.githubusercontent.com/u/233210009?s=60&v=4){kind=small}
There was a problem hiding this comment.
Mechanism is right and the RO:0002411 constraint is semantically sound. Requesting changes on three points, all about the block being visible only on stderr: (1) the residual TSV does not distinguish blocked rows from unmapped ones, and that file is a documented input contract for the ground-or-propose-metpo workflow; (2) NEXT_TASKS.md section 9 still queues 'causally upstream of' for backfill and still asserts the label-correspondence gate is sufficient; (3) subject_types/object_types values are never validated against CausalNodeTypeEnum, so a typo silently blocks every edge. Details in the review comment.
All three 🟡s were about the same gap — the constraint fired, but only into a stderr summary that scrolls away. The residual TSV is the durable work queue, and it listed a blocked label identically to an unmapped one. A curator reading "causally upstream of, 13" would try to map it again, which is exactly the mistake the constraint just prevented. Added `status` and `blocked_by` columns: blocked rows now read `blocked_by_node_type` / `RO:0002411`. Nothing validated the type names, so a typo like BIOLOGICAL_PROCES would silently block every edge the row could ground. The permissible values are now read from CausalNodeTypeEnum in the schema — rather than hardcoded, so a schema change cannot leave this file quietly disagreeing — and an unknown name raises. A constraint nobody can see is worse than no constraint. NEXT_TASKS section 9 still queued `causally upstream of` as a free win and still credited label-correspondence with making the loop safe, which this work disproved. It now strikes that row, says why an exact label match was not sufficient, and points at the type columns as the thing that actually checks it — including the caveat that rows left at */* are unconstrained, so a new mapping is only as safe as the constraint written beside it. 233 tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 3m 3s —— View job Review — PR #239 (re-review after
|
There was a problem hiding this comment.
Two of the three earlier points are closed: the residual TSV now carries status/blocked_by and reads 'causally upstream of 13 blocked_by_node_type RO:0002411', and the enum values are read from CausalNodeTypeEnum rather than hardcoded. NEXT_TASKS section 9 is corrected.
Two left: (1) .claude/commands/ground-or-propose-metpo.md still documents the old three-column contract and still names 'causally upstream of' in the head of the worklist, with no instruction to skip blocked rows — the consuming workflow is the loop this PR exists to close; (2) ground_causal_predicates.py:253 recovers the predicate label by splitting the display key on ' (', and labels containing ' (' exist in the corpus today ('positively influences (saturating)'), so a constrained one would be mislabeled 'unmapped' or KeyError in the staleness gate. Details in the review comment.
blocked_total was keyed by the rendered string f"{pred} ({s}->{o})" and the
label recovered with split(" (")[0]. That truncates any label containing " (",
and the corpus has one — `positively influences (saturating)`. Give it a
constraint and the residual TSV would mark `positively influences` blocked
while the real row read `unmapped`: the wrong row flagged, the right one
silently wrong. Keyed by the (label, shape) tuple now, with a test using that
exact corpus label.
.claude/commands/ground-or-propose-metpo.md is the tracked file that drives
this loop, and it was worse than out of date — line 64 explicitly instructed
`causally upstream of`→`RO:0002411`, the mistake this PR exists to prevent,
as a "straight to RO" freebie. Removed, with the reason stated.
Also updated there: the residual column list now includes status/blocked_by;
step 1 gains "skip every row whose status is blocked_by_node_type" with the
explanation that the CURIE is known and deliberately withheld; and the
head-of-worklist example no longer advertises `causally upstream of`.
Restored `causally upstream of or within`→`RO:0002418`, which I had removed as
collateral because it shared a line — it is a different term and dropping it
was not intended. It carries the same hazard, so the whole RO list now says
these are processual relations and each needs subject_types/object_types
rather than */*.
234 tests.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 3m 26s —— View job Review — PR #239 (re-review after
|
There was a problem hiding this comment.
Both prior 🟡s are properly closed: the blocked counter is keyed on (label, shape) so the split-on-' (' failure mode is removed rather than guarded, and the command doc now carries status/blocked_by plus the skip rule. Good catch on line 64 of that doc having actively instructed the RO:0002411 mapping — that was the live source of the error, not just lagging documentation.
One new 🟡, introduced by this commit: the sentence added at .claude/commands/ground-or-propose-metpo.md:61-63 says 'These are all processual relations — RO defines them over occurrents' about a list that includes enables (RO:0002327, domain material entity — your own mapping row at predicate_grounding.tsv:9 says 'node→activity edges'), part of / has part (BFO:0000050/51, domain-neutral), produces (RO:0003000, range material entity — predicate_grounding.tsv:24 calls it 'the material-entity→product slot'), and has input/output (RO:0002233/4, range continuant). An agent following that premise would constrain enables to process-only subject types and block 100% of its correct edges — and that constraint passes the new enum check, because the names are real. It is the #235 shape with the sign flipped, in the file that tells the next agent how to write domain constraints. Scope the processual claim to the regulation/causal subset and say the participation and parthood relations must be checked individually.
Details in the review comment. No corpus change involved.
"These are all processual relations" was true for the regulation and causal subset and wrong for the rest of the same list. Checked against RO rather than re-asserted: `enables` (RO:0002327) is "c enables p iff c is capable of p" — material entity to process; `produces` (RO:0003000) is defined "where a and b are material entities"; `part of`/`has part` (BFO) are deliberately domain-neutral. The repo's own mapping notes said as much for two of them. Replaced with the rule that actually generalises: write the type columns for every row and read each relation's own domain and range to decide what they are, with the four cases spelled out so the next reader does not have to re-derive them. A wrong blanket rule in the file that drives the loop is worse than none — it would have been applied to `enables` and `produces` on the next run. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 2m 39s —— View job Review — PR #239 (re-review after
|
There was a problem hiding this comment.
The last 🟡 is closed, and closed by checking rather than re-asserting. The blanket 'these are all processual relations' is replaced with the rule that generalises — write the type columns for every row and read each relation's own domain and range — plus the four cases spelled out: regulation/causal over occurrents, enables material entity → process, produces material-entity-to-material-entity, part of / has part domain-neutral. Each is a correct reading, and matches what this repo's own mapping notes already said for two of them. Giving the next reader the four answers rather than just deleting the wrong sentence is the better fix.
Re-checked and holding: blocked keyed on (label, shape) with the real corpus label 'positively influences (saturating)' in the test; mapping[label][0] guarded by is_blocked with no KeyError path; residual TSV row 12 reads 'causally upstream of 13 blocked_by_node_type RO:0002411' and its header matches the command doc's column contract; the mapping row is 9 fields against a 9-column header with the quoted notes field intact; the three type names are real CausalNodeTypeEnum values. Both Python consumers of either TSV use csv.DictReader and nothing reads them positionally, so the added columns break no consumer.
One 🔵 for follow-up, not this PR: the type check is reached only for edges with an empty predicate_id, so a hand-applied violating grounding is never surfaced. A qc check over grounded edges reading the same two columns would turn this into a ratchet.
Not verified: uv/just/python are outside this job's allowed tools, so '234 tests pass' and the residual being byte-identical to the generator's output are unchecked by me — just qc regenerates and diffs, so CI proves the latter. No history/ record needed; no data/traits/** changes.
1 participant
Closes #236. The precondition for fanning out the grounding loop.
What #235 established
An exact label match is not sufficient to ground a predicate, and nothing in the repo could have caught it.
causally upstream ofIS the label of RO:0002411, so it passed the id↔label gate — but RO defines the relation over occurrents, and all 13 corpus edges carrying it connect material entities. The gate compares a CURIE to its label and never looks at the edge;predicate_idis an unbound string in the schema; nothing inqcinspects domain or range.The mechanism
mappings/predicate_grounding.tsvgainssubject_typesandobject_types—|-separatedCausalNodeTypeEnumnames, or*for any. All 99 existing rows are*/*explicitly rather than by omission, so the file states its own permissiveness instead of relying on a missing column.ground_causal_predicates.pybuilds anode_id → node_typemap per graph and refuses edges outside the declared types. They stay in the residual — a wrong grounding is worse than a missing one — and are reported under their own heading, because folding them into the plain residual would read as "no CURIE known", which is the opposite of the situation.It demonstrably blocks the mistake it was built for
causally upstream ofis restored to the mapping with a process constraint. It grounds zero edges, deliberately: the CURIE is right, the corpus usage is not, and recording that is more useful than leaving 13 edges silently residual with no explanation.Exactly the three shapes the #235 review identified.
Tests
Six new, plus the nine existing ones updated for the wider signature: blocked outside the types, grounded inside them, object type alone can block, unconstrained rows still ground anything, a node with no declared type cannot satisfy a constraint, and both
*and empty parse as "any".No corpus change
0 newly grounded, residual unchanged at 1221,
just qcexits 0, 232 tests pass.Whether to constrain the regulation relations — RO:0002213 is 27/206 process→process (13%), RO:0002212 is 9/102 (8%) — is the separate curation decision #236 records. It now has a mechanism to express, which it did not before.
🤖 Generated with Claude Code