Skip to content

Reverse 15 electron-transfer edges off TRAIT subjects (#295) - #300

Merged
realmarcin merged 4 commits into
mainfrom
fix/295-electron-predicate-subject-domain
Aug 5, 2026
Merged

Reverse 15 electron-transfer edges off TRAIT subjects (#295)#300
realmarcin merged 4 commits into
mainfrom
fix/295-electron-predicate-subject-domain

Conversation

@realmarcin

@realmarcin realmarcin commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Closes #295 (for the electron pair; see follow-up below for the rest).

The issue's premise was right, but for a different reason than stated

The issue says METPO:2000008/METPO:2000009 "are declared with an organism
domain". They are not — not directly. Both declare no rdfs:domain and no
rdfs:range at all in the vendored data/raw/metpo.owl.

But both are rdfs:subPropertyOf METPO:2000001 ("organism interacts with
chemical"), and that property declares:

<rdfs:domain rdf:resource="https://w3id.org/metpo/1000525"/>   <!-- microbe -->
<rdfs:range  rdf:resource="https://w3id.org/metpo/1000526"/>   <!-- chemical entity -->

OWL propagates a domain through subPropertyOf, so the organism domain is
inherited and upstream. It is not self-inflicted by TraitMech's own
domain: declarations, and it cannot be edited away locally.

That matters because domain in OWL is an inference rule, not a constraint.
<trait> uses electron acceptor <chemical> does not fail validation — it makes
a reasoner conclude the TRAIT node is a microbe. predicate_id is an unbound
string in the schema, so nothing in just qc could see it. Same failure mode as
the RO:0002411 mapping in #235.

What changed

15 edges reversed (13 uses electron donor, 2 uses electron acceptor)
from <trait> --uses electron X--> <chemical> to
<chemical> --enables--> <trait> (RO:0002327), across 14 files.

This is the idiom the corpus already uses for exactly this shape: enables
appears 7x as CHEMICAL -> TRAIT, including ferric_iron --enables--> dir_trait
in dissimilatory_iron_reduction.yaml — the same graph two of the reversed
edges live in, with the description "Fe(III) acts as terminal electron acceptor
enabling DIR." Evidence blocks are untouched.

The donor/acceptor role is not lost: it stays in the object node's label and
grounding (inorganic_electron_donor, METPO:1007502) and in each edge
description, which is where the pre-existing sibling edge already carried it.

A forward guard against it recurring. subject_types for both rows in
mappings/predicate_grounding.tsv is now NONE — a new sentinel meaning "no
node type satisfies this domain", distinct from */empty ("any"). It was
needed because CausalNodeTypeEnum has no organism member (graphs are
deliberately taxon-agnostic), so the constraint was previously inexpressible:
"exclude TRAIT" would still have wrongly permitted CHEMICAL, GENE_OR_PROTEIN, etc.
ground_causal_predicates.py now refuses to ground such a label onto any edge
and reports blocked_by_node_type, mirroring how #236 handled RO:0002411.

Playbook corrected. docs/CURATION_PLAYBOOK.md previously taught this
pattern, with a worked example using a trait subject. It now shows the
chemical-subject form and explains why. The OBJECT_PROPERTY records' domain:
is unchanged and stays correct — the organism-subject form is valid at the
assertion site; only the causal-graph reuse was wrong.

Why not the alternatives

  • TRAIT as a stand-in for "an organism bearing the trait" makes the node
    polysemous. dir_trait rdfs:subClassOf anaerobic_respiration sits in the same
    graph, so the node is already committed to being a phenotype class. That
    ambiguity would propagate through the Causal graphs are 62% fragmented and audit-graphs cannot see it #183 backfill.
  • Adding ORGANISM nodes over-models graphs that are intentionally
    taxon-agnostic, and would need a new enum member used by nothing else.

Scope: this fixes ~4% of the actual defect

While verifying, I found the same inherited domain reaches 66 METPO
properties
, and the corpus asserts them on non-organism subjects in
366 edges across 193 files and 14 predicate labels:

count predicate count predicate
249 produces 8 uses energy source
30 reduces 6 hydrolyzes
21 oxidizes 6 degrades
14 exports 3 accumulates
12 imports 2 disproportionates
12 uses carbon source 3 (does not produce, is hydrolyzed to, transports)

Each needs its own replacement judgement — produces on a
BIOLOGICAL_PROCESS subject is not obviously enables — so rewriting them
here would have been 366 edges of unreviewed ontology guesswork. Filed as a
follow-up issue instead; this PR stays scoped to the pair #295 named.

Verification

  • just qc — exit 0, 477/477 files validate, 0 ERROR rows, derived reports current
  • uv run pytest — 379 passed
  • 3 ELLIPTICAL_SNIPPET findings were re-keyed in conf/evidence_snippet_baseline.tsv,
    not re-baselined: the baseline key embeds subject->object, so reversing an edge
    changes the key. Same file, same code, byte-identical message; retargeted the 3
    rows individually rather than running --write-baseline, which would have
    re-frozen the whole corpus and could mask real regressions.

🤖 Generated with Claude Code


⚠️ Update after review — read before merging

Two independent reviews found that enables is not range-correct either.
data/raw/biolink-model.yaml:5099-5110 defines enables (exact_mappings: [RO:0002327]) with range: biological process or activity. A TRAIT is a
disposition, not a process, so <chemical> enables <trait> swaps the METPO
domain violation for an RO range violation. Verified independently; the claim
holds.

This is pre-existing, not introduced here. main already had 149
enables edges with TRAIT objects; this PR takes it to 164. Gating the
enables row honestly would block all 164 and fail just qc, so the range
question is filed rather than solved here:

What changed in response: the playbook no longer prescribes
<chemical> enables <trait> as the general remedy for the 66-predicate family
— that block was going to be the reference for #301's 366 edges and the #183
backfill. It now carries the range caveat and recommends pointing the chemical
at the graph's process node, which is range-correct today. Also added:
curation_history events on all 14 files (a correcting event on DIR that
supersedes #294's now-inaccurate entry), a sentinel/enum collision guard with a
test, and canonical sort order restored to the baseline.

Merge judgement: the 15 edges move off a taxonomy-crossing entailment
("trait is a microbe") onto an established corpus convention that is wrong in a
narrower way, so #295's specific defect is gone. That is a real but partial
improvement. If #302 decides these belong on a process node or a new METPO
relation, these 15 get rewritten again — worth deciding before merging.

Note on the gate's reach: ground_edges_in_doc skips any edge that already has a non-empty predicate_id, so the NONE sentinel blocks newly written, ungrounded edges. A hand-written edge arriving with predicate_id: METPO:2000009 already filled in — the form all 15 of these were in — is not caught. It is a forward guard, not a retroactive one.

METPO:2000008/2000009 do not declare a domain directly, but both are
`rdfs:subPropertyOf METPO:2000001` ("organism interacts with chemical"),
whose `rdfs:domain` is METPO:1000525 (microbe). Domain is an inference
rule, not a constraint, so the 15 corpus edges asserting
`<trait> uses electron donor|acceptor <chemical>` entailed that each
TRAIT node IS a microbe. `predicate_id` is an unbound string in the
schema, so nothing in `just qc` saw it.

CausalNodeTypeEnum has no organism member — causal-graph nodes are
deliberately taxon-agnostic — so no causal-graph edge can satisfy that
domain. Reversed all 15 to the idiom the corpus already uses for this
shape, `<chemical> enables <trait>` (RO:0002327, already 7x as
CHEMICAL->TRAIT, including `ferric_iron enables dir_trait` in the very
graph two of these edges sit in). Evidence blocks are untouched; the
donor/acceptor role stays in the node label, grounding, and description.

To stop the shape recurring, `subject_types` for both rows in
`mappings/predicate_grounding.tsv` is now `NONE`, a new sentinel meaning
"no node type satisfies this domain" — distinct from `*`/empty, which
mean "any". `ground_causal_predicates.py` refuses to ground such a label
onto any edge and reports it as `blocked_by_node_type`, mirroring how
RO:0002411 was handled in #236.

The OBJECT_PROPERTY records' `domain:` is unchanged and stays correct:
the organism-subject form is valid at the assertion site. Only the
causal-graph reuse was wrong.

This fixes the electron pair only. The same defect affects 366 edges
across 193 files and 14 predicate labels in the same subproperty family
(`produces` alone accounts for 249) — filed separately rather than
rewritten here, since each predicate needs its own replacement judgement.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@culturebot-reviewer

culturebot-reviewer Bot commented Aug 5, 2026

Copy link
Copy Markdown

Reviewing PR #300

  • Read repo conventions (README.md, docs/)
  • Review the 15 reversed edges in data/traits/**
  • Verify the NONE sentinel in predicate_grounding.tsv + ground_causal_predicates.py
  • Check evidence-snippet baseline re-keying
  • Check graph reachability / derived HTML consistency
  • Post verdict

View job run

The number was written before the issue was filed and guessed wrong.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@culturebot-reviewer

culturebot-reviewer Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 4m 48s —— View job


Review: PR #300

  • Read repo conventions (README.md, docs/)
  • Review the 15 reversed edges in data/traits/**
  • Verify the NONE sentinel in predicate_grounding.tsv + ground_causal_predicates.py
  • Check evidence-snippet baseline re-keying
  • Check graph reachability / derived HTML consistency
  • Post verdict

The diagnosis is right and the subPropertyOf tracing is correct. The problem is the replacement.

🟡 RO:0002327 has a process range — CHEMICAL --enables--> TRAIT violates it

This repo's own vendored data/raw/biolink-model.yaml:5099-5110, which is what the mapping row cites as the cross-walk:

enables:
  description: holds between a physical entity and a process, where the physical entity executes the process
  domain: physical entity
  range: biological process or activity
  exact_mappings: [RO:0002327]

A TRAIT node is not a biological process or activity, and inorganic sulfide does not execute lithotrophy. So the 15 edges trade a domain violation for a range violation — the same shape as the RO:0002411 defect in #235 that the PR cites as its own precedent, and equally invisible to just qc (label-correspondence passes because the CURIE's label really is "enables"; the enables row at mappings/predicate_grounding.tsv:9 is */*, so the gate this PR just built is not applied to the predicate it moved 15 edges onto).

The clearest demonstration is inside a file the PR edits. data/traits/physiology/lithoautotrophic.yaml:109-127 now carries two edges with the same subject and predicate:

- subject: inorganic_electron_donor
  predicate: enables
  object: lithoautotrophic_trait          # TRAIT — range violation
- subject: inorganic_electron_donor
  predicate: enables
  object: respiratory_energy_conservation # BIOLOGICAL_PROCESS — correct

The range-correct edge is already there. Most of the 14 files have an equivalent process node (energy_metabolism, respiratory_energy_conservation, electron_transport_chain) that the donor could point at instead. Where the target really has to be the trait class, this repo already has a documented remedy: proposals/metpo_traitmech_v2/proposal.md:71 proposes manifests as precisely because "TraitMech's typical object is a TraitMech-internal trait class" and the upstream candidates' ranges were wrong. That reasoning applies here verbatim.

The 7 pre-existing CHEMICAL -> TRAIT enables edges are debt, not a warrant. Whichever way this lands, object_types on the enables row needs to say so, or the next PR repeats it.

🟡 The playbook now teaches the same range violation, at 366-edge scale

docs/CURATION_PLAYBOOK.md generalizes the pattern well past the electron pair:

- subject: glucose                     # CHEMICAL grounded to CHEBI:17234
  predicate: enables                   # RO:0002327
  object: <some_trait>

Glucose executes nothing. This block is what #301 (366 edges, 193 files) and the #183 backfill will be curated against, so a wrong idiom here costs an order of magnitude more than the 15 edges in this PR. Worth settling the relation before the doc blesses it.

🔵 The gate is one-sided

ground_edges_in_doc skips any edge that already has a non-empty predicate_id (scripts/ground_causal_predicates.py:153-155). The NONE sentinel therefore catches a newly written, ungrounded uses electron donor edge, but not a hand-written one that arrives with predicate_id: METPO:2000009 already filled in — which is the form all 15 of these were in. "A gate so it cannot recur" is stronger than what the mechanism does.

What checks out

  • NONE sentinel — parsed before the enum-name validation, so a typo still hard-fails; subj_ok is not None handles the falsy empty frozenset correctly (an if subj_ok: would have silently disabled it). Tests cover both.
  • Baseline re-keying — correct and minimal. _key drops the index and detail and compares occurrence counts per key, so the three re-keys preserve exactly the right totals (chemoorganoheterotrophic 2 → 1+1, photoorganoheterotrophic 2 → 1+1, organoheterotrophic 1 → 1). Not re-baselining was the right call.
  • Reachabilityaudit_causal_graphs.py:32-35 is deliberately undirected, so reversal is structurally neutral. The unchanged reports/causal_graph_audit.tsv is consistent with that.
  • Evidence — snippets untouched; the reworded descriptions still match what the snippets support.
  • Advisory only: no history/records/** entry for the 14 modified trait files.

Verdict

Requesting changes on the choice of replacement relation, not on the analysis. The METPO domain problem is real and worth fixing; enables just isn't the term that fixes it.

Fix this →
· branch

@culturebot-reviewer culturebot-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

RO:0002327 enables has range "biological process or activity" (data/raw/biolink-model.yaml:5099-5110, which the mapping row itself cites as the cross-walk). The 15 reversed edges point it at TRAIT nodes, so they trade the METPO domain violation for an RO range violation — the same shape as the RO:0002411 defect in #235 that this PR cites as precedent, and equally invisible to qc. lithoautotrophic.yaml:109-127 now carries the violating edge directly beside a range-correct one with the same subject and predicate (object respiratory_energy_conservation, a BIOLOGICAL_PROCESS). The playbook generalizes the idiom to 'glucose enables ', which is what #301's 366 edges and the #183 backfill will be curated against.

The diagnosis, the subPropertyOf tracing, the NONE sentinel (correctly parsed before enum validation; is not None handles the falsy empty frozenset), the baseline re-keying (counts preserved exactly; not re-baselining was right), and the undirected-reachability reasoning all check out. It is the choice of replacement relation that needs revisiting. Full detail in the review comment.

Two independent reviews converged on the same defect: RO:0002327 `enables`
has `range: biological process or activity` (data/raw/biolink-model.yaml:5099),
so pointing it at a TRAIT node trades the METPO domain violation for an RO
range violation. Verified — the claim is correct.

It is not, however, introduced here: 149 `enables` edges already had TRAIT
objects on main, and this PR took that to 164. Gating the `enables` row would
block all 164 and fail `just qc`, so the range question is filed as #302
(with #303 for the donor/acceptor distinction these 15 edges collapsed) rather
than solved in a PR about 15 edges.

What changes here is the over-claim:

- CURATION_PLAYBOOK.md no longer prescribes `<chemical> enables <trait>` as
  the general remedy for all 66 predicates in the family. That block would
  have been the reference for #301's 366 edges and the #183 backfill. It now
  states the range caveat, points at #302/#303, and recommends the
  range-correct form available today — chemical to the graph's *process* node.
- The `blocked_by_node_type` claim is future-tense: the reversal removed every
  edge carrying those labels, so no residual row exists to go looking for.

Also from review:

- 14 trait files get a REVERSE_CAUSAL_EDGE_DIRECTION curation_history event;
  they were rewritten with no provenance. The dissimilatory_iron_reduction
  event explicitly supersedes the #294 entry that still describes those two
  edges in their old direction and grounding — added as a correcting event
  rather than editing history in place.
- load_mapping now fails if CausalNodeTypeEnum ever defines NONE, which would
  let the sentinel silently shadow a real node type — the failure class the
  surrounding code deliberately makes fatal. Test added.
- conf/evidence_snippet_baseline.tsv restored to canonical _key sort order;
  it was re-keyed in place, which would have produced spurious diff noise on
  the next --write-baseline. Still exactly 3 rows changed vs main.

just qc exit 0; 380 tests pass.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@realmarcin

Copy link
Copy Markdown
Contributor Author

Reviewed and confirmed — the range finding is correct, and I verified it independently rather than taking it on faith. data/raw/biolink-model.yaml:5099-5110:

enables:
  domain: physical entity
  range: biological process or activity
  exact_mappings: [RO:0002327]

A TRAIT is a disposition, not a process, so <chemical> enables <trait> does carry a false type entailment. My PR body's framing was too confident.

Where I disagree: the scope of the remedy. This isn't debt the PR created or can pay off. On main there were already 149 enables edges with a TRAIT object; this PR takes that to 164. Setting honest object_types on the enables row — the fix suggested — would block all 164 and fail just qc immediately. So the choice was between solving a 164-edge corpus convention inside a PR scoped to 15 edges, or filing it. I filed it:

What I did change, because it was the part that actually scaled: the playbook no longer prescribes <chemical> enables <trait> as the general remedy for the 66-predicate family. That block was going to be the reference for #301's 366 edges and the #183 backfill, which is where a wrong idiom gets expensive. It now carries the range caveat, points at #302/#303, and recommends the form that is range-correct today — chemical to the graph's process node, as lithoautotrophic.yaml already does.

Also fixed from the review: curation_history events on all 14 files (they had none), with the DIR one explicitly superseding the #294 entry that still describes those edges in their old direction — added as a correcting event rather than editing history in place. Plus the sentinel/enum collision guard with a test, and the baseline restored to canonical sort order.

On merging as-is: the 15 edges move from "trait is a microbe" — which crosses into taxonomy — onto an established 149-edge corpus convention that is wrong in a narrower way. That's a real if partial improvement, and #295's specific entailment is gone. But it is a judgement call, and #302 may well decide these 15 should land somewhere else entirely, in which case they'd be rewritten again. Flagging that explicitly rather than merging past it.

@culturebot-reviewer

culturebot-reviewer Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 3m 20s —— View job


Re-review: PR #300 (after caad3904)

  • Diff the latest commit against my prior review
  • Check the reworked docs/CURATION_PLAYBOOK.md block
  • Verify the new curation_history events on the 14 trait files
  • Verify the sentinel/enum collision guard + test
  • Check the baseline sort-order restore
  • Post verdict

The substantive disagreement from round one is settled, and I'm not re-litigating it. You verified the range claim independently, filed #302/#303, and pulled the over-claim out of the doc that was going to be read by #301's 366 edges — that was the part that scaled, and it's fixed. docs/CURATION_PLAYBOOK.md:129-172 now leads with "do not assume enables" and shows the process-node form. Accepting the filing decision.

One thing didn't come along with it.

🟡 The mapping row still prescribes the idiom the playbook now warns off

mappings/predicate_grounding.tsv:5-6, notes column on both electron rows:

The corpus records these relations as <chemical> RO:0002327 <trait> instead

Written flat, as the answer. Added in the same commit as the playbook block that says the opposite:

What to use instead is not settled — do not assume enables. … Until #302 lands, prefer pointing the chemical at the graph's process node

A curator who hits uses electron donor and gets blocked_by_node_type reads the notes column — that's what it's for and where the block sends them. They will not necessarily then open the playbook. Right now the two places disagree, and the TSV is the one on the shorter path. Either make it descriptive rather than prescriptive ("the 15 edges in #295 were moved to <chemical> RO:0002327 <trait>; see #302 — this form is not settled") or point it at the process-node form the playbook recommends.

Same fix belongs on the enables row at line 9, whose notes read "common in causal-graph node→activity edges." 164 of its edges have a TRAIT object, not an activity — the row's own description of itself is the thing that made this easy to miss. A #302 pointer there costs a clause and lands where the next curator is looking. I'm not asking for object_types to be gated; you're right that it fails just qc on 164 edges and belongs to #302.

On the 15 edges

Not blocking on these. For the record, so #302 doesn't restart cold: 12 of the 14 files already have a BIOLOGICAL_PROCESS node (chemolithoautotrophic.yaml and organoheterotrophic.yaml don't), so the range-correct target mostly exists — but it isn't the mechanical repoint I implied last round. In lithoautotrophic.yaml:117-127 the donor→process edge is already there, so repointing would collapse two edges into one and drop the evidence block on the trait-targeted edge. That's a merge judgement per graph, not a rewrite. Your read was better than mine here.

What checks out in this commit

  • Sentinel collision guardscripts/ground_causal_predicates.py:112-119 runs before any row is parsed, so a schema that gains a NONE member fails at load rather than silently blocking every gated row. Test asserts on the message.
  • Provenance — all 14 files carry a REVERSE_CAUSAL_EDGE_DIRECTION event, and each one states the residual range caveat rather than presenting the new form as clean. The dissimilatory_iron_reduction.yaml:202-216 event names the 2026-08-05T00:00:00Z entry it supersedes instead of editing it — right call.
  • Baseline — back in canonical _key order (verified both hunks sort correctly), still exactly 3 rows vs. main.
  • "No corpus edge carries either label" — holds. The 20 remaining uses electron hits are prose, history, and respiration.yaml:107's distinct uses electron flow from.

Advisory, not blocking: ground_edges_in_doc still skips edges with a non-empty predicate_id (scripts/ground_causal_predicates.py:161-163), so the gate is forward-only. The playbook says so now ("forward guard"); the PR body still says "a gate so it cannot recur."

Verdict

Requesting changes on the mapping-row notes only — two clauses in one file. Everything else I raised is addressed.

Fix this →
· branch

@culturebot-reviewer culturebot-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Requesting changes on one item only; the round-one disagreement is settled.

mappings/predicate_grounding.tsv:5-6 notes still prescribe <chemical> RO:0002327 <trait> as what the corpus records — contradicting docs/CURATION_PLAYBOOK.md, added in the same commit, which now says "do not assume enables" and recommends the graph's process node until #302 lands. A curator who trips the blocked_by_node_type gate reads the notes column, not the playbook, so the wrong idiom is on the shorter path. Same clause belongs on the enables row at line 9, whose notes describe it as "common in causal-graph node→activity edges" when 164 of its edges have a TRAIT object. Not asking for object_types to be gated — that fails just qc on 164 edges and is #302's.

Everything else raised last round is addressed: the playbook no longer generalizes the idiom to the 66-predicate family, the sentinel/enum collision guard is correctly ordered before row parsing, all 14 files carry provenance (with the DIR event superseding rather than editing #294's), and the baseline is back in canonical sort order at exactly 3 changed rows.

Not blocking the 15 edges. For #302: 12 of the 14 files do have a BIOLOGICAL_PROCESS node, but repointing is not mechanical — lithoautotrophic.yaml:117-127 already has the donor→process edge, so it would collapse two edges and drop an evidence block. Your read on scope was better than mine.

The notes column is what a curator reads when the gate blocks them -
it is on a shorter path than the playbook, and it still prescribed
<chemical> RO:0002327 <trait> flatly while the playbook added in the
same commit said not to assume enables. Now descriptive: it records
what the 15 edges were moved to, marks the form unsettled, points at
 #302/#303, and recommends the process-node object meanwhile.

The enables row described itself as 'common in causal-graph node->activity
edges' while 164 of its edges have a TRAIT object - the self-description
is part of why this was easy to miss. It now states the biolink range,
says why object_types is not gated today, and points at #302.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@culturebot-reviewer

culturebot-reviewer Bot commented Aug 5, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 14m 45s —— View job


Re-review: PR #300 (after 592e4db7)

  • Diff 592e4db7 against my round-two review
  • Check the reworked notes column in mappings/predicate_grounding.tsv
  • Verify the numbers the notes now assert
  • Confirm nothing else regressed
  • Post verdict

The one item I was blocking on is fixed. mappings/predicate_grounding.tsv:5-6 no longer prescribes <chemical> RO:0002327 <trait> — it records what the 15 edges were moved to, says the form is not settled, names the range problem and the donor/acceptor collapse, points at #302/#303, and sends the curator to the process-node object. That's the version that agrees with the playbook, and it's on the path a blocked curator actually walks. Line 9 carries the same correction, including the sentence that matters most: "do not read the */* as license to add more TRAIT-object edges." Better than what I asked for — I'd have settled for a pointer.

Verified rather than assumed

The notes now assert numbers, so I counted them independently rather than trusting the PR body:

claim result
164 enables edges with a TRAIT object 164 on HEAD ✓
pre-existing, not introduced here 149 on origin/main — delta is exactly the 15 ✓
gate is a forward guard, no corpus edge carries either label 0 uses electron donor/acceptor edges remain ✓
12 of 14 files have a BIOLOGICAL_PROCESS node ✓ — chemolithoautotrophic.yaml and organoheterotrophic.yaml are the two without

TSV integrity: all 101 rows parse at 9 fields, so the long prose introduced no embedded tab. The notes column is read by csv.DictReader and consumed by nothing — check_biolink_coverage.py doesn't touch it and it isn't surfaced in generated HTML — so the angle brackets and #302 refs are inert rather than a rendering or comment-parsing hazard.

Two things for #302, not for this PR

Only 22 of the 164 have a CHEMICAL subject. The row's note frames the problem as range-only ("biolink gives enables domain 'physical entity' and range 'biological process or activity'"), but 142 of those 164 edges have a subject that is not a chemical and may not satisfy physical entity either. #302 is a domain question as well as a range one, and it's a larger surface than the electron-edge discussion has implied. Worth carrying into the issue before anyone scopes it as "repoint the chemicals."

The PR body still overstates the gate. It says "a gate so it cannot recur," but ground_edges_in_doc skips any edge arriving with a non-empty predicate_id (scripts/ground_causal_predicates.py:161-163) — which is the form all 15 of these were in. docs/CURATION_PLAYBOOK.md:136 says "forward guard" and is correct; the PR body is the one that's still ahead of the mechanism. Cosmetic, and squash-merge text is easy to fix at merge time.

Verdict

Approved. Everything I raised across both rounds is addressed, and the two items above belong to #302 and to the merge commit message respectively, not to this diff.

On the merge judgement you flagged: I agree it's a judgement call and I'd make the same one. The 15 edges move off a taxonomy-crossing entailment onto an established convention that is wrong more narrowly, and the corpus-scale documentation — the part #301's 366 edges and the #183 backfill get curated against — now tells the truth about that convention instead of blessing it. If #302 relocates these 15, it relocates them alongside 149 others, which is the right unit of work for that decision.

Disclosure: uv run pytest and just qc are blocked by this environment's permission layer, so those results are taken on your word. The only change since my last review is TSV prose, which cannot affect them.
· branch

@culturebot-reviewer culturebot-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Round-two blocker resolved. The notes column on both electron rows is now descriptive rather than prescriptive, marks the enables form unsettled, points at #302/#303, and recommends the process-node object; the enables row states the biolink range, the 164-edge count, and why object_types is not gated today.

Independently verified: 164 enables->TRAIT edges on HEAD vs 149 on origin/main (delta +15, matching the reversed edges); 0 remaining 'uses electron donor'/'uses electron acceptor' edges, so the playbook's forward-guard claim holds; 12 of 14 files have a BIOLOGICAL_PROCESS node and exactly one (lithoautotrophic.yaml) already carries the donor->process edge. All 101 TSV rows parse at 9 fields; the notes column is not consumed by code or surfaced in generated HTML, so the angle brackets and issue refs are inert.

Two non-blocking notes for #302, not for this PR: only 22 of the 164 enables->TRAIT edges have a CHEMICAL subject, so #302 is a domain question as well as a range one -- 142 of them have subjects that may not satisfy 'physical entity' either. And the PR body still describes the sentinel as 'a gate so it cannot recur' when ground_edges_in_doc skips any edge arriving with a non-empty predicate_id; the playbook says 'forward guard' correctly, the PR body does not.

Could not run pytest or just qc in this environment (permission layer), so the verification claims in the PR body are taken on the author's word; the only change since my last review is TSV prose.

@realmarcin
realmarcin merged commit d9fe1e7 into main Aug 5, 2026
8 checks passed
@realmarcin
realmarcin deleted the fix/295-electron-predicate-subject-domain branch August 5, 2026 19:08
realmarcin added a commit that referenced this pull request Aug 6, 2026
…review)

Review found the dissimilatory_iron_reduction prose factually wrong. All
three of DIR's Fe(III) species are terminal electron acceptors — the
file's own node descriptions say so ("Terminal electron acceptor reduced
in DIR") — so there is no donor-shaped edge in that graph to be confused
with. The real #303 loss is that after PR #300 reversed two of them onto
enables, nothing in predicate terms records the acceptor role at all.
Rewritten in both the observations cell and proposal.md.

Also from the review:

- Publish the explicit 164-edge partition as an appendix, so the buckets
  are checkable rather than inferrable from counts. This resolves the
  query about oxygen_preference: its "O2 as terminal electron acceptor"
  node is typed MOLECULAR_FUNCTION (it denotes the USE of O2, not O2), so
  it lands in confers, not under has electron acceptor whose definition
  requires a chemical species. Retyping it is corpus work, left to the
  migration PR.
- State that the declared OWL range is deliberately weaker than the
  definitions, and why tightening it was rejected.
- Fix this cohort's own Scope-A row, which repeated v2's now-stale "none
  in corpus" claim; there are 120, tracked in #319.

The reviewer's arithmetic finding does not hold: the published breakdown
has nine terms summing to 146 (81+32+12+5+5+4+4+2+1); the review's tally
omitted CHEMICAL 4 and reached 142. Verified against the corpus and left
unchanged. The substring-match observation is real but pre-existing and
is filed as #321.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
realmarcin added a commit that referenced this pull request Aug 6, 2026
…, #303) (#320)

* Propose the trait-realization and electron-role predicates (#302, #303)

Both defects share one root cause: the corpus has no range-correct
relation whose object is a trait. RO:0002327 (enables) has been doing
that job, but biolink gives it range 'biological process or activity',
and a TRAIT node is a disposition — so 164 edges entail
trait ⊑ BiologicalProcessOrActivity (#302). The obvious alternative is
worse: METPO's chemical-interaction predicates inherit rdfs:domain
microbe via METPO:2000001, which no causal-graph node can satisfy (#301).
That squeeze is what forced PR #300 to collapse donor and acceptor onto
one generic relation, losing the role for 7 of 15 edges (#303).

Cohort v8 proposes the three predicates that resolve it, following the
v1 convention of domain = range = METPO:1007401 (trait causal node),
which sidesteps the microbe domain and the process range at once:

  METPO:2007700  confers              146 edges / 114 records
  METPO:2007701  has electron donor    13 edges /  13 records
  METPO:2007702  has electron acceptor  5 edges /   3 records

The partition is exhaustive: 146 + 13 + 5 = 164, computed from the
corpus and cross-checked against ENABLES_RANGE_ON_TRAIT in
reports/predicate_domain_audit.tsv. Cross-ontology alignments go in
xrefs + a SSSOM file (skos:closeMatch to RO:0002327, METPO:2000009 and
METPO:2000008), never in definition_source (#83).

No corpus edges are grounded here; the 164-edge migration follows as a
separate PR, since #301 notes doing it all at once would be unreviewable.

Also fixes a pre-existing bug this cohort ran into (#318):
verify_metpo_proposal's Scope-A check ran even for cohorts shipping no
classes template, so class_tsv_text was "" and every corpus traitmech:
id read as uncited — failing every predicate-only cohort (v2, v4, v6).
The script already announced it would skip that case and check_scope_c
already honoured it. v1/v7 still fail, correctly: 120 synthetic ids are
genuinely un-lifted, tracked in #319.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Correct the DIR narrative and publish the explicit partition (PR #320 review)

Review found the dissimilatory_iron_reduction prose factually wrong. All
three of DIR's Fe(III) species are terminal electron acceptors — the
file's own node descriptions say so ("Terminal electron acceptor reduced
in DIR") — so there is no donor-shaped edge in that graph to be confused
with. The real #303 loss is that after PR #300 reversed two of them onto
enables, nothing in predicate terms records the acceptor role at all.
Rewritten in both the observations cell and proposal.md.

Also from the review:

- Publish the explicit 164-edge partition as an appendix, so the buckets
  are checkable rather than inferrable from counts. This resolves the
  query about oxygen_preference: its "O2 as terminal electron acceptor"
  node is typed MOLECULAR_FUNCTION (it denotes the USE of O2, not O2), so
  it lands in confers, not under has electron acceptor whose definition
  requires a chemical species. Retyping it is corpus work, left to the
  migration PR.
- State that the declared OWL range is deliberately weaker than the
  definitions, and why tightening it was rejected.
- Fix this cohort's own Scope-A row, which repeated v2's now-stale "none
  in corpus" claim; there are 120, tracked in #319.

The reviewer's arithmetic finding does not hold: the published breakdown
has nine terms summing to 146 (81+32+12+5+5+4+4+2+1); the review's tally
omitted CHEMICAL 4 and reached 142. Verified against the corpus and left
unchanged. The substring-match observation is real but pre-existing and
is filed as #321.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
realmarcin added a commit that referenced this pull request Aug 6, 2026
…323)

* Migrate 164 enables->TRAIT edges onto the v8 predicates (#302, #303)

RO:0002327 (enables) has range 'biological process or activity', and a
TRAIT node is a disposition, so all 164 of these edges entailed
trait ⊑ BiologicalProcessOrActivity. They now carry the predicates
proposed in proposals/metpo_traitmech_v8 (merged in #320):

  confers                METPO:2007700   146 edges, direction unchanged
  has electron donor     METPO:2007701    13 edges, direction reversed
  has electron acceptor  METPO:2007702     5 edges, direction reversed

The 18 electron edges are reversed back to <trait> -> <chemical>, which
restores the donor/acceptor role PR #300 had to collapse onto enables
(#303). The organism-subject problem that forced that collapse does not
arise here: these predicates take a causal-node domain, not
METPO:2000001's microbe domain (#301).

The three mapping rows are GATED rather than the '*'/'*' used by earlier
placeholder rows: confers is object_types=TRAIT with the 9 observed
subject types, and the electron pair is TRAIT -> CHEMICAL. That enforces
the "a chemical species" clause the declared OWL range deliberately does
not, and is why oxygen_preference's MOLECULAR_FUNCTION node ("O2 as
terminal electron acceptor", the USE of O2 rather than O2) grounds to
confers instead.

Applied by scripts/migrate_enables_trait_edges.py, which edits the raw
lines rather than round-tripping through write_validated_trait: that
helper is not byte-identical on hand-edited YAML (#322) and would have
buried 164 real changes in reflow churn across 128 files.

Baseline deltas, both intended and both verified:

- predicate_domain_audit: 530 -> 366. ENABLES_RANGE_ON_TRAIT is fully
  burned down, 164 -> 0. The remaining 366 are the untouched #301
  microbe-domain class.
- evidence_snippet: 6 rows re-keyed, count unchanged at 2737. Reversing
  an edge changes its baseline key (subject->object), so 6 pre-existing
  findings re-key; all 6 old keys are present in the previous baseline.
  No new snippet defects.

validate-strict: 477 files, 0 errors. pages/ regenerated.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Catch the O2 acceptor edge, and update the playbook (PR #323 review)

Two review findings, both valid.

manganese_oxidation described O2 as the "terminal oxidant", which names
the terminal-electron-acceptor role, but ACCEPTOR_RE matched only
`electron acceptor|terminal electron` so the edge fell to the `confers`
catch-all. That is not false, just strictly weaker — precisely the loss
#303 exists to stop. Widened the rule to `oxidant` (and `reductant`, for
symmetry on the donor side) and reversed that edge onto METPO:2007702.
Verified against the pre-migration corpus that the widened rule
reclassifies exactly one edge and no others, so the partition moves
146/13/5 -> 145/13/6 and nothing else shifts. proposals/metpo_traitmech_v8
counts, appendix and observations cells updated to match.

docs/CURATION_PLAYBOOK.md still told curators the #302 decision "is not
settled", cited the now-zero 164-edge count, and gave a worked `enables`
example — the opposite of what this PR establishes, in the one place the
repo documents the convention. Rewritten to state the settled convention
with a table of the three predicates, the trait -> chemical direction of
the electron pair, the node-type gating, and the placeholder-id caveat.

Also closed the loop the proposal had deferred TO this PR: the
oxygen_preference MOLECULAR_FUNCTION node keeps its type and grounds to
`confers`, and the mapping gate (object_types=CHEMICAL) stops it drifting
onto the electron pair later. Recorded in the proposal's classification
notes rather than left dangling.

Two advisory findings filed rather than fixed: #324 (the script's
mid-loop abort can leave a repo-wide partial migration) and #325 (no
history/records entry for a 128-file change, and nothing enforces one).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Correct the five prose count sites left at 146/13/5 (PR #323 review)

The previous commit corrected the partition tables, the appendix, the
grand total and both ROBOT traits_addressed cells, but not the prose
around them — so proposal.md asserted two different partitions about 60
lines apart. Fixed:

  proposal.md:93    114 of these 146 subjects        -> 145
  proposal.md:99    The 18 electron edges            -> 19  (13 + 6)
  proposal.md:187   Edge partition (146 + 13 + 5)    -> (145 + 13 + 6)
  proposal.md:288   Repoint the 146 ... reverse 18   -> 145 ... 19
  predicate_grounding.tsv:102  across the 146 migrated edges -> 145

Line 288 mattered most: it is the adoption plan someone follows
literally when METPO mints the real ids, and it was wrong on both the
repoint count and the reversal count.

The subject_types pipe-list on the confers mapping row is deliberately
unchanged: CHEMICAL still has 3 edges, so all nine node types remain
observed. Line 254 keeps "146/13/5 to 145/13/6" because that sentence
records the correction itself.

Every remaining count claim cross-checked against the corpus:
confers 145, donor 13, acceptor 6, electron 19, total 164. PR
description synced too, since it becomes the squash commit message.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* Restore the confers edge to manganese_oxidation's own record (PR #323 review)

Hand-editing that file's curation event to describe the O2 reversal
overwrote the half the script had written: the record claimed "Migrated 1
causal edge(s) ... (1 to has electron acceptor)" while the file actually
migrated two — multicopper_oxidase --confers--> the trait as well. A
128-file migration where one record disclaims half its own change is
exactly what misleads a later reader of the audit trail.

Rewritten to the form the script emits for the other mixed file
(dissimilatory_manganese_reduction), so the record now matches both the
file and what a re-run would reproduce, while keeping the explanation of
why the O2 edge was reversed.

Audited every MIGRATE_ENABLES_TRAIT_EDGES event in the corpus against its
own file rather than fixing only the reported one: 0 remaining
disagreements across all 128.

Also fixed the pre-existing numerator at proposal.md:93 (from #320): "114
of these 145 subjects" was the trait-RECORD count leaking into a sentence
about subjects. By the appendix it is 125 (81 BIOLOGICAL_PROCESS + 32
GENE_OR_PROTEIN + 12 ENVIRONMENTAL_FACTOR), now stated with the breakdown
so it cannot drift from the table again.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
realmarcin added a commit that referenced this pull request Aug 6, 2026
#340)

The corpus-as-of stamp is the maximum curation_history timestamp across
the whole corpus, inlined into every rendered page. Because the value is
global but stored per-page, ANY PR appending a curation event -- i.e.
every data PR, since the playbook requires one -- rewrote all 477 trait
pages. PR #300 changed 14 trait files and produced a 508-file diff of
which 477 were nothing but a footer timestamp; #339 in this same session
touched 495 files for one curation event.

Took the issue's option 1, storing the value once rather than 477 times,
but without dropping it from trait pages: they now carry their OWN
latest curation timestamp, labelled "Record as of". That changes only
when the record changes, and "when was THIS trait last curated" is the
more useful question on a trait page anyway. The corpus-wide stamp stays
on the aggregate pages, where it is a property of what is being shown.

Measured, not asserted. Appending one curation event to one record and
re-rendering:

  before: 477+ pages rewritten
  after:  13 -- the changed trait page, browse, graph, umap, and the 9
          category pages, all of which legitimately carry the corpus stamp

The changed trait page IS among the 13, checked explicitly; a fix that
stopped the real page changing would be worse than the churn.

pages/index.html carries no stamp either way -- its template does not
extend base.html. Pre-existing, verified against main, not touched here.

Three tests pin the distinction, including that a record's stamp is not
the corpus maximum, which is the property the whole change rests on.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

Chemical-use predicates are declared with an organism domain but used with TRAIT subjects

1 participant