Burn down the 11 disposition-typing findings — none of the eight were retypes (#352) - #360
Conversation
…retypes (#352) #353 shipped the detection and baselined what it found. This is the burn-down: DISPOSITION_MISTYPED 8 -> 0, DUPLICATE_GROUNDING 3 -> 0. ONLY FOUR OF THE EIGHT WERE MISTYPES. #352 framed the fix as "sweep CAPACITY nodes matching the disposition pattern and retype them", and for four nodes that is exactly right - salt tolerance, salt-tolerance breadth, oxygen tolerance and low-pH tolerance are dispositions, now TRAIT and grounded METPO:1000622 / 1000622 / 1000609 / 1003008. For the other four, retyping would have been wrong, and the thing that exposes it is the grounding. Every TRAIT node in the corpus is grounded, and the only correct grounding for each of those four is the term its OWN record already carries. Grounding them that way trades a DISPOSITION_MISTYPED for a DUPLICATE_GROUNDING and calls it progress. What it actually means is the node RESTATES its anchor: - ph_homeostasis_capacity "Capacity to balance and maintain cytoplasmic pH" against cytoplasmic_ph_homeostasis, IN THE SAME GRAPH, already BIOLOGICAL_PROCESS and grounded GO:0051453. - elongation_capacity "Capacity of a cell to elongate into a rod via sidewall growth" against lateral_elongation "Sidewall growth mode that lengthens rods" - both already carried `reduced in -> sphere_shaped_trait`. - loss_sporulation_capacity IS METPO:1000872, the record's own term. Collapsed to low_spo0a_activity -causes-> non_spore_forming_trait, which is the shape loss_sporulation_genes already uses in that graph. - growth_at_4c IS METPO:1000618, and it is a leaf hanging off the node grounded to it. That is #352's third bullet read strictly. It warns that an UNGROUNDED new TRAIT node becomes a free anchor, which suggests requiring a grounding fixes it. Requiring one does something better: it separates a real mistype from a restatement. CAPACITY IS NOT VESTIGIAL, which #352 left open. 24 nodes carry it; these 8 leave 16, and the survivors are a different sense - reducing_power (a pool of reductants), cytoplasmic_buffering_capacity (a reservoir), swimming_velocity (a rate), metabolic_versatility (a breadth). A buffer has a capacity; so does a battery; neither is something an organism can DO. Same two-senses shape `reduces` recorded in predicate_grounding.tsv, and it is why #353's heuristic is organism-scoped rather than matching bare "capacity to". THE 3 DUPLICATE_GROUNDINGS. growth_external_ph_5_5_9 shared METPO:1000478 with ph_delta_high_trait, but the two say different things: an ABSOLUTE external range (~5.5-9.0) versus a BREADTH (~5-9 pH UNITS), which is what a pH delta is. Regrounded to METPO:1000332 (pH range). catalase and urease shared their function node's GO term; GO:0004096 is catalase ACTIVITY, and a protein is not its activity - the graphs already say so correctly with catalase -enables-> catalase_function. Dropped from the proteins, kept on the functions. Note this is NOT a corpus-wide sweep: 72 GENE_OR_PROTEIN nodes are grounded to GO terms and that shorthand is fine where no separate function node exists; only these two graphs model both. NOT AN IMPROVEMENT IN CONNECTIVITY, and saying so before anyone asks. UNREACHABLE_FROM_TRAIT fell 1303 -> 1296, but FRAGMENTED_GRAPH is flat at 218 and the islands are intact. Every one of the seven is explained by a retype adding an anchor INSIDE an existing island - in oxygen_preference, superoxide_dismutase and two others stopped being reported because oxygen_tolerance, sitting in their island, became a TRAIT. Filed as #359: the metric moves for two different reasons and only one is progress, and this is the second time it has invited the wrong claim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two consequences of the merges that I regenerated the report too early to capture: `reduced in` drops 3 -> 2 residual because elongation_capacity's duplicate edge to sphere_shaped_trait was deduplicated into lateral_elongation's, and `has capability` disappears entirely because its only corpus edge was psychrotolerant_trait -> growth_at_4c, the leaf that restated its own parent. Both are the migration working as intended, not collateral: an ungrounded predicate with no remaining edges should stop appearing in a coverage report. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 5m 26s —— View job Review posted — requested changes
Two 🟡 findings, both grounding-sense mismatches in the retype half: 1. 2. Everything else verified clean: Not verified: I could not run |
![culturebot-reviewer[bot]](https://avatars.githubusercontent.com/u/233210009?s=60&v=4){kind=small}
There was a problem hiding this comment.
Two of the four retype groundings don't match what the node says. The rest of the change checks out — the merges, the two GO-activity drops, and the METPO:1000478 -> METPO:1000332 regrounding are all right, and I verified the reports are regenerated consistently.
🟡 salt_tolerance_breadth is grounded to a term that is not a breadth
data/traits/environment/nacl_delta_low.yaml:70 — METPO:1000622 is halotolerant: "A halophily preference in which an organism can tolerate high salt concentrations but does not require them for growth." The node says "Capacity to grow across a range of ambient NaCl concentrations" — a breadth, not a degree of tolerance. The graph then asserts salt_tolerance_breadth -is a (rdfs:subClassOf)-> nacl_delta (METPO:1000335), so the record now claims halotolerant ⊑ NaCl delta. In METPO, 1000622 is a subclass of 1000629 (halophily preference) and 1000335 of 1000532/1000534; that subsumption is not in the ontology.
This is the distinction the PR itself insists on for pH — an absolute range (1000332) is not a delta (1000478) — applied in the opposite direction here. METPO:1000622 is used twice in this PR, at nacl_delta_low.yaml:70 for a breadth and at slightly_halophilic.yaml:50 for a degree of tolerance. At most one of those can be right, and the audit can't see it because DUPLICATE_GROUNDING is scoped to a single graph.
The test applied was "does this CURIE collide with another grounding in this record" ("1000622 collides with neither"), which is not the same as "does this term mean what the node means." Run the restatement test instead and this node fails it: nacl_delta is in the same graph, already TRAIT, already grounded METPO:1000335, and described as "Breadth of the growth-supporting NaCl range" — the same claim. That is the ph_homeostasis_capacity / elongation_capacity pattern, so this looks like a fifth merge rather than a fourth retype.
🟡 oxygen_tolerance grounded to aerotolerant is the wrong sense
data/traits/environment/oxygen_preference.yaml:103 — METPO:1000609 is "An oxygen preference that does not use O₂ for growth but tolerates its presence," i.e. the aerotolerant-anaerobe phenotype. The node is generic: "Capacity of a cell to survive exposure to molecular oxygen," with in-edges detoxifying_enzyme_expression -increases-> and superoxide_dismutase -increases->. An obligate aerobe with high SOD has oxygen tolerance and is not METPO:1000609.
There is a structural tell as well. 1000609 is a subclass of 1000601, the record's own oxygen_preference_trait grounding — so the node is now a sixth child phenotype in a graph that enumerates the other four and links each with is a -> oxygen_preference_trait. This one carries no such edge, so the graph asserts a phenotype it does not place in the classification it belongs to. Either ground it to something that means generic O₂ survival, or treat it as a phenotype and wire it in like its siblings.
Verified, for the record
METPO:1003008acidotolerant,1000332pH range,1000618psychrotolerant,1000872non-spore forming,1000478pH delta 5–9 all resolve indata/raw/metpo.owlwith the labels the history entries claim.1000478was indeedph_delta_high.yaml's own record identifier, so that duplicate was real.- Dropping
GO:0004096/GO:0009039from the protein nodes while keeping them on the*_functionnodes is correct, and the stated scope (only the two graphs that model both) holds. - The three merges leave no dangling edges and no self-edges;
reports/causal_graph_audit.tsvhas 0DANGLING_EDGE, 0DISPOSITION_MISTYPED, 0DUPLICATE_GROUNDING, and is byte-identical toconf/causal_graph_audit_baseline.tsv. - The connectivity caveat is accurate:
FRAGMENTED_GRAPHis 218 in both trees andUNREACHABLE_FROM_TRAITis 1296. Most of the −7 is anchor-list rewording inoxygen_preference/ph_delta/slightly_halophilic; the one genuine gain isph_delta_lowgoing 5 components to 4 from the merge. Saying so up front was the right call. - History record present under
history/infrastructure/disposition-typing-burndown/, plus per-recordcuration_historyentries.
Two cosmetic leftovers if you're touching these anyway: the merged edges still describe the deleted nodes — ph_delta_low.yaml "determines pH homeostasis capacity" and sphere_shaped.yaml "removes the elongation capacity."
I could not run just qc or the test suite here (dependency install is blocked in this sandbox), so the green-CI claims are unverified from my side; everything above is from reading the corpus and the regenerated reports.
Review of the first burn-down pass (#360) found that 2 of the 4 nodes it retyped were restatements, caught by the SAME test the other restatements failed: the grounding chosen for each contradicted an edge or a definition the graph already had. salt_tolerance_breadth was retyped and grounded METPO:1000622 (halotolerant) while keeping its `is a -> nacl_delta` edge. nacl_delta is METPO:1000335, a delta (sub 1000532/1000534), whereas 1000622 is a halophily preference (sub 1000629) - so the graph asserted halotolerant sub NaCl-delta, a subsumption METPO does not have. That is the absolute-vs-breadth distinction this migration already insists on for pH, missed for salt. Merged into nacl_delta. oxygen_tolerance was grounded METPO:1000609 (aerotolerant), which METPO defines as "does NOT use O2 for growth but tolerates its presence" - false of the obligate aerobes the node also covers - and which is itself sub METPO:1000601, whose own definition is "oxygen requirements OR TOLERANCE for growth". So it was a sixth child phenotype in a graph that wires the other four in with `is a` and left this one unlinked. Merged into oxygen_preference_trait. The migration is now 2 retypes, 6 merges, 3 regroundings. THE MERGE IS A REAL CONNECTIVITY GAIN AND THE RETYPE WAS NOT, which is the distinction #359 is about. oxygen_preference FRAGMENTED_GRAPH goes components=3 of 14 (sizes 8, 4, 2) -> components=2 of 13 (sizes 11, 2): the ROS-defence island now attaches to the trait via detoxifying_enzyme_expression and superoxide_dismutase, instead of merely sitting beside a new anchor. The retype left that finding unchanged and the PR body said so. Note UNREACHABLE_FROM_TRAIT is 1296 either way - the count alone cannot tell the two apart, which is exactly #359's point. Also fixes the audit trail. The first pass logged all 11 events as RETYPE_CAUSAL_NODE, including the 7 merges and regroundings; an audit trail that calls a merge a retype cannot answer the question it exists to answer. The script now emits MERGE / DROP / REGROUND / UNGROUND_CAUSAL_NODE and appends the curation_history entry itself via record_curation_event rather than leaving it to be hand-written after the fact. Its timestamp is fixed rather than wall-clock because pages/ derives "Corpus as of" from the latest curation_history entry (#228). just qc green (audit-graphs and audit-snippets both 0 new) - 519 tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Claude encountered an error after 5m 21s —— View job Review posted — requested changes
The two findings from the last pass are properly fixed. But the correction was applied to two of four, and the same test kills the other two. Both surviving retypes have the identical shape you just rejected for 🟡
|
realmarcin
changed the title
There was a problem hiding this comment.
Both findings from the last pass are properly fixed, and the MERGE/DROP/REGROUND/UNGROUND action split is a real improvement to the audit trail. But the correction was applied to two of the four retypes, and the same sense test kills the other two.
salt_tolerance (slightly_halophilic.yaml:50) grounded METPO:1000622. The record is METPO:1000625, "requires low to moderate salt concentrations for optimal growth"; 1000622 is "can tolerate high salt but does not require them for growth". Direct siblings under 1000629, mutually exclusive by definition, so the graph for a salt-requiring organism now asserts the salt-not-required preference. Worse than the 1000609 case you just fixed, where the term was at least a subclass of the record's own. Node is a sink with no edge to slightly_halophilic_trait. The migration's justification -- "the record is METPO:1000625, so this is a distinct term rather than a restatement" -- is the distinctness test acd4a12 replaced.
low_ph_tolerance (ph_delta.yaml:77) grounded METPO:1003008. "Acidotolerant" is defined as tolerating acid while maintaining optimal growth near neutral pH; the node has no optimum clause and ph_delta is the generic pH-breadth record, so acidophiles are in scope and are not 1003008. Same shape: sink, one in-edge, unlinked to ph_delta_trait, and the retype is what removed two UNREACHABLE_FROM_TRAIT findings while FRAGMENTED_GRAPH stayed at components=3.
Minor: the playbook's CAPACITY table uses the salt_tolerance description as its canonical disposition-to-TRAIT example, so it moves with that node; and its "all anchor effect, no connectivity" line is now stale, since oxygen_preference went 3 to 2 components and ph_delta_low 5 to 4. The two edge descriptions naming deleted nodes are still there (ph_delta_low.yaml:113, sphere_shaped.yaml:133).
Everything else verified: no dangling or duplicate edges and no orphans in the touched graphs; the audit report is byte-identical to the baseline with 0 DISPOSITION_MISTYPED / 0 DUPLICATE_GROUNDING / 0 DANGLING_EDGE and the -7 breaks down as claimed; every CURIE and every cited subsumption resolves in metpo.owl; the two GO-activity drops and their stated scope are correct; the residual/coverage reports move consistently. I could not run just qc or the tests here (dependency install blocked), so the CI claims are unchecked from my side.
Full detail: #360 (comment)
|
Claude finished @realmarcin's task in 3m 50s —— View job Review posted — requested changes
The branch is still at 🟡
|
There was a problem hiding this comment.
No new commits since my last pass — the branch is still at acd4a12, and both 🟡 findings are unchanged in the tree. Restating them briefly, plus one thing I got wrong last time about how to fix them.
salt_tolerance (slightly_halophilic.yaml:50) grounded METPO:1000622 and low_ph_tolerance (ph_delta.yaml:77) grounded METPO:1003008. Both verified again against data/raw/metpo.owl: 1000622 is "can tolerate high salt concentrations but does not require them for growth", a direct sibling of the record's own 1000625 ("requires low to moderate salt concentrations for optimal growth") under 1000629; 1003008 is "tolerate acidic environments ... while maintaining optimal growth near neutral pH", which excludes the acidophiles the generic ph_delta record covers. Both nodes appear only as edge objects — sinks, with no edge to the record's trait node.
Correction to my own advice: I suggested "reground to a term that means generic salt / low-pH survival". METPO does not have one. acidotolerant and halotolerant are the only matching labels in the file, and each already has its own record (acidotolerant.yaml, halotolerant.yaml) grounding its trait node to that term. So the real options are (a) merge into slightly_halophilic_trait / ph_delta_trait the way oxygen_tolerance was merged in acd4a12, or (b) mint a traitmech: ID per manage-identifiers and file a METPO proposal. Not a reground.
🔵 Still open from last pass: the playbook's CAPACITY table (docs/CURATION_PLAYBOOK.md:158) uses the salt_tolerance description as its canonical example, so it moves with that node; its "all anchor effect, no connectivity" line is stale after acd4a12 (oxygen_preference 3→2 components, ph_delta_low 5→4); and ph_delta_low.yaml:115 / sphere_shaped.yaml:133 still describe the deleted nodes. The PR body also still describes four retypes.
Re-verified clean: reports/causal_graph_audit.tsv is byte-identical to conf/causal_graph_audit_baseline.tsv, 218 FRAGMENTED_GRAPH + 1296 UNREACHABLE_FROM_TRAIT and nothing else; the six merges/drops leave no residual references outside history text; the carboxydotrophic.yaml oxygen_tolerance node is enzyme-scoped QUALITY and correctly not swept, which supports the organism-scoped heuristic. I still cannot run just qc or the tests here — dependency install is blocked — so the CI claims are unchecked from my side.
The first pass counted a declined node into `residual` on the reasoning that the report describes what the corpus looks like and an ungrounded node belongs there however it got that way. Review showed that is the wrong model of the file: its consumers treat it as a WORK QUEUE, not a census. match_uniprot_to_proteins.py's load_target_labels() takes every GENE_OR_PROTEIN row from reports/node_grounding_residual.tsv and, under --apply, appends a UniProtKB row to mappings/node_grounding.tsv with NO existing-row check. So once #360 lands, `catalase` would appear in the residual TSV, earn a second mapping row conflicting with its GO:0004096 one, and load_mapping() raises on exactly that shape - taking out `just ground-nodes` and the derived-reports freshness check with it. .claude/commands/ground-or-propose-metpo.md reads the same file the same way. The semantics settle it independently of the breakage: a declined node is NOT awaiting a grounding. It has one, deliberately withheld. Proposing a UniProt accession for it would be actively wrong even if nothing downstream broke. So declined nodes are reported only through the `declined` counter, which is what that counter is for. This also means the residual TSV is byte-identical to before in every case, so no consumer is perturbed at all. Verified on #360's corpus state: catalase and urease are declined and absent from the residual TSV's GENE_OR_PROTEIN rows - only `oxyr`, a genuinely unmapped protein, remains a target for the matcher. 524 tests pass - just qc green - ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
) Third review round on #360. Both remaining retypes are mis-grounded, so the migration is now 0 retypes, 8 merges, 3 regroundings. salt_tolerance was grounded METPO:1000622 (halotolerant) on the reasoning that the record's own METPO:1000625 (slightly halophilic) makes the term "distinct". Verified against data/raw/metpo.owl: 1000622 and 1000625 are DIRECT SIBLINGS under 1000629, and 1000622 means "tolerates high salt but DOES NOT REQUIRE it for growth" while 1000625 means the organism "REQUIRES low to moderate salt for optimal growth". The node asserted of this record the negation of what the record's own term says. Merged into slightly_halophilic_trait. low_ph_tolerance was grounded METPO:1003008 (acidotolerant), defined as tolerating acid "WHILE MAINTAINING OPTIMAL GROWTH NEAR NEUTRAL pH" — which excludes the acidophiles this generic pH-delta record covers. A grounding narrower than the node it labels is a false claim about every organism in the excluded part. Merged into ph_delta_trait. THE TEST THAT FAILED FOUR TIMES was "is this term distinct from the record's own?". A sibling term is maximally distinct and still wrong. The right question is whether the term is COMPATIBLE with the record and no NARROWER than the node. Written into the playbook, because the wrong test is the intuitive one. CORRECTING AN OVERCLAIM OF MY OWN in the same pass: I first wrote that merging salt_tolerance attaches the ectoine island. Measurement says otherwise — the node was already in the trait's component via osmoprotectant_transport -> compatible_solutes -> osmotic_stress, so that merge changes no structure. It is a correctness fix, and saying more would be this migration committing the exact error it was written to catch. MEASURED, NOT ASSERTED, across all eight: retyping changed component structure in ZERO of the eight graphs merging improves three — oxygen_preference 3 components -> 2, ph_delta 3 -> 2, ph_delta_low 5 -> 4 and leaves five unchanged as pure deduplication UNREACHABLE_FROM_TRAIT reads 1296 under either fix. That is #359's point, and the playbook now points at reports/causal_graph_connectivity.tsv rather than the finding counts. Playbook also updated because its canonical "disposition" example WAS salt_tolerance's own description, which this pass merges away. RETYPE kept as an empty table rather than deleted: "we looked and there were none" and "we never modelled retypes" are different claims, and only one is true. 519 tests pass - ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
realmarcin
changed the title
|
Claude finished @realmarcin's task in 5m 23s —— View job Review posted — requested changes
Both 🟡 findings are fixed, and fixed the right way. The data is now correct. What is left is two claims in the diff that aren't. 🟡 The playbook's
|
|
Both 🟡 fixed in
I took merge, per your option (a) — Your review made me catch an error of my own. Writing up the salt merge I claimed it attaches the ectoine island. It doesn't — measuring, Three notes on the 🔵s:
Noted that you can't run |
There was a problem hiding this comment.
The two findings from the last pass are fixed, and fixed the right way — salt_tolerance merged into slightly_halophilic_trait, low_ph_tolerance into ph_delta_trait, edges repointed, no dangling or duplicate triples, no orphans, and both curation_history rationales check out against metpo.owl. Every connectivity number in the PR body verifies against origin/main's report, including the self-correction: slight_halophile_low_salt_osmoadaptation goes 2 of 11 to 2 of 10, so that merge really is deduplication with no structural effect. The empty-but-kept RETYPE table is the right call.
The data is now correct. What's left is two claims in the diff that aren't.
docs/CURATION_PLAYBOOK.md:161 — both TRAIT examples are nodes this PR deleted. Last round flagged that the row used salt_tolerance's description; the fix swapped it for oxygen_tolerance's ("Capacity of a cell to survive exposure to molecular oxygen"), which acd4a12 merged away one commit earlier. The other example, "Ability to grow at 4 C", is growth_at_4c, dropped in f60cb99e. Neither string survives in data/traits/** outside the history prose describing its own removal. So the table tells a curator this sense becomes TRAIT using two nodes that didn't, while line 174 thirteen lines below says "#352 retyped nothing in the end; all eight were merged". The honest row is probably not "-> TRAIT" at all but the decision procedure: ground it, and if the only available term restates, contradicts, or narrows, merge.
"filed as a proposal" — nothing was filed. slightly_halophilic.yaml:287 and verbatim at scripts/migrate_disposition_typing.py:118: "METPO has no generic salt-tolerance disposition to reground to -- filed as a proposal." First half is right and I re-checked it. Second half isn't: this PR touches no files under proposals/, none of v1-v10 covers a generic salt-tolerance disposition (the closest, METPO:1007720 salt-in strategy in v7, is an osmoadaptation process), and no issue is cited in the history record. It matters more than its size because it sits in curation_history — the field whose only job is to be true — in the pass that rewrote the audit trail so a merge would stop being logged as a retype. Drop the clause or file the proposal and cite it.
Minor: CURATION_PLAYBOOK.md:193 points at reports/causal_graph_connectivity.tsv in the present tense and says "quote that table", but the file doesn't exist on this branch or on main — it arrives with #363. And ph_delta_low.yaml:115 / sphere_shaped.yaml:133 still name the deleted nodes, though both read fine as English.
Verified clean: no dangling edges, self-edges, duplicate triples or orphans in the touched graphs, and the repointed edges collide with nothing pre-existing; reports/causal_graph_audit.tsv byte-identical to the baseline at 218 FRAGMENTED_GRAPH + 1296 UNREACHABLE_FROM_TRAIT and nothing else, with the -7 breaking down as claimed against main's 8/3/218/1303; METPO:1000332 is "pH range" and the right home for an absolute range; evidence_snippet_baseline.tsv moves with exactly the three repointed edges; residual references to all eight removed nodes appear only in history prose, and carboxydotrophic.yaml's enzyme-scoped oxygen_tolerance is correctly untouched.
I could not run just qc, linkml-validate or the tests here — dependency install is blocked — so the CI and 519-tests claims are unchecked from my side.
Full detail: #360 (comment)
Both are claims, not data — but one of them sits in curation_history, whose only job is to be true, in the pass that rewrote the audit trail so a merge would stop being logged as a retype. "filed as a proposal" was false. Nothing under proposals/ covers a generic salt-tolerance disposition and this PR adds nothing there. The first half of the sentence is right and stays: METPO genuinely has no term to reground to, since halotolerant and acidotolerant are the only candidate labels and each already anchors its own record. Now cites issue #364, which proposes a tolerance axis distinct from the preference axis, and says plainly that nothing under proposals/ exists yet. The playbook's CAPACITY table listed two TRAIT examples that are both nodes THIS PR DELETED. Last round caught the row using salt_tolerance's description; the fix swapped in oxygen_tolerance's, which acd4a12 merged away one commit earlier, alongside "Ability to grow at 4 C" which is growth_at_4c, dropped in f60cb99. So the table told a curator "this sense becomes TRAIT" using nodes that did not become TRAIT, thirteen lines above a sentence saying #352 retyped nothing. Rewritten so the column says what to DO rather than what to type: a disposition reading makes a node a CANDIDATE, and the grounding step decides. The examples are kept and labelled as what they are — all three merged or dropped, none survived — which is a better lesson than a list of nodes that did become traits, since there are none. Also stopped citing reports/causal_graph_connectivity.tsv in the present tense: it arrives with #363, not on this branch. The hand-measured numbers stay, attributed as hand-measured. 519 tests pass - ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All three fixed. Both of the substantive ones were fair, and the playbook one was bad in a way worth naming. The Taking your suggestion: the column now says what to do rather than what to type. A disposition reading makes a node a candidate; the grounding step decides. I kept the examples and labelled them as what they are — all three merged or dropped, none survived — because that's a better lesson than a list of nodes that did become traits, of which there are zero. "filed as a proposal". Also right, and the worse of the two because it sits in
Leaving Thanks for re-deriving the connectivity numbers against
|
|
Claude finished @realmarcin's task in 3m 15s —— View job Review posted — approved
Both 🟡 findings are fixed, and the fixes are checkable rather than assertions. "filed as a proposal" is gone from all three places it lived — The
🔵 Three small things, all optional
Verified
The retype-count arc — 4 → 2 → 0, with the migration keeping an empty Not verified: I could not run |
There was a problem hiding this comment.
Both findings from the last pass are fixed, and fixed checkably.
"filed as a proposal" is gone from all three places it lived (slightly_halophilic.yaml:288, migrate_disposition_typing.py:126, and the rendered page). What replaced it is the part that is true — METPO has no generic salt-tolerance disposition, halotolerant and acidotolerant each already anchor their own record, and "nothing under proposals/ yet", which I re-checked: the branch touches no proposals/ file and no proposal in the tree mentions salt tolerance in any casing.
The CAPACITY table no longer prescribes a type. The column reads "try to ground it — then see below", and the row is followed by an explicit note that none of its three examples still exists. I verified that: "survive exposure to molecular oxygen", "Ability to grow at 4 C" and "grow and survive under elevated salinity" appear nowhere in data/traits/** outside the curation_history prose describing their own removal. Keeping the examples and labelling them as the ones that did not survive is a better row than the one it replaces. The section no longer contradicts line 180. And causal_graph_connectivity.tsv is now attributed as hand-measured and arriving with #363.
Three optional things. CURATION_PLAYBOOK.md:180 says "all eight were merged" and :158 "#352 merged 8", but seven were merged — growth_at_4c was dropped (migrate_disposition_typing.py:195, "drop": True), which the PR body's own table says and the new paragraph thirteen lines above gets right with "merged or dropped". The 060908Z history record (:42) says the playbook "now points at reports/causal_graph_connectivity.tsv", which this commit reversed, and no history record was added for the pass despite it touching data/traits/** — advisory only. And I could not reach the GitHub API from this sandbox, so I did not confirm #364 exists or proposes what the curation_history clause says.
Verified: reports/causal_graph_audit.tsv is still byte-identical to conf/causal_graph_audit_baseline.tsv, and this commit touches no report, baseline, or causal_graphs block, so the structural verification from my 97bf998 pass carries over — 218 FRAGMENTED_GRAPH + 1296 UNREACHABLE_FROM_TRAIT and nothing else. The corpus has exactly 16 node_type: CAPACITY nodes across 13 files, the number the playbook claims survives, and none of the four named survivors was touched. The only data change is curation_history prose, the one page that moves with it is the right one, and the fixed 05:00:00Z timestamp is unchanged so nothing re-derives the "Corpus as of" date (#228).
The retype-count arc 4 -> 2 -> 0, with an empty RETYPE table kept and a comment explaining why it is not deleted, is a more useful artifact than the sweep #352 originally asked for.
I could not run just qc, linkml-validate or the tests here — dependency install is blocked — so the CI and 519-tests claims are unchecked from my side.
Full detail: #360 (comment)
* Decline a node grounding the graph already carries (#361) ground_causal_nodes.py wrote any mapped CURIE into any ungrounded node, including one that another node in the SAME graph already carried - which is exactly the shape audit-graphs reports as DUPLICATE_GROUNDING. The writer manufactured findings the auditor then reported against us. Concretely: #352 removed GO:0004096 from the `catalase` node and GO:0009039 from `urease`, on the argument that a protein is not its activity and the graphs already say so with `catalase -enables-> catalase_function`. But mappings/node_grounding.tsv still maps both proteins to those GO ACTIVITY terms, so the next `just ground-nodes --apply` re-created both duplicates. Measured on that branch's corpus: files modifiable 2, nodes grounded 2 - exactly the two that had just been ungrounded. DELETING THE MAPPING ROWS WOULD OVER-CORRECT. #352's own reasoning says the protein -> GO-activity shorthand is fine wherever the graph does not also model the function as its own node, and 72 GENE_OR_PROTEIN nodes rely on it. The row is not the defect; writing it into a graph that already says the same thing is. So the guard belongs in the writer, not the mapping table. ground_nodes_in_doc now tracks the CURIEs present in each graph - seeded from existing groundings and updated as it writes, so two ungrounded nodes mapping to one CURIE cannot both take it - and declines a candidate that collides. Declined nodes stay ungrounded, so they are counted into `residual` too: that report describes what the corpus actually looks like, and an ungrounded node belongs there however it got that way. The summary prints what was withheld and why rather than silently skipping it. Scoped per graph, not per record, because DUPLICATE_GROUNDING is a within-graph defect - the same CURIE in a different graph of the same record is not a duplicate and is still written. Verified against the fix/352 branch state: grounded 2 -> 0, declined 2. On main the corpus is unaffected: 0 modifiable, residual TSV byte-identical. ground_causal_predicates.py has NO analogous gap - nothing flags a duplicate predicate_id and nothing should, since many edges legitimately share one. 523 tests pass (4 new) - just qc green - ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Keep declined nodes out of the residual TSV (#362 review) The first pass counted a declined node into `residual` on the reasoning that the report describes what the corpus looks like and an ungrounded node belongs there however it got that way. Review showed that is the wrong model of the file: its consumers treat it as a WORK QUEUE, not a census. match_uniprot_to_proteins.py's load_target_labels() takes every GENE_OR_PROTEIN row from reports/node_grounding_residual.tsv and, under --apply, appends a UniProtKB row to mappings/node_grounding.tsv with NO existing-row check. So once #360 lands, `catalase` would appear in the residual TSV, earn a second mapping row conflicting with its GO:0004096 one, and load_mapping() raises on exactly that shape - taking out `just ground-nodes` and the derived-reports freshness check with it. .claude/commands/ground-or-propose-metpo.md reads the same file the same way. The semantics settle it independently of the breakage: a declined node is NOT awaiting a grounding. It has one, deliberately withheld. Proposing a UniProt accession for it would be actively wrong even if nothing downstream broke. So declined nodes are reported only through the `declined` counter, which is what that counter is for. This also means the residual TSV is byte-identical to before in every case, so no consumer is perturbed at all. Verified on #360's corpus state: catalase and urease are declined and absent from the residual TSV's GENE_OR_PROTEIN rows - only `oxyr`, a genuinely unmapped protein, remains a target for the matcher. 524 tests pass - just qc green - ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Correct the history record to match the shipped behaviour (#362 review) The record was written against the first pass and still said "declined nodes stay ungrounded so they are counted into residual as well" - the exact behaviour ebfbed2 removed, and the one that would re-arm the mapping-conflict break if someone acted on it. It is the only committed explanation of why the code is shaped this way, so it saying the opposite of the code is worse than it saying nothing. Rewritten to state what ships and why: declined nodes are reported only via the `declined` counter and kept OUT of residual, because the residual TSV's consumers treat it as a work queue rather than a census. Test tally corrected 523/4-new -> 524/5-new. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#360 landed eight merges, so the committed report — generated before it — is stale in exactly eight rows, and audit-derived-reports (the staleness check this PR adds) would have failed on main the moment both were in. Corpus totals: 864 -> 861 components over 4137 -> 4129 wired nodes, attached 69.5% -> 69.7%. Worth recording what the regenerated table says, because it reproduces #360's hand-measured claims from the tool rather than from me: oxygen_preference 14/3/8 -> 13/2/11 structural gain ph_delta 12/3/8 -> 11/2/9 structural gain ph_delta_low 12/5/3 -> 11/4/4 structural gain nacl_delta_low 9/1/9 -> 8/1/8 deduplication psychrotolerant 12/3/8 -> 11/3/7 deduplication slightly_halophilic 11/2/9 -> 10/2/8 deduplication non_spore_forming 12/1/12 -> 11/1/11 deduplication sphere_shaped 11/1/11 -> 10/1/10 deduplication Three of eight moved the component count; five only removed a node. That is the split #360 argued for, and UNREACHABLE_FROM_TRAIT reports 1296 across all of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…363) * Measure component structure, the number retyping cannot move (#359) #359 observed that UNREACHABLE_FROM_TRAIT falls when a node is retyped into a TRAIT anchor INSIDE an existing island, without the graph becoming any more connected. Neither headline count can tell that apart from a real gain: FRAGMENTED_GRAPH reports one finding per split graph however many pieces it is in, so 3 components -> 2 does not move it either. Adds connectivity_rows(), emitting reports/causal_graph_connectivity.tsv with one row per graph — wired_nodes, components, largest_component, sizes — plus a corpus summary line on the audit. On #352's worked example, oxygen_preference.yaml: main (pre-#352) wired=14 components=3 largest= 8 attached=57.1% RETYPED (wrong) wired=14 components=3 largest= 8 attached=57.1% MERGED (right) wired=13 components=2 largest=11 attached=84.6% The retype is bit-for-bit indistinguishable from doing nothing, which is correct — it connected nothing. UNREACHABLE_FROM_TRAIT is 1296 for BOTH fixes. That is the blindness #359 describes, now measurable. Corpus baseline: 353 graphs, 864 components over 4137 wired nodes, attached 69.5%. The metric is anchor-free for the same reason _components is: it asks "is this one graph?" without needing to know which node the record is about, so no amount of retyping or renaming moves it. Scoped to edge-referenced nodes to match FRAGMENTED_GRAPH — an unwired node is ORPHAN_NODE's business, and counting it here would let one defect depress two metrics. Topology extraction is now a shared _topology() helper used by BOTH audit() and the metric, so the ratchet and the measurement cannot drift on what "connected" means. Verified behaviour-preserving: regenerated causal_graph_audit.tsv is byte-identical to the committed copy. IT CARRIES NO RATCHET OF ITS OWN. It is a measurement, not a verdict, and never affects the exit code — the point of #359 is that a number which cannot be gamed by retyping is worth having even when nothing gates on it. Wired into audit-derived-reports for staleness, passing --connectivity-out to the temp dir: omitting it would have made a staleness check write the very file it was judging, the failure that recipe's own header warns about. 524 tests pass (5 new) - just qc green - ruff clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * Regenerate the connectivity report for #360's merged graphs #360 landed eight merges, so the committed report — generated before it — is stale in exactly eight rows, and audit-derived-reports (the staleness check this PR adds) would have failed on main the moment both were in. Corpus totals: 864 -> 861 components over 4137 -> 4129 wired nodes, attached 69.5% -> 69.7%. Worth recording what the regenerated table says, because it reproduces #360's hand-measured claims from the tool rather than from me: oxygen_preference 14/3/8 -> 13/2/11 structural gain ph_delta 12/3/8 -> 11/2/9 structural gain ph_delta_low 12/5/3 -> 11/4/4 structural gain nacl_delta_low 9/1/9 -> 8/1/8 deduplication psychrotolerant 12/3/8 -> 11/3/7 deduplication slightly_halophilic 11/2/9 -> 10/2/8 deduplication non_spore_forming 12/1/12 -> 11/1/11 deduplication sphere_shaped 11/1/11 -> 10/1/10 deduplication Three of eight moved the component count; five only removed a node. That is the split #360 argued for, and UNREACHABLE_FROM_TRAIT reports 1296 across all of it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes #352.
DISPOSITION_MISTYPED8 → 0,DUPLICATE_GROUNDING3 → 0.#353 shipped the detection and baselined what it found. This is the burn-down.
None of the eight were mistypes
#352 framed the fix as "sweep
CAPACITYnodes matching the disposition pattern and retype them". Not one of the eight survived the attempt, and the thing that killed each is the grounding: everyTRAITnode in the corpus is grounded, so retyping forces you to name the term the node is — and for all eight, the only available term restates the record, contradicts it, or is narrower than the node.ph_homeostasis_capacityGO:0051453cytoplasmic_ph_homeostasis, same graphelongation_capacitylateral_elongation; both alreadyreduced in → sphere_shaped_traitloss_sporulation_capacityMETPO:1000872growth_at_4cMETPO:1000618salt_tolerance_breadthMETPO:1000622is a → nacl_delta, asserting halotolerant ⊑ NaCl-deltaoxygen_toleranceMETPO:1000609METPO:1000601; false of obligate aerobessalt_toleranceMETPO:1000622METPO:1000625, and its negationlow_ph_toleranceMETPO:1003008ph_deltarecord coversIt took three review rounds to get here and the retype count went 4 → 2 → 0. Rounds 2 and 3 were caught reviewing this PR.
The test that failed four times
"Is this term distinct from the record's own?" — it passed all four nodes that later failed.
salt_toleranceis the clean counterexample:METPO:1000622(halotolerant) and the record'sMETPO:1000625(slightly halophilic) are direct siblings underMETPO:1000629, so maximally distinct — and 1000622 means "tolerates high salt but does not require it" while 1000625 means the organism "requires low to moderate salt". The node asserted the negation of its own record.The right question is whether the term is compatible with the record and no narrower than the node. Written into the playbook, because the wrong test is the intuitive one.
Connectivity: measured, not asserted
UNREACHABLE_FROM_TRAITreads 1296 under either fix (1303 → 1296), andFRAGMENTED_GRAPH's count sits flat at 218 in both — it reports one finding per split graph however many pieces it's in. Neither headline number can see the difference. That is #359, and #363 addsreports/causal_graph_connectivity.tsvto measure it.I corrected an overclaim of my own here. I first wrote that merging
salt_toleranceattaches the ectoine island; measurement showed the node was already in the trait's component viaosmoprotectant_transport → compatible_solutes → osmotic_stress, so that merge changes no structure. Claiming otherwise would have been this PR committing the exact error it exists to catch.CAPACITYis not vestigial — the issue left this open24 nodes carry it; these 8 leave 16, and the survivors are a different sense:
reducing_power(a pool of reductants),cytoplasmic_buffering_capacity(a reservoir),swimming_velocity(a rate),metabolic_versatility(a breadth). A buffer has a capacity; so does a battery. Neither is something an organism can do. That's why #353's heuristic is organism-scoped rather than matching bare capacity to.The three duplicate groundings
growth_external_ph_5_5_9sharedMETPO:1000478withph_delta_high_trait, but they say different things: an absolute external range (~5.5–9.0) versus a breadth (~5–9 pH units), which is what a pH delta is. Regrounded toMETPO:1000332.catalase/ureaseshared their function node's GO term.GO:0004096is catalase activity, and a protein is not its activity — the graphs already say so withcatalase -enables-> catalase_function. Dropped from the proteins, kept on the functions. Not a corpus-wide sweep: 72GENE_OR_PROTEINnodes use that shorthand legitimately where no function node exists.Audit trail
The first pass logged all 11 events as
RETYPE_CAUSAL_NODE, including the ones that weren't retypes. The script now emitsMERGE/DROP/REGROUND/UNGROUND_CAUSAL_NODEand appends thecuration_historyentry itself viarecord_curation_event, with a fixed timestamp becausepages/derives "Corpus as of" from the latest entry (#228).RETYPE_CAUSAL_NODEnow goes unused, which is the honest outcome. TheRETYPEtable is kept but empty — "we looked and there were none" and "we never modelled retypes" are different claims.Related
ground-nodeswould have re-created the catalase/urease duplicates on the next--apply; guard added there.just qcgreen · 519 tests pass · ruff clean · history record included per #325.