Skip to content

Model the acetate kinase step instead of a chemical producing ATP (#331) - #339

Merged
realmarcin merged 2 commits into
mainfrom
fix/331-acetyl-phosphate-atp
Aug 6, 2026
Merged

Model the acetate kinase step instead of a chemical producing ATP (#331)#339
realmarcin merged 2 commits into
mainfrom
fix/331-acetyl-phosphate-atp

Conversation

@realmarcin

Copy link
Copy Markdown
Contributor

acetyl_phosphate --produces--> atp had a CHEMICAL subject on METPO:2007800, whose
definition is that the activity or state of the subject brings the object into
existence. A chemical has neither in that sense.

RO:0001001 (derives into) didn't fit either, and #327 already said why: ATP's matter
comes mostly from ADP
and only the phosphoryl group transfers, so it fails that
relation's inherits-the-significant-portion-of-the-matter test. That's exactly why it was
filed separately rather than swept into #327.

The answer was already in the record

The edge's own description says "The acetate kinase step generates ATP", and its evidence
says "ACK convert acetyl-phosphate to acetate, yielding ATP via SLP". The process was
named but never modelled.

Added it as a BIOLOGICAL_PROCESS node and split the edge:

acetate_kinase_step --has input--> acetyl_phosphate RO:0002233
acetate_kinase_step --has output--> atp RO:0002234

Both range-correct, and this is option 1 from the issue — the one that lets the gate tighten.

Wired on both sides deliberately: atp had exactly one edge, so replacing it without an
input edge would have orphaned the new node and atp together. audit-graphs confirms
no new findings (1535, unchanged).

The gate now matches its definition

This was the last CHEMICAL-subject produces edge, so METPO:2007800's
subject_types drops CHEMICAL — the tightening #331 predicted. Also added a has input
mapping row (RO:0002233), gated to the three activity node types for the same reason
has output is.

The one honest cost

Evidence-snippet baseline moves 2737 → 2738, and the delta is exactly explainable:

  • the pre-existing MISSING_SNIPPET re-keys onto the new has-output edge, and
  • one genuinely new finding, because the has-input edge reuses the same evidence, which
    carries notes but no verbatim snippet.

A snippet cannot be invented — the schema specifies a verbatim quote and the source text
isn't to hand — so it joins the known backlog (2586 such findings) rather than being
fabricated.

What was checked

just qc green · just validate-strict 477 / 0 errors · 431 tests pass ·
audit-graphs unchanged · zero CHEMICAL-subject produces edges remain · curation event
added with history verified in order.

Closes #331.

🤖 Generated with Claude Code

`acetyl_phosphate --produces--> atp` had a CHEMICAL subject on
METPO:2007800, whose definition is that the ACTIVITY OR STATE of the
subject brings the object into existence. A chemical has neither in that
sense. RO:0001001 (derives into) did not fit either, and #327 said why:
ATP's matter comes mostly from ADP and only the phosphoryl group
transfers, so it fails that relation's inherits-the-significant-portion-
of-the-matter test. That is why it was filed rather than swept into #327.

The resolution was already written in the edge's own description -- "The
acetate kinase step generates ATP" -- and in its evidence, which says ACK
converts acetyl phosphate to acetate yielding ATP. The process was named
but not modelled. Added it as a BIOLOGICAL_PROCESS node and split the
edge into `acetate_kinase_step has input acetyl_phosphate` (RO:0002233)
and `acetate_kinase_step has output atp` (RO:0002234), both range-correct.

Wired on BOTH sides deliberately: atp had exactly one edge, so replacing
it without an input edge would have orphaned the new node and atp
together. audit-graphs confirms no new findings.

This was the last CHEMICAL-subject `produces` edge, so METPO:2007800's
subject_types gate drops CHEMICAL and now matches its definition exactly.
Added a `has input` mapping row (RO:0002233), gated to the three activity
node types for the same reason `has output` is.

Evidence-snippet baseline moves 2737 -> 2738, and the delta is exactly
explainable: the pre-existing MISSING_SNIPPET re-keys onto the new
has-output edge, plus one genuinely new finding because the has-input
edge reuses the same evidence, which carries notes but no verbatim
snippet. A snippet cannot be invented -- the schema specifies a verbatim
quote and the source text is not to hand -- so it joins the known
backlog rather than being fabricated.

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

culturebot-reviewer Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 6m 1s —— View job


Review

The modelling call is right and the reasoning in the PR body holds up. Two things I checked that the body asserts, both confirmed independently:

  • Reachability is safe. _reachable() in scripts/audit_causal_graphs.py:95 is undirected, so acetate_kinase_step — acetyl_phosphate — acetyl_coa — wood_ljungdahl_pathway — acetogenesis_trait keeps both the new node and atp anchored. No new UNREACHABLE_FROM_TRAIT.
  • The gate claim is true. I enumerated the subjects of all 84 produces edges: every one is GENE_OR_PROTEIN, TRAIT, STATE or BIOLOGICAL_PROCESS. Dropping CHEMICAL from subject_types orphans nothing.

RO:0002233 / RO:0002234 are correct, the new mapping row is well-formed and mirrors the has output row exactly, the snippet-baseline delta (+2 rows, −1) matches the claimed 2737→2738, and no snippet was invented — the new edge carries notes only, which is the honest thing to do.

I could not run just qc or the test suite (uv run is blocked in this sandbox), so the green-CI claims are unverified by me.


🟡 data/traits/upper/biological_process.yaml:123 — the new mapping row covers this edge, but it was left ungrounded.

That edge is molecular_function --has input--> chemical_input with no predicate_id. Its sibling three lines below carries predicate_id: RO:0002234, and its own evidence note literally reads Has input [RO:0002233]. Before this PR the label was unmapped so the gap was inevitable; now it isn't — MOLECULAR_FUNCTION → CHEMICAL passes the new row's gate, which is exactly why the row disappeared from reports/predicate_grounding_residual.tsv.

Two consequences: the mapping table and the corpus disagree, and the next just ground-predicates --apply will silently modify a file this PR didn't touch. The precedent set by #327 is the other way — the derives into row landed with predicate_id: RO:0001001 written onto all four covered edges (disproportionation.yaml:175,184, cellulolysis.yaml:195, starch_degradation.yaml:90). Add predicate_id: RO:0002233 to that edge. Fix this →

🔵 acetate_kinase_step is label-only. mappings/node_grounding.tsv:59 already carries acetate kinase → GO:0008776 (acetate kinase activity), and typing the node MOLECULAR_FUNCTION instead would both ground it and match the GO-CAM shape data/traits/upper/biological_process.yaml uses for exactly this has input/has output pair. Against that: this trait's own research report (rendered at pages/traits/metabolism/acetogenesis.html:493) says to "prefer a process label plus explicit reaction edges" for the SLP step. Your call — I'd only ask that it be a decision rather than an omission.

🔵 The step has has output atp but not has output acetate, while acetyl_phosphate --is converted to--> acetate still runs alongside it. Half the reaction is on the process node and half on the chemical chain. Not wrong, just asymmetric.

No history/records/acetogenesis/… record — advisory only, and consistent with the two other curation events added to this file today, which have none either.


Verdict: requesting changes for the one 🟡. Everything else here is sound; that edge is a one-line addition.

  • Read repo conventions (README.md, docs/CURATION_PLAYBOOK.md, docs/GROUNDING_POLICY.md)
  • Review the substantive diff
  • Verify ontology grounding (RO:0002233/RO:0002234, GO:0008776, METPO:2007800 gate)
  • Check causal-graph structure & reachability
  • Check evidence integrity, snippet baseline, curation history
  • Post verdict
    · branch fix/331-acetyl-phosphate-atp

@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.

The modelling call is right and the reasoning holds up. Verified independently: reachability in audit_causal_graphs.py:95 is undirected so nothing is orphaned; all 84 produces edges have GENE_OR_PROTEIN/TRAIT/STATE/BIOLOGICAL_PROCESS subjects, so dropping CHEMICAL from the gate is safe; the RO CURIEs are correct and the snippet-baseline delta matches.

One blocker: the new has input mapping row now covers data/traits/upper/biological_process.yaml:123, but that edge was left without predicate_id: RO:0002233 while its sibling three lines below carries RO:0002234. The mapping table and the corpus disagree, and the next just ground-predicates --apply will silently touch a file this PR didn't. #327 set the opposite precedent — the derives into row landed with RO:0001001 written onto all four covered edges.

Details and two optional suggestions in the review comment.

The 🟡 is the same failure #328 had, and the reviewer is right that #327
set the opposite precedent. Adding the `has input` row made
upper/biological_process.yaml's `molecular_function --has input-->
chemical_input` edge LOOK mapped, so it dropped out of the residual
report, but nothing grounded it -- leaving it in neither the grounded set
nor the backlog, with the next `ground-predicates --apply` silently
touching a file this PR had not.

Grounded to RO:0002233, which its own evidence note already named
("Has input [RO:0002233]") and which its sibling has-output edge has
carried all along. Curation event appended by the tool.

Both 🔵s recorded as decisions in the curation event rather than left as
omissions, which is what the reviewer asked for:

- The node is typed BIOLOGICAL_PROCESS, not MOLECULAR_FUNCTION. The
  latter would have grounded to GO:0008776 via node_grounding.tsv, but
  this trait's own research report asks for a process label plus explicit
  reaction edges for the SLP step, and that is exactly what this is.
- The step carries has-output atp but deliberately not has-output
  acetate. The existing acetyl_phosphate is-converted-to acetate edge
  already states that half of the reaction on the chemical chain;
  asserting it again at the process level is the duplication at two
  levels of description that #303 criticised in
  dissimilatory_iron_reduction.

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

Copy link
Copy Markdown
Contributor Author

Fixed, and you're right that #327 set the opposite precedent — I should have followed it.

🟡 The ungrounded has input edge

This is the same failure #328 had: adding a mapping row made an existing edge look mapped, so it dropped out of reports/predicate_grounding_residual.tsv, but nothing grounded it — leaving it in neither the grounded set nor the backlog, with the next ground-predicates --apply silently touching a file this PR hadn't.

Grounded to RO:0002233, which its own evidence note already named ("Has input [RO:0002233]") and which its sibling has-output edge has carried all along. Curation event appended by the tool.

That's twice now that adding a mapping row has stranded a pre-existing edge — worth remembering as the standard follow-up step when a row lands, which is exactly what the #327 precedent encodes.

🔵 Node type — decided, not omitted

Fair ask. Kept as BIOLOGICAL_PROCESS, and the reason is now in the curation event: MOLECULAR_FUNCTION would indeed ground to GO:0008776 via node_grounding.tsv, but this trait's own research report asks for "a process label plus explicit reaction edges" for the SLP step — which is precisely what this is. Your pointer to that line is what settled it; I'd otherwise have taken the grounding.

🔵 Half-reaction asymmetry — also decided

Kept, with the reasoning recorded: the existing acetyl_phosphate --is converted to--> acetate edge already states that half of the reaction on the chemical chain. Adding has output acetate would assert the same fact twice at two levels of description — which is the specific thing #303 criticised in dissimilatory_iron_reduction. So the asymmetry is the lesser of the two.

history/records/

Still none, consistent with the other events on this file; the convention gap is #325.

Re-verified

just qc green · just validate-strict 477 / 0 · 431 tests pass · snippet baseline steady at 2738 with 0 new · audit-graphs unchanged.

@culturebot-reviewer

culturebot-reviewer Bot commented Aug 6, 2026

Copy link
Copy Markdown

Claude finished @realmarcin's task in 1m 38s —— View job


Re-review (commit 64716512)

The 🟡 is fixed correctly and completely.

  • data/traits/upper/biological_process.yaml:126 now carries predicate_id: RO:0002233, matching its sibling's RO:0002234 three lines down and the CURIE its own evidence note already named. RO:0002233 is has input — correct.
  • It's the only other has input edge in the corpus (the other is the new acetate_kinase_step → acetyl_phosphate, grounded at data/traits/metabolism/acetogenesis.yaml:226), so the new mapping row now has zero uncovered edges. The mapping table and corpus agree again, and ground-predicates --apply has nothing left to silently touch.
  • reports/predicate_grounding_residual.tsv drops the has input 1 unmapped line — the edge left the backlog by being grounded, which is the state that was missing before.

The derived artifacts are consistent with each other, not just individually plausible:

  • reports/node_grounding_residual.tsv gains acetate kinase step BIOLOGICAL_PROCESS — the honest consequence of keeping the node label-only rather than typing it MOLECULAR_FUNCTION. It's in the backlog rather than quietly absent.
  • conf/evidence_snippet_baseline.tsv is +2/−1 exactly as claimed: acetyl_phosphate->atp re-keys onto acetate_kinase_step->atp, plus the one new acetate_kinase_step->acetyl_phosphate row. Still no invented snippet — notes only.
  • biological_process.yaml curation history is chronological through the new 2026-08-06T09:03:55Z event.

Both 🔵s are now decisions with reasons recorded in the curation event rather than omissions, which is what I asked for. The BIOLOGICAL_PROCESS call cites the trait's own research report; the deliberate absence of has output acetate cites #303's duplication-across-levels criticism. I don't agree or disagree — they're defensible and, more importantly, they're now legible to whoever reads this file next.

One thing to note for accuracy: the acetogenesis curation event was amended in place rather than appended. That's fine here — it's the event this same unmerged PR created — but it does mean the recorded reasoning and the change landed in different commits.

As before, I could not run just qc, validate-strict, or the test suite (uv run is blocked in this sandbox), so the green-CI claims remain unverified by me. Everything I could check statically holds.

Verdict: approving.

  • Verify the 🟡 fix (RO:0002233 on biological_process.yaml:126)
  • Confirm no other edges the new mapping row leaves stranded
  • Check derived reports & baselines regenerated consistently
  • Check curation event records the two 🔵 decisions
  • Post verdict
    · branch fix/331-acetyl-phosphate-atp

@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.

The 🟡 is fixed correctly and completely.

data/traits/upper/biological_process.yaml:126 now carries predicate_id: RO:0002233, matching its sibling's RO:0002234 and the CURIE its own evidence note already named. It's the only other has input edge in the corpus, so the new mapping row has zero uncovered edges — the mapping table and corpus agree again, and ground-predicates --apply has nothing left to silently touch. The residual report drops the has input line by grounding rather than by disappearing.

Derived artifacts are consistent with each other: node_grounding_residual.tsv gains acetate kinase step (the honest cost of the label-only node, in the backlog rather than quietly absent), and the snippet baseline is +2/−1 exactly as claimed with no invented snippet.

Both 🔵s are now decisions with reasons in the curation event rather than omissions. I don't need to agree with either call — they're defensible and legible to the next reader, which was the ask.

Note for accuracy: the acetogenesis curation event was amended in place rather than appended. Fine here since it's this PR's own event, but the reasoning and the change landed in different commits.

I still could not run just qc, validate-strict, or the tests (uv run is blocked in this sandbox), so the green-CI claims are unverified by me. Everything checkable statically holds.

@realmarcin
realmarcin merged commit 46cab59 into main Aug 6, 2026
7 checks passed
@realmarcin
realmarcin deleted the fix/331-acetyl-phosphate-atp branch August 6, 2026 09:16
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.

acetyl_phosphate --produces--> ATP has a CHEMICAL subject that neither produces nor derives-into fits well

1 participant