Stop the encodes row claiming provenance it does not have (#342) - #350
Conversation
mappings/predicate_grounding.tsv grounded `encodes` to biolink:encodes with source=biolink and a note claiming an "exact label match against biolink slot". The pinned data/raw/biolink-model.yaml (4.4.0) has no such slot -- the gene-to-product slot is `has gene product` -- so the CURIE resolved to nothing upstream. Swept all 13 biolink: CURIEs used in the mapping table against the model. biolink:encodes is the ONLY one with no backing slot; the other twelve resolve. That answers the question the issue asked and bounds the problem to one row. The row keeps the coinage, because a gene CLUSTER to protein COMPLEX edge does not fit `has gene product`'s range of gene product mixin -- repointing would trade a missing term for a wrong one. What changes is the claim: source=local rather than biolink, and predicate_id skos:closeMatch rather than exactMatch, since there is no upstream term to be exactly equal to. `local` joins METPO/biolink/RO/RDFS in the column's vocabulary. The real defect was not the coinage but that nothing noticed. The signal had been sitting in reports/biolink_coverage.tsv all along as the only applied biolink: CURIE with both backing columns empty -- a report nobody consults is not a check. audit-biolink-curies is that check, and is in qc. Deliberately narrow: it does NOT require the corpus LABEL to match a slot name. Most labels are synonyms -- generates, yields, forms and six others all ground to biolink:produces -- so requiring that would flag correct rows. It requires only that the CURIE names a real slot, and exempts rows declaring source=local, which keeps a coinage legitimate while making it say so. Canaried by setting the row back to source=biolink: the gate fails and names it. Six tests, including that a synonym label passes and that a local row is exempt. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 4m 23s —— View job Review — PR #350
The central factual claim checks out. I swept all 13 distinct 🟡 The disclaimer doesn't travel with the CURIE
Anything consuming those files sees a CURIE in the
Three predicates with no adequate upstream term, all handled by minting a placeholder METPO id and filing it in a cohort. 🟡
|
![culturebot-reviewer[bot]](https://avatars.githubusercontent.com/u/233210009?s=60&v=4){kind=small}
There was a problem hiding this comment.
Diagnosis and the gate are sound — I verified all 13 biolink CURIEs against the pinned model and biolink:encodes is indeed the only one with no backing slot. But the resolution stops one step short: source=local lives in a TSV column that is never serialized into the corpus and is discarded by ground_causal_predicates.py:165, while biolink:encodes remains a live predicate_id in 8 trait files, expanding to an IRI that resolves to nothing. The repo already has the remedy for a predicate with no adequate upstream term — a placeholder METPO id in a proposal cohort (confers/METPO:2007700, has electron donor/METPO:2007701, has carbon source/METPO:2007806). Applying it here makes the source column true and leaves the new gate nothing to exempt. Also: 'Closes #342' contradicts the row note's 'the open half of #342'. Full review in the linked comment.
…review) The 🟡 is right and it changes the fix rather than refining it. source=local lived in one column of one TSV, ground_causal_predicates discards it outright, and the unqualified `biolink:encodes` was what got written into the corpus -- so a reader of a trait record saw an apparently-upstream term and the disclaimer was nowhere near them. Annotating provenance at a layer the data does not carry is not fixing the claim. Minted METPO:2007813 `encodes` into the v9 cohort instead, and migrated all 10 edges across 8 records. That removes the false claim from the DATA rather than describing it elsewhere, and puts the term in the same pending-upstream state as the other 13 v9 predicates instead of a category of its own. RO:0002205 (has gene product) was considered and rejected on the same grounds the coinage existed for: it relates a GENE to a gene product, while every corpus edge relates a gene CLUSTER or OPERON to a protein COMPLEX or a biosynthetic PROCESS. Recorded as skos:closeMatch at medium confidence -- the granularity differs on both sides, and saying `high` would overstate it. audit-biolink-curies stays. It now passes with no exemptions at all, which is a better resting state than passing because one row opted out, and it still catches the next unbacked CURIE. Worth recording: the #322 round-trip guard caught this commit. Inserting curation events as hand-built YAML left 8 records non-canonical, the guard failed, and re-emitting them through the helper fixed it -- which is exactly the workflow #322 was for, on the first bulk edit after it landed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @realmarcin's task in 3m 40s —— View job Review — PR #350 (re-review after
|
…d 2) Both 🔵s are the PR's own thesis pointed at the PR. The exemption was `source == "local"`, a free-text cell. A future unbacked CURIE could have been silenced by typing five characters into a TSV -- the same shape of failure this gate exists to catch, one level up, and exactly what the reviewer said. Now an explicit ALLOWED_UNBACKED set keyed to the CURIE, following conf/id_label_targets.yaml's `exceptions:` convention. It is EMPTY, because #342's only coinage was minted as METPO:2007813 rather than exempted, so adding an entry is a code change someone reviews rather than a cell edit. Two tests: source=local alone no longer exempts, and an explicitly allowed CURIE does. reports/biolink_coverage.tsv is now in audit-derived-reports. It had drifted -- regenerating it moved 549 lines unrelated to this change (produces METPO:2000202 -> METPO:2007800, defines residual -> applied) -- and it is the report that recorded biolink:encodes as unbacked for however long that was true. A stale report nobody reads is even less of a check than a current one, which is the argument this PR is built on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
The METPO minting is right and I verified it: METPO:2007813 is free in the 20078xx block, both cohort files carry it, all 10 encodes edges across 8 records are repointed, and the new subject_types/object_types columns match the actual node types (all subjects GENE_OR_PROTEIN; objects exactly GENE_OR_PROTEIN/BIOLOGICAL_PROCESS/ORGANELLE). closeMatch at medium against RO:0002205 is the right call. Two leftovers from the second commit: (1) reports/biolink_coverage.tsv:28 still reads 'applied encodes biolink:encodes' - it derives current_curie from the mapping table, was regenerated in commit 1 and not in commit 2, and no gate covers it, so the repo now ships a derived report asserting an applied CURIE that exists nowhere in the corpus; (2) scripts/audit_biolink_curies.py and justfile:94-103 still document the abandoned source=local remedy - the docstring says encodes 'is kept' as a coinage, and the failure message tells the next contributor to set source=local, which is exactly what this PR decided against. Also a 9-vs-8 record count in the ROBOT row that goes upstream. Full detail in the linked comment.
|
All four taken, and the first changed the fix rather than refining it. 🟡 The disclaimer not travelling with the CURIEYou're right, and it's the argument that matters: Minted
🟡
|
|
Claude finished @realmarcin's task in 2m 56s —— View job Review — PR #350 (round 3, after
|
There was a problem hiding this comment.
Round 3. The two 🟡s from last round are fixed and I verified both: reports/biolink_coverage.tsv:28 now reads METPO:2007813 with both backing columns correctly empty and row counts consistent with the mapping/residual sources (109 applied + 568 residual), and the new audit-derived-reports block gates it the same way the other four reports are gated. ALLOWED_UNBACKED is keyed to the CURIE, empty, and both directions are tested.
One 🟡 left: the gate's failure message (scripts/audit_biolink_curies.py:97-100) and the recipe comment (justfile:101-103) still tell contributors to 'keep the coinage and set source=local'. unbacked() no longer reads that column, tests/test_audit_biolink_curies.py:64 asserts a source=local row IS flagged, and no row in the 109-row mapping table uses that value any more. The docstring was rewritten; the message a failing contributor actually sees was not, and following it leaves the gate red. It should point at the remedy the repo used: repoint, or mint a placeholder METPO:20078xx in a cohort as encodes did.
Also unchanged from last round: the ROBOT row still says '9 trait records' where it is 8 (10 edges, confirmed by grep), and that row goes upstream. Full detail in the linked comment.
…350 round 3) The 🟡 is a good catch and neatly framed: the failure message told contributors to set source=local, which the tests added LAST round prove no longer exempts anything. It also could not be caught by qc, being prose in a passing gate. Rewritten to name what actually works -- repoint to a real slot, or MINT it as #342 did -- and to say plainly that source=local does not exempt and the escape is ALLOWED_UNBACKED, keyed to the CURIE. The gate read only the mapping table. That is not the only route into the corpus: ground_causal_predicates writes from the table, but a curator can type a predicate_id straight into a record, and #342's whole point is that the CURIE in the RECORD is what a reader believes. It now checks both, reporting an example file per offending CURIE so it can be found. Canaried by typing biolink:not_a_slot into a record: exit 1, CURIE and file named. The first attempt at that canary was a no-op -- it targeted RO:0002327 in crispr_cas_system.yaml, which #323 had already migrated away -- and reported a false pass until I checked the substitution actually happened. Also fixed the 9-vs-8 record count in both the v9 row and the mapping note, and guarded relative_to() against a traits dir outside the repo, which the new tests hit immediately (the same guard audit_causal_graphs already carries). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
All three taken. 🟡 The message contradicting the codeGood catch, and neatly framed — the failure message told contributors to set Rewritten to name what actually works — repoint to a real slot, or mint it as #342 did — and to state plainly that 🔵 The gate reading the mapping table, not the corpusRight, and it undercuts the PR's own argument. It now checks both, reporting an example file per offending CURIE. Canaried by typing 🔵 The 9-vs-8 countFixed in both the v9 row and the mapping note. One incidentalThe new corpus tests immediately hit
|
|
Claude finished @realmarcin's task in 2m 49s —— View job Review — PR #350 (round 4, after
|
There was a problem hiding this comment.
Round 4. The corpus sweep is right and I verified it: all 12 biolink: CURIEs still written as a predicate_id in data/traits/** name real top-level slots in the pinned 4.4.0 model, no biolink:encodes survives, there are no prose false positives in the regex sweep, and the 9-vs-8 count is corrected in both the ROBOT row and the mapping note (10 edges across 8 files, confirmed). reports/biolink_coverage.tsv:28 is consistent with the mapping table.
One 🟡 left, and it is the same one from round 3, applied in only one of the two places it was reported. scripts/audit_biolink_curies.py:129-136 was rewritten correctly. justfile:100-103 was not — git log origin/main..HEAD -- justfile shows the file untouched since round 2, and it still reads 'A local coinage stays legitimate when nothing upstream fits, provided the row says source=local rather than claiming biolink provenance.' unbacked() no longer reads that column, tests/test_audit_biolink_curies.py:58 asserts a source=local row IS flagged, and no row in the 109-row mapping table uses that value. The same paragraph also still claims in the present tense that biolink_coverage.tsv holds 'the only applied biolink: CURIE with both backing columns empty', which this PR made false.
Mechanical fix — mirror the wording already in the script. Full detail in the linked comment.
…round 4) The 🟡 is the same defect a third time in this PR: I corrected the remedy in the script's failure message last round and left it in the justfile recipe comment. As the reviewer notes, qc cannot catch it -- prose in a passing recipe, which is the class this PR was opened over. Corrected, and while there, the comment now also states that the gate checks the corpus, which it had not said either. The corpus sweep regexed YAML in a module that already imports yaml. Now parses causal_graphs[].edges[].predicate_id properly. That is not just tidiness: a text scan matches a CURIE quoted inside curation_history prose, and several records now contain exactly that BECAUSE they describe this issue -- methanogenesis.yaml and intracellular_inclusion.yaml both mention biolink:encodes in their history. A regex would have reported those as live groundings the moment the phrasing shifted. A test pins it. Switching to parsing broke two of my own tests, correctly: their fixtures used a flat `edges:` list rather than the real causal_graphs[].edges[] shape, so they would have passed through the parsing version unseen and proved nothing. Fixtures rebuilt in the real structure. Re-canaried after the change: a CURIE typed into a record still exits 1 and names the file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Both taken — and the 🟡 is the same defect for the third time in this PR, which is worth naming rather than glossing. 🟡 The recipe commentI corrected the remedy in the script's failure message last round and left it in the justfile comment. You're right that The pattern across this PR: the claim lived in three places (mapping note, script message, recipe comment) and I fixed them one review round at a time. The lesson isn't "be more careful" — it's that a claim repeated in three places will go stale in at least one, which is an argument for the minting fix over the annotation fix I started with. 🔵 Regexing YAMLFixed, and it wasn't only tidiness. A text scan matches a CURIE quoted inside Switching to parsing broke two of my own tests, correctly: their fixtures used a flat Re-canaried after the change: a CURIE typed into a record still exits 1 and names the file.
|
|
Claude finished @realmarcin's task in 3m 19s —— View job Review — PR #350 (round 5, after
|
There was a problem hiding this comment.
Round 5. Both round-4 items are resolved and I verified both. justfile:100-109 now mirrors the script's failure message — minting as the remedy, source=local explicitly non-exempting, ALLOWED_UNBACKED as the reviewed escape — and the biolink_coverage.tsv sentence is past tense, so it no longer asserts what this PR made false.
The parse rewrite doesn't narrow coverage: all 377 predicate_id: biolink:* occurrences (12 distinct CURIEs) sit at one indent level, all 353 edges: keys in the corpus are nested under causal_graphs:, and every trait file is a top-level mapping — so causal_graphs[].edges[] reaches exactly what the regex reached. All 12 name real slots in the pinned 4.4.0 model; no biolink:encodes predicate_id survives. The fixture rebuild into the real structure is the right call, since the old flat edges: fixtures would have passed through the parsing version unseen.
Substance re-verified: mapping row -> METPO:2007813, reports/biolink_coverage.tsv:28 consistent with it, ROBOT row and mapping note both say 8 records, and grep gives 10 edges across 8 files.
Three optional items left, none blocking: the prose-false-positive comment at scripts/audit_biolink_curies.py:108-111 overstates the case (no prose mention is in the predicate_id: biolink:... form the old regex needed, and no line in data/traits/** ends in predicate_id:, so the risk was latent rather than live); an unparseable file is silently counted clean at lines 104-107, which validate-strict covers in practice but which is this PR's own subject; and main()'s slot filter still has no fixture-level test. Detail in the linked comment.
The reword script matched on a 60-character prefix and substituted into it, which turned "proper polar growth and normal branch spacing" into "proper powth and normal branch spacing" in branched_shaped.yaml. Repaired, and swept every edge this PR touched for the same signature -- that is the only one. A regex that edits prose it does not parse was the wrong tool for four descriptions; doing them by hand would have been faster and safer. CURATION_PLAYBOOK.md still said "33 pre-existing edges" for ENABLES_RANGE_VIOLATION. It is 16 after this PR -- and the page is where a curator learns whether their edge shape is allowed, so a stale count there misstates how settled the area is. Third stale-prose spot in this PR, which is the same pattern #350 ended on. Added GROUND_CAUSAL_NODES events for the two groundings, per the 🔵. They were a real change to the records, not just metadata: an ungrounded TRAIT node still counts as a reachability anchor, which is how the audit number moved without the graph changing. Folded the other 🔵 into #352: nothing detects two nodes sharing a grounding within one graph, which is the machine-readable signature of exactly the duplication that issue is about, and would have found the ph_delta_high case without anyone reading descriptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…351) * Retype six mis-typed dispositions, and route six real states (#334) The recommendation going in was to check node TYPES before touching predicates, because that has been the actual defect three times running (#328's assay_measurement, #331's unmodelled acetate kinase step, #330's negated node name). It holds again: half of the STATE/CAPACITY objects here are not states at all. Their own descriptions give them away -- "Capacity of an organism to grow and survive under high hydrostatic pressure", "Ability to grow when external phosphate is unavailable", "Detoxification-mediated tolerance of arsenic oxyanions". Those describe what an organism CAN do, which is what a TRAIT is. Five nodes retyped (one carries two in-edges), six edges then take METPO:2007700 confers unchanged, because with the object correctly a TRAIT the existing relation already fits. The other six objects ARE genuine states -- a proton gradient, a community composition, an internal microoxic environment -- so retyping them would be wrong. They take RO:0002326 (contributes to), which fits because the subject genuinely contributes to the OCCURRENCE OR GENERATION of the object. That is biolink's actual wording, and it is the same test the motive-force edges FAILED in #341, where the subject powers a machine it does not generate. Getting the same relation right here and wrong there is the distinction, not an inconsistency. ENABLES_RANGE_VIOLATION: 28 -> 16. Retyping to TRAIT adds reachability anchors, so audit-graphs improved: 1535 -> 1521 findings, none new. Also widened the confers gate for ORGANELLE, which appeared once buoyancy became a TRAIT (gas_vesicle confers buoyancy) -- listed rather than left open so each new subject kind stays an explicit decision. The #322 round-trip guard caught the hand-built curation events again; 11 records re-emitted through the helper. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Ground the new TRAIT anchors, and correct my audit claim (PR #351 review) The sharpest point is one I should have caught: "audit-graphs improved 1535 -> 1521" was ANCHOR ACCOUNTING, not connectivity. pressure_optimum's 5-node island and ploidy's 4-node island stopped being reported UNREACHABLE_FROM_TRAIT without a single edge changing -- they just acquired an ungrounded anchor inside them. FRAGMENTED_GRAPH still says components=2 for both, and is still 218 overall. The islands are intact. Making a metric fall without improving the graph is exactly what I should be catching rather than causing, and the PR body said the opposite. Two of the retyped nodes duplicated traits that already exist and were left ungrounded, which the playbook requires of every TRAIT row (462 of 482 TRAIT nodes carry one, so ungrounded is the exception here): - hhp_tolerance IS piezotolerant, already traitmech:000003 -> grounded to it - growth_external_ph_5_5_9 is the same concept as its own record's ph_delta_high_trait -> grounded to METPO:1000478, which makes the duplication explicit rather than leaving two unrelated-looking anchors The playbook still told curators this case was unsettled. It now gives the rule the retyping followed -- a node described as a capacity to, an ability to, or a tolerance of is a disposition, so retype and use confers, and ground it -- plus the separate row for genuine states, with the occurrence-or-generation test that distinguishes them from #341's motive-force edges. Four edge descriptions still said "enables" under the new predicate; reworded. Filed #352 for the 🔵: salt tolerance, low-pH tolerance, oxygen tolerance and growth at 4C are the same concept still typed CAPACITY, surviving only because their in-edges did not happen to violate the range -- an unrelated fact. That wants the typing rule as its trigger, not a range-violation sweep. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Repair a description my own reword mangled (PR #351 round 2) The reword script matched on a 60-character prefix and substituted into it, which turned "proper polar growth and normal branch spacing" into "proper powth and normal branch spacing" in branched_shaped.yaml. Repaired, and swept every edge this PR touched for the same signature -- that is the only one. A regex that edits prose it does not parse was the wrong tool for four descriptions; doing them by hand would have been faster and safer. CURATION_PLAYBOOK.md still said "33 pre-existing edges" for ENABLES_RANGE_VIOLATION. It is 16 after this PR -- and the page is where a curator learns whether their edge shape is allowed, so a stale count there misstates how settled the area is. Third stale-prose spot in this PR, which is the same pattern #350 ended on. Added GROUND_CAUSAL_NODES events for the two groundings, per the 🔵. They were a real change to the records, not just metadata: an ungrounded TRAIT node still counts as a reachability anchor, which is how the audit number moved without the graph changing. Folded the other 🔵 into #352: nothing detects two nodes sharing a grounding within one graph, which is the machine-readable signature of exactly the duplication that issue is about, and would have found the ph_delta_high case without anyone reading descriptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
mappings/predicate_grounding.tsvgroundedencodestobiolink:encodeswithsource=biolinkand a note claiming an "exact label match against biolink slot". Thepinned
biolink-model.yaml(4.4.0) has no such slot — the gene-to-product slot ishas gene product— so the CURIE resolved to nothing upstream.The sweep answers the issue's open question
All 13
biolink:CURIEs used in the mapping table, checked against the pinned model:biolink:encodesis the only one with no backing slot. The other twelve resolve(
produces,located_in,part_of,catalyzes,interacts_with, …). That bounds theproblem to one row.
What changes, and what doesn't
The coinage stays. A gene cluster → protein complex edge doesn't fit
has gene product'srange: gene product mixin, so repointing would trade a missing termfor a wrong one.
The claim changes:
source=localrather thanbiolink, andpredicate_idskos:closeMatchrather thanexactMatch— there's no upstream term to be exactly equalto.
localjoinsMETPO/biolink/RO/RDFSin the column's vocabulary.The real defect was that nothing noticed
The signal had been sitting in
reports/biolink_coverage.tsvthe whole time — the onlyapplied
biolink:CURIE with both backing columns empty. A report nobody consults isnot a check.
audit-biolink-curiesis that check, and it's inqc.Deliberately narrow: it does not require the corpus label to match a slot name.
Most labels are synonyms —
generates,yields,formsand six others all ground tobiolink:produces— so requiring that would flag correct rows. It requires only that theCURIE names a real slot, and exempts rows declaring
source=local, which keeps acoinage legitimate while forcing it to say so.
What was checked
source=biolinkmakes the gate fail and name it.localrow is exempt.audit-justfile-pathscaught the unstaged script, andaudit-qc-pathsconfirms thefilter still covers what
qcreads.just qcgreen · 467 tests pass · ruff clean.Closes #342.
🤖 Generated with Claude Code