Measure component structure, the number retyping cannot move (#359) - #363
Conversation
#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>
|
Claude finished @realmarcin's task in 4m 22s —— View job Review
No 🔴 or 🟡. The measurement is sound and the committed report is real: 353 rows, of which 135 have Four optional notes, none blocking. 🔵 🔵 🔵 The regression test's second assertion doesn't test the regression — 🔵 The stale-connectivity branch sets History record is present and consistent with Approving. |
![culturebot-reviewer[bot]](https://avatars.githubusercontent.com/u/233210009?s=60&v=4){kind=small}
There was a problem hiding this comment.
No blocking issues. The committed report is internally consistent with the audit (135 of 353 rows at components=1, leaving 218 fragmented graphs — exactly the 218 FRAGMENTED_GRAPH rows in causal_graph_audit.tsv), the _topology() extraction is behaviour-preserving by inspection, and audit-derived-reports compares the regenerated audit TSV against git on every run, so green qc is itself the proof the refactor moved nothing.
Four optional notes in the review comment, none blocking. The one worth a second look: largest = len(components[0]) (scripts/audit_causal_graphs.py:223) is unguarded, and _components() filters out None seeds — so a graph whose only wired id is None raises IndexError where the old FRAGMENTED_GRAPH path degraded silently. Not reachable with today's valid corpus.
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>
… retypes (#352) (#360) * Burn down the 11 disposition-typing findings, half of which were not 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> * Regenerate biolink_coverage.tsv for the merged edges (#352) 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> * Correct two of the four claimed retypes to merges after review (#352) 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> * Merge the last two claimed retypes: none of the eight were retypes (#352) 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> * Correct two claims the data no longer supports (#360 review) 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> --------- Co-authored-by: Claude Fable 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>
|
Claude finished @realmarcin's task in 2m 34s —— View job Review
The only change since the last pass is
The report is still internally consistent with the audit: 353 rows, 135 at No 🔴 or 🟡. One new note, plus the four from the first pass, all optional. 🔵 🔵 The history record's stated corpus baseline ( Still open from the first pass, unchanged and still optional: the unguarded No Approving. |
There was a problem hiding this comment.
Re-reviewed after the regeneration commit (ec5ec58). Spot-checked three of the eight changed rows against the YAML by hand — oxygen_preference 13/2/11,2; ph_delta_low 11/4/4,3,2,2; non_spore_forming 11/1/11 — all exact, and the report is still internally consistent with the audit (135 of 353 rows at components=1, leaving 218, matching the 218 FRAGMENTED_GRAPH rows). qc is green, which means audit-derived-reports regenerated the connectivity report and found it byte-identical. Nothing blocking; the four notes from the first pass remain optional, plus one new 🔵 on prompts/issue-cycle.md:117.
1 participant
Closes #359.
#359 observed that
UNREACHABLE_FROM_TRAITfalls when a node is retyped into aTRAITanchor inside an existing island, without the graph becoming any more connected. Neither headline count can tell that apart from a real gain —FRAGMENTED_GRAPHreports one finding per split graph however many pieces it is in, so3 components → 2doesn't move it either.The measurement
connectivity_rows()emitsreports/causal_graph_connectivity.tsv, one row per graph:plus a corpus summary on the audit:
It separates the two cases that #352 could not
Measured on
oxygen_preference.yaml, the worked example from #352:The retype is bit-for-bit indistinguishable from doing nothing — correctly, because it connected nothing. Meanwhile
UNREACHABLE_FROM_TRAITreads 1296 for both fixes. That's the blindness #359 describes, now measurable.There's a test pinning exactly this: retyping leaves every connectivity field unchanged while the finding count drops, and merging moves components 2→1 and largest 2→3.
Design
_componentsis: 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.FRAGMENTED_GRAPH. An unwired node isORPHAN_NODE's business; counting it here would let one defect depress two metrics._topology()helper, used by bothaudit()and the metric, so the ratchet and the measurement cannot drift on what "connected" means. Verified behaviour-preserving: regeneratedcausal_graph_audit.tsvis byte-identical to the committed copy.One defect this found in itself
--connectivity-outoriginally defaulted to a fixed repo path. Every test that runs the audit via subprocess redirects--outto a tmpdir but knew nothing about the new flag — so one of them wrote the connectivity report into the working tree, clobbering the committed copy with a single row naming a pytest tmpdir. The staleness gate caught it, which is a decent advert for the gate.Fixed by defaulting
--connectivity-outnext to--outrather than to a fixed path, so redirecting the report redirects this too. Regression test included.audit-derived-reportsalso passes the flag explicitly to its temp dir — otherwise a staleness check would write the very file it's judging, the failure that recipe's own header warns about.just qcgreen · 525 tests pass (6 new) · ruff clean · history record included per #325.