Ground the four unambiguous NOT_ATTEMPTED taxa, and record why four stay (#401) - #458
Merged
Conversation
…tay (#401) NOT_ATTEMPTED means "the tool would ground this and the KB does not" - the one status that is unambiguously outstanding work. #401 found nine. One (Desulfobacteraceae) was grounded in the meantime, four are grounded here, and four remain for reasons that are not "nobody got round to it". Grounded, each checked against the issue's own criterion - the preferred_term names the taxon its id names, and the majority is >= 0.88: Chromobacterium violaceum -> s__Chromobacterium_violaceum 0.97 Methylobacterium extorquens -> s__Methylobacterium_extorquens 0.88 Rothia kefirresidentii KRP -> g__Rothia 1.0 novel multiheme-cytochrome Geobacter sp. -> g__Geobacter 0.948 Held on a bare majority, which is what #396 is about: Colwellia -> g__Cognaticolwellia at 0.548, a rename decided by a coin flip, and Euryarchaeota -> p__Methanobacteriota at 0.531. Held on the type-species question: both Nitrospira entries -> g__Nitrospira_D at 0.81, where _D is a non-type split. That is #377's pathology again, and #377 established it cannot be fixed by grounding to the type-bearing term either - that term holds a minority of genomes, and majority_fraction is bounded [0.5, 1.0] because it means "the fraction backing the winner". Both available answers assert something unsupported, so neither is taken. Those two also carry preferred_terms that are descriptors rather than taxa. The tool writes a block without updating gtdb_grounding_status, so each --apply is followed by --apply-status; coherence is clean and re-applying leaves every changed record byte-identical. A test pins all eight decisions and fails if a NOT_ATTEMPTED taxon appears that is neither grounded nor listed as held - so the status means held from here on, rather than forgotten. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The review demonstrated that --apply re-grounds all four taxa this PR called held. They were never added to WITHHELD_GROUNDINGS - the guard the repo built for exactly this, protecting the identical 0.534 KBase ORT case - so a routine re-run would write g__Cognaticolwellia @0.548 into the KB, which is the outcome #401 exists to prevent. All four are on the list now, each with its reason, and --apply skips them. With the reason established, NOT_ATTEMPTED was also the wrong value: it means "nothing above explains why". WITHHELD means a curator decided. Status changed for all four, which empties NOT_ATTEMPTED entirely. That in turn changes a guard in test_gtdb_coherence_validator: it asserted NOT_ATTEMPTED >= 1 on the reasoning that 0 almost certainly meant mislabelling rather than completion. It is genuinely 0 now, and the difference is that every one of #401's nine is individually accounted for - five grounded, four withheld with a reason apiece. The anti-mislabelling guard moves to the new test, which fails if a NOT_ATTEMPTED taxon appears that is neither grounded nor held. The docstring claimed a single criterion for the four groundings - "each preferred_term names the taxon its id names" - which is false for two of them, and is the same criterion used to hold two others. Rothia names a species while its id is the genus, and there was no alternative: the crosswalk has no R. kefirresidentii row. Geobacter's preferred_term is a descriptor. What actually separates them from the held descriptors is that the held ones would have to ground to a non-type split, so descriptor and weak target compound. Also updated the counts in SKILL.md and the schema, and regenerated the datamodel. The review verified the four groundings field-for-field against the crosswalk, reproduced them from main by --apply plus --apply-status byte-identically, and independently confirmed the Nitrospira reasoning - the genus type species is N. marina, not N. moscoviensis, and it maps to g__UBA8639 at 1 of 21 genomes, so the type-bearing grounding would need majority_fraction 0.048. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
Review round 1 — "request changes", and rightlyThe reviewer verified the grounding work field-for-field against the crosswalk and reproduced all four blocks from The hold was a comment, not a hold
All four are on the list now with their reasons, and And
|
Round 2 verified the holds work under both --apply and --apply --refresh, and that all four groundings reproduce byte-identically. It then found the guard protecting them had no test: every reference to WITHHELD_GROUNDINGS outside the script takes next(iter(...)), so deleting the four new entries left the entire suite green. The miss would surface only after someone ran --apply and a later test caught the resulting YAML. tests/test_gtdb_withheld_groundings.py now carries all five and asserts its dict and the script's name the same taxa, so a one-sided edit fails. Verified: deleting one hold from the script fails that test. Also fixed a sentence contradicting its own table eight lines below it - SKILL.md still said nine items were unambiguously outstanding beside a table this PR had updated to zero - and the 317 open items it quoted, which measures 301. And the WITHHELD enum description, which said withholding is "usually because the NCBITaxon id names a different organism ... fix the id and this becomes GROUNDED". That is false for all five current instances: the #416 entry's own reason records that its id was already corrected, and #401's four are holds on correct ids. It now names all three reasons and says plainly that no current instance is the id kind, so a curator does not go looking for an id to fix. Filed #459 for the skill's stale rank breakdown and coverage percentages, which were already wrong on main. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
Review round 2 — verdict: mergeRound 2 verified round 1's fixes against the tool rather than the diff: the hold works under both The one that mattered: the new holds were themselves untestedEvery reference to
Documentation
Filed, not fixed#459 — the skill's rank breakdown and coverage percentages (
|
Network integrity findingsWarnings only — a member with no interaction yet, or a participant matched by ontology id rather than by name, or one on a community-level interaction that resolves to no member. Reported, but does not fail the build. The full report is attached to the workflow run as an artifact. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #401.
The split, taken from the issue itself
NOT_ATTEMPTEDmeans "the tool would ground this and the KB does not" — the one status that is unambiguously outstanding work. #401 found nine. One (Desulfobacteraceae) was grounded in the meantime; four are grounded here, and four stay.Grounded —
preferred_termnames the taxon its id names, majority ≥ 0.88s__Chromobacterium_violaceums__Methylobacterium_extorquensg__Rothiag__GeobacterHeld on a bare majority — #396's territory
Colwellia→g__Cognaticolwelliaat 0.548: a rename decided by a coin flip. Adopting it would restate a near-tie as a fact, about a genus the record names plainly.Euryarchaeota→p__Methanobacteriotaat 0.531: same shape.Held on the type-species question — #374, and #377 for why it's stuck
Both
Nitrospiraentries →g__Nitrospira_Dat 0.81, where_Dis a non-type split. That is #377's pathology again — and #377 established it cannot be fixed by grounding to the type-bearing term either, because that term holds a minority of genomes whilemajority_fractionis bounded[0.5, 1.0](the field means "the fraction backing the winner"). Both available answers assert something unsupported, so neither is taken.Those two also carry
preferred_terms that are informal descriptors —Nitrospira-like nitrite oxidizer,Nitrospirae core floodplain members— so grounding them would assert an identity the source may not support.Operational note
The tool writes a block without updating
gtdb_grounding_status, so each--applyis followed by--apply-status— otherwise the record contradicts itself andvalidate-strictrejects it. Coherence is clean, and re-applying leaves every changed record byte-identical.The test
Pins all eight decisions, and fails if a
NOT_ATTEMPTEDtaxon appears that is neither grounded nor listed as held — so from here the status means held, not forgotten. 9 tests.just qcgreen.major_outageper GitHub's status API — no run on this repo since 14:50 UTC. Expect no checks, as on #456 and #457.🤖 Generated with Claude Code