Conversation
This was referenced May 26, 2026
Add the Mammalian Phenotype Ontology (MP) prefix constant and a UberGraph-backed write_mp_ids() function paralleling write_emapa_ids in anatomy. MP is a standard rdfs:subClassOf hierarchy (unlike EMAPA's part_of partonomy), so this uses the default get_subclasses_of() walk. Every MP term is typed as biolink:PhenotypicFeature. The function is not wired into any Snakefile rule yet; the wiring lands in a later commit so each step in the MP integration is independently revertable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…for_impact_report
Mirror the anatomy.py split so the source-impact report CLI can drive disease/
phenotype too. Extract the identifier+concord loading and glom orchestration into
a new compute_cliques_for_impact_report(concordances, identifiers, excluded_sources,
*, mondoclose=None, badxrefs=None) function. build_compendium becomes a thin wrapper
that adds metadata_yamls and create_typed_sets + write_compendium on top.
The disease version is more involved than anatomy's because:
- MONDO_close is fed to glom as close={MONDO: ...}, not as a pair stream. The
Snakefile always passes it explicitly via the mondoclose kwarg, but the impact-
report CLI discovers it from disk alongside the other concord files, so the new
function pulls any MONDO_close-named path out of the iterated list before the
main concord loop. If absent (e.g. when running with incomplete intermediates),
the close map is empty rather than raising.
- Per-source bad-xrefs filtering (HP/MONDO/UMLS) and selective remove_overused_xrefs
(MONDO/HP/EFO only) are preserved verbatim. DEFAULT_BAD_XREFS is the fallback used
when the CLI invokes the function without explicit badxrefs.
Module-level constants — DISEASE_UNIQUE_PREFIXES, OVERUSE_FILTERED_CONCORDS,
DEFAULT_BAD_XREFS, MONDO_CLOSE_BASENAME — replace the inline literals so the
SEMANTIC_TYPE_CONFIG entry that lands in a later commit has a single place to
point at for the rules.
The Snakemake call site at diseasephenotype.snakefile:243 keeps passing mondoclose
and badxrefs explicitly, so production behaviour is unchanged. Final verification
of bit-equivalence against a full disease build is deferred to the impact-report
commit where the full build runs anyway.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mirror EMAPA's anatomy wiring on the disease/phenotype side: - build_disease_obo_relationships() now also writes disease/concords/MP. Uses UberGraph build_sets() with set_type="xref" from the MP root MP:0000001; the default subClassOf walk is correct (unlike EMAPA which needed part_of). - create_typed_sets()'s prefix-priority loop extended from [MONDO, HP] to [MONDO, HP, MP] so pure-new MP cliques (those without a MONDO or HP partner) are typed as PhenotypicFeature directly via prefix authority rather than falling through to the majority-vote path. MP is always last in the list, so MONDO and HP keep typing priority for any clique that contains them. - New disease_mp_ids Snakemake rule with retries: 10 (UberGraph backed). - get_disease_obo_relationships gets the MP concord output + metadata-MP.yaml, passes "MP": output.mp_metadata_yaml through to build_disease_obo_relationships. The whole rule now also carries retries: 10, matching anatomy's equivalent — this was missing before and is the same UberGraph backend that motivated the retry there. SSSOM-derived MP↔HP/NCIT mappings are deliberately not loaded; see the docs commit later in this branch for the rationale and the prior PR #300 cases that made that the safer default for now. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
config.yaml additions (MP placed near HP in each list, since both are phenotype ontologies): - generate_dirs_for_labels_and_synonyms_prefixes — labels/synonyms pipeline picks up the per-source directory. - ubergraph_ontologies — documentation list of UberGraph-backed sources. - disease_labelsandsynonyms — labels/synonyms feed into disease_compendia. - disease_ids — picked up by the expand() in disease_compendia. - disease_concords — same. MP is intentionally NOT added to disease_outputs; that list holds the biolink-type compendium filenames (Disease.txt, PhenotypicFeature.txt), not source names. src/cli/source_impact_report.py: - Import src.createcompendia.diseasephenotype. - Register "disease" in SEMANTIC_TYPE_CONFIG so the impact report's section 4 (pure-new/expanded/merged clique counts) populates for any disease source, not just MP. compendium_files is [Disease.txt, PhenotypicFeature.txt]. After this commit `uv run source-impact-report --source MP` will discover MP under babel_outputs/intermediate/disease/ (once the disease build has run) and the synthetic-mode re-glom will compute its clique impact. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Mirror EMAPA's three-layer test surface so MP regressions surface the same way:
- tests/datahandlers/test_obo_ontologies.py: new
test_write_mp_ids_collects_subclass_hierarchy. Asserts >5000 MP terms (MP has
~13k) all prefixed MP: and typed PhenotypicFeature. Guards against a regression
where the root or hierarchy predicate is accidentally changed and the walk
collapses.
- tests/pipeline/conftest.py: new mp_pipeline_outputs session fixture, paralleling
emapa_pipeline_outputs but writing to the diseasephenotype intermediate
directory. Uses ubergraph_connection as a prerequisite. New
_write_mp_concord helper calls build_sets("MP:0000001", ...). MP added to
VOCABULARY_REGISTRY so the shared partitioning tests in
test_vocabulary_partitioning.py exercise MP automatically.
- tests/pipeline/test_mp_pipeline.py: two @pytest.mark.pipeline tests mirroring
test_emapa_pipeline.py — ids syntactic check and concord syntactic check.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
write_mp_ids() and the MP concord branch of build_disease_obo_relationships both build SPARQL queries that reference MP:0000001. The query templates in src/ubergraph.py declare a fixed set of CURIE prefixes (UBERON, CL, EMAPA, GO, CHEBI, MONDO, HP, NCIT, PR, EFO) and MP was missing, so the queries failed with QueryBadFormed against the UberGraph endpoint. Add `prefix MP: <http://purl.obolibrary.org/obo/MP_>` to each of the five SPARQL templates that declare a prefix block. PR #300 added the same declaration when it tried to add MP as a disease/phenotype source. This should logically have landed with commit 902072f ("Wire MP into the disease/phenotype pipeline") but slipped through; pulling it out as its own commit keeps the fault and its fix clearly attributable. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Five MP source docs paralleling docs/sources/EMAPA/: - README.md, download.md, filtering.md — standard structure for a UberGraph- backed OBO source. - mappings.md — the SME-facing document. Explicitly records that SSSOM mappings from mh_mapping_initiative were considered and intentionally not used in this PR, and names the two PR-#300 failure cases (MP:0003342↔HP:0001748 wrong, MP:0001914↔NCIT:C26791 missed via EFO) as the concrete reasons. Points at the impact report for the artefact that should drive SME conversation about whether to revisit SSSOM. - impact-report.md — auto-generated by `uv run source-impact-report --source MP` after a full `uv run snakemake -c all disease` build. Key findings: - 14,634 MP ids; 663 UberGraph xref rows. - 14,325 pure-new MP-only cliques; 214 existing cliques expanded; 0 cliques merged — UberGraph alone introduces no controversial bridges across previously-separate cliques. This is the key SME-facing signal: the historically-controversial SSSOM merges from PR #300 do not occur on the UberGraph-only path. - Only 2 HP partner-prefix rows in the MP concord file — confirms PR #300's observation that UberGraph has essentially no MP↔HP coverage. Bridging MP to HP requires SSSOM (or another mapping source) by design. - Partner-prefix breakdown contains a small number of https/http rows (18 total) — these look like raw URLs that were not normalised into CURIEs by build_sets(). Worth a follow-up but not blocking. - Section 2 shows 93 MP ids landed in Disease.txt rather than PhenotypicFeature.txt — surfaces MP terms that got pulled into a MONDO/UMLS-dominated clique typed as Disease. Worth SME review case-by- case. The full local build was run with one workaround: the get_omim rule's HTTP fetch returned 403 (OMIM gates the default Python urllib user-agent). The file downloads fine with curl using a browser UA; I dropped it into babel_downloads/OMIM/ manually and Snakemake picked it up. Fixing the UA in pull_via_urllib is a separate, repo-wide concern and worth its own commit/PR. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…-emapa No merge conflicts — all overlapping files (config.yaml, diseasephenotype.py, diseasephenotype.snakefile, tests/pipeline/conftest.py) auto-merged cleanly. Format tests/pipeline/conftest.py to satisfy ruff format check. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Brings main (including the reviewed source-impact tool from #742 — the createcompendia/cliques.py → model/cliques.py move and the tests/cli/ + tests/model/ reorganization) into the MP/MPO branch. Git's rename detection handled the file moves cleanly; only one content conflict needed resolving: - src/cli/source_impact_report.py: keep both new imports — this branch's `import src.createcompendia.diseasephenotype as diseasephenotype` (used by the `disease` PIPELINE_CONFIG entry) and main's `from src.categories import ANATOMICAL_ENTITY, CELL, CELLULAR_COMPONENT, GROSS_ANATOMICAL_STRUCTURE` (used by main's reviewed `anatomy` entry). Auto-merged results verified: config.yaml keeps MP in the disease/ubergraph/ labels lists on top of main's reviewed compact structure; diseasephenotype's compute_cliques_for_impact_report is self-contained (gloms via babel_utils, not the moved cliques module) so the move doesn't affect it; no stale createcompendia.cliques imports remain; ruff/snakefmt/rumdl clean; 236 offline tests pass. The EMAPA references inherited from main (e.g. AddingNewSources.md's worked example) are known non-functional leftovers and are left untouched here; the plan is to refine the tooling with MP as the exemplar, then merge EMAPA so both exist as distinct worked examples. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The MP/ source docs directory existed but was missing from the "Sources documented so far" list in docs/sources/README.md. Add an entry mirroring the others (UberGraph subClassOf extraction from MP:0000001, PhenotypicFeature typing, disease pipeline). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The "Related reading" pointer at docs/sources/EMAPA/ resolves to nothing on this branch — EMAPA is a non-functional leftover inherited from main and its source docs are not present here. Point it instead at docs/sources/MP/, which exists and is an equivalent OBO-from-UberGraph worked example. The rest of the doc's EMAPA-based worked example (write_emapa_ids, test_emapa_pipeline.py, the GrossAnatomicalStructure registration discussion and its identifier counts) is left as-is; converting the narrative exemplar to MP — and later re-adding EMAPA as a second exemplar — is the planned refinement. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds MP (Mammalian Phenotype Ontology) as a new disease/phenotype source by extracting MP IDs from UberGraph and generating MP xref concords, and updates the source-impact reporting machinery so clique-impact reporting works for disease/phenotype sources (not just anatomy).
Changes:
- Add MP identifier extraction + UberGraph xref concord generation, and wire MP into the disease/phenotype Snakemake/config pipelines.
- Refactor disease/phenotype compendium building to expose
compute_cliques_for_impact_report()for the source-impact report CLI. - Add MP-focused pipeline/unit tests and new MP source documentation (including a committed impact report).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/pipeline/test_mp_pipeline.py | New pipeline test ensuring MP ids/concord intermediates are produced and syntactically valid. |
| tests/pipeline/conftest.py | Adds an MP pipeline fixture to generate MP ids + concords via UberGraph. |
| tests/datahandlers/test_obo_ontologies.py | Adds network-marked tests for OBO/UberGraph ontology ID extraction and MP subclass coverage sanity check. |
| src/ubergraph.py | Adds SPARQL MP: prefix to relevant UberGraph queries. |
| src/snakefiles/diseasephenotype.snakefile | Adds MP IDs rule and includes MP concord + metadata in the disease OBO relationships step. |
| src/prefixes.py | Registers MP (and EMAPA) as known CURIE prefixes. |
| src/createcompendia/diseasephenotype.py | Implements write_mp_ids, MP xref concord generation, and factors clique computation into compute_cliques_for_impact_report. |
| src/cli/source_impact_report.py | Registers the disease pipeline for synthetic clique-impact reporting using the new compute helper. |
| docs/sources/README.md | Adds MP entry to the sources documentation index. |
| docs/sources/MP/README.md | New MP source overview documentation. |
| docs/sources/MP/mappings.md | Documents UberGraph-only xrefs and explicitly defers SSSOM mappings with rationale. |
| docs/sources/MP/impact-report.md | Committed generated MP source-impact report. |
| docs/sources/MP/filtering.md | Documents MP root, inclusion logic, and typing behavior. |
| docs/sources/MP/download.md | Documents MP’s UberGraph-backed “no download” behavior and artifacts. |
| docs/AddingNewSources.md | Updates “Related reading” to include MP as an OBO-from-UberGraph worked example. |
| config.yaml | Wires MP into disease IDs/concord/labels lists and UberGraph ontology/prefix lists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- src/prefixes.py: remove the duplicated `MP = "MP"` assignment so the constant is declared once (avoids lint warnings and hidden edits). - diseasephenotype.compute_cliques_for_impact_report: skip blank lines and raise a clear RuntimeError on malformed MONDO_close rows instead of letting a short line abort the run with an opaque IndexError, mirroring the concord row-shape validation below it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- test_mp_pipeline: check os.path.exists(concord_path) before opening so a
missing file fails on the explicit assertion rather than inside open().
- conftest._write_mp_concord: build the root CURIE from the MP constant
(f"{MP}:0000001") instead of hard-coding the prefix string.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
My earlier Copilot-review hardening of the MONDO_close reader assumed exactly two tab-separated columns, but MONDO_close is a 3-column concord (subject, predicate, object) written by ubergraph.build_sets() just like every other concord in this function. The `len(x) != 2` check therefore rejected every real row and aborted the whole disease build: RuntimeError: Line "MONDO:0000739<TAB>oio:closeMatch<TAB>MEDDRA:10051962" is not a valid MONDO_close entry Validate 3 columns instead, still skipping blank lines and raising a clear RuntimeError on a genuinely malformed row. This restores main's behaviour of keying on the subject and column-2 value; the separate, pre-existing latent bug of keying on the predicate (x[1]) rather than the close-match object (x[2]) -- which leaves glom()'s `close=` guard a no-op -- is deferred to its own follow-up PR with a before/after impact analysis (it changes disease clique merging broadly and is orthogonal to adding MP). Add unit tests feeding the real 3-column format (and a malformed row) through compute_cliques_for_impact_report so this column-count regression can't recur. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Produced by `uv run source-impact-report --source MP` against a full local `uv run snakemake -c all disease` build on biolink_version 4.4.3. Updates the committed report and adds the three SME-facing detail files (new-cliques.csv, modified-cliques.csv, new-xrefs.tsv); modified-cliques.json stays gitignored. Key signals are stable vs the prior report: 0 cliques merged (UberGraph-only introduces no controversial bridges) and only 2 HP partner-prefix rows. Counts shifted with current UberGraph content and a newer report template: 14,750 MP identifiers (was 14,634), 14,657 landing in PhenotypicFeature.txt and 93 in Disease.txt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cross-link the deferred x[1]->x[2] close-match-guard fix to its dedicated PR (#883), and note the concrete impact (~1,219 MEDDRA identifiers dropped from Disease.txt) that the follow-up's before/after analysis quantifies. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…test basenames Lessons from running disease builds and adding a tools/ test: - CLAUDE.md "Running Babel": `snakemake --forcerun A B` consumes both as rules and silently runs the whole pipeline (no positional target → default `all`); put the target positionally. Also: config.yaml is read once per invocation, so editing it mid-run has no effect. - tests/README.md: test files under the non-package tests/tools/ are imported as top-level modules (pytest prepend mode), so each needs a basename unique across the whole suite, or it collides with e.g. tests/test_clique_diff.py. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Split the test docs along the human/agent axis the way the rest of the repo does: keep tests/README.md focused on taxonomy, the per-file index, and strategy; move the "how not to break things" guidance into a directory-scoped tests/CLAUDE.md that auto-loads when editing files under tests/. tests/CLAUDE.md captures the import-mode gotchas (no tests/tools/__init__.py; unique test-file basenames under pytest prepend mode), which subset to run, and the conventions for adding a test (shared assert_*_file_valid validators, docstrings + section comments, column-constant mirroring). README.md now points to it for the import-mode detail; root CLAUDE.md points to it too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The disease entry in source_impact_report.PIPELINE_CONFIG was missing the
clique_classifier, biolink_types, and compendium_prefixes keys that the anatomy
entry carries. Without a classifier, _biolink_type_for() returned None for every
disease/phenotype clique, so:
- clique_biolink_type was always blank in the report;
- preferred_curie() fell back to the lexicographically smallest CURIE instead
of the Biolink id_prefixes order, picking DOID (or even Fyler/EFO) as the
clique leader instead of MONDO (Disease) or HP (PhenotypicFeature);
- preferred labels were blank (no compendium_prefixes to load labels from).
Extract classify_disease_clique() from create_typed_sets() (mirroring anatomy's
classify_anatomy_clique) and wire it, the two disease biolink types, and the
disease label prefixes into PIPELINE_CONFIG so the report classifies each clique
and orders its identifiers the same way the real build does. create_typed_sets()
now raises RuntimeError on an untypable clique instead of print()+exit(), matching
the anatomy version and the repo's error-handling convention.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Rebuilt the disease/phenotype pipeline locally and reran source-impact-report --source MP against the fixed PIPELINE_CONFIG. Every modified clique now carries its biolink type (Disease/PhenotypicFeature) and is led by the correct preferred identifier (MONDO/HP/EFO in Biolink id_prefixes order) rather than DOID/Fyler; pure-new MP cliques type as PhenotypicFeature with MP as the registered, surviving leader. Some row churn also reflects fresh upstream data from this build. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Every HP term Babel ingests describes a human (NCBITaxon:9606) phenotype and every
MP term a mammalian (NCBITaxon:40674) phenotype, but until now phenotype identifiers
carried no taxon. Add a write_phenotype_taxa() helper that derives a
babel_downloads/<PREFIX>/taxa file from the prefix's already-built ids file, plus
disease_hp_taxa / disease_mp_taxa rules that produce babel_downloads/{HP,MP}/taxa and
feed them into disease_compendia as inputs. TaxonFactory then populates each
identifier's "t" field at write_compendium time; cliques that mix HP and MP members
carry both taxa via the existing per-clique union.
Deriving the taxa file from the ids file (rather than re-walking the ontology) keeps
it exactly in sync with the identifiers Babel ingests, so it never tags a term we
don't emit or misses one we do. The prefix->taxon map lives in config.yaml
(disease_phenotype_taxa), adjacent to disease_ids.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add docs/sources/HP/README.md (HP -> NCBITaxon:9606 "Homo sapiens"), note the MP -> NCBITaxon:40674 "Mammalia" tagging in docs/sources/MP/README.md, and index both in docs/sources/README.md. Each explains that the taxa file is derived from the ids file and that mixed HP+MP cliques carry both taxa, with the HP/MP merge question left for SME review. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The repo convention caps Snakemake `retries:` at 3 for network-backed rules: UberGraph requests already get per-request retry-with-backoff inside TripleStore.execute_query, so the whole-rule retry is only a coarse safety net. disease_mp_ids and get_disease_obo_relationships were still at 10; bring both to 3. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…n CLAUDE.md Two gotchas from adding HP/MP phenotype taxa and fixing the source-impact report: - A PIPELINE_CONFIG entry needs clique_classifier, biolink_types, and compendium_prefixes, not just compute_fn; omitting them blanks the clique type and makes the report pick the lexicographically-smallest CURIE as leader (the DOID/Fyler-instead-of-MONDO/HP bug this PR fixed). - When every term of an ontology shares one fixed taxon, derive its taxa file from the prefix's ids file so it stays in sync with what Babel ingests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Jun 30, 2026
Add docs/sources/MP/disjointness.md explaining the post-glom split, why unique_prefixes/concord-dropping are insufficient, and the measured impact (added/split/moved/deleted) from babel-clique-diff comparing the overlap-allowed build to the disjoint build. Commit the clique-diff CSV + summary JSON under docs/sources/MP/disjointness/. Update the MP and HP READMEs: MP/HP are now disjoint, so an MP clique carries only the Mammalia taxon and an HP clique only Homo sapiens (correcting the earlier "mixed cliques carry both taxa" note). Cross-link prior PRs (#790, #300, #883, #742/#781). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gaurav
added a commit
that referenced
this pull request
Jul 10, 2026
…886) Adds MP (Mammalian Phenotype Ontology) as a disease/phenotype source, **kept disjoint from HP (Human Phenotype)**: no clique may contain both an HP and an MP identifier. This is the SMEs' decision after reviewing the overlap-allowed MP addition in #790 — human and mouse phenotypes behave differently enough that conflating them is undesirable. MP may still merge with non-HP disease ids (MONDO/MESH); only HP triggers a separation. MP's xrefs are also **restricted to an allowlist of trusted target prefixes** (`HP`, `MGI`, `MPATH`, `UMLS`). MP uses `oboInOwl:hasDbXref` to mean "this phenotype is *about* that thing", not "is equivalent to it", so nine of the thirteen namespaces it xrefs into are category errors when read as equivalences. See "MP xref allowlist" below. This PR also **starts assigning taxa to HP as well as MP** — every HP identifier becomes `NCBITaxon:9606` and every MP identifier `NCBITaxon:40674`. HP had no taxa file on `main`, so this changes existing output beyond MP; see "Taxa" below. This is a clean reimplementation off `main` (curated cherry-pick of #790's substantive MP commits, dropping churn) plus the new disjointness logic — so #790 (`add-mpo`) stays **untouched as the "allow overlap" fallback** in case this decision is revisited. Closes #897 — a bug in `pull_uber_synonyms()` surfaced while re-verifying the MP build after deleting `babel_downloads/MP` (see "Review fixes" below). Closes #906 — a bad MP xref that cliqued "external male genitalia hypoplasia" with "Bifid scrotum" (see "MP xref allowlist" below). The issue carries executable BabelTest assertions that keep running against live NodeNorm after it closes. ## How disjointness is enforced `glom()`'s `unique_prefixes` cannot do this (it only forbids duplicate *same-prefix* ids), and dropping MP's own concord is insufficient (the HP and EFO concords emit direct `HP→MP`/`EFO→MP` xrefs, plus transitive MESH/SNOMED/MONDO bridges). Instead, a **post-glom split** (`split_mutually_exclusive_cliques`, driven by `MUTUALLY_EXCLUSIVE_PREFIX_GROUPS = [[HP, MP]]`) peels MP out of any clique that also contains HP, keeping the HP side intact. It runs as the last step of `compute_cliques_for_impact_report`, the single path shared by the real build and the source-impact report, so both agree. Precedent: the type-driven split in `chemicals.py`. Full writeup + impact: [`docs/sources/MP/disjointness.md`](docs/sources/MP/disjointness.md). ## MP xref allowlist MP's UberGraph xrefs are not equivalences. Of the 663 xref rows MP contributes, only 78 survive review: | Prefix | Rows | The targets are | |---|---|---| | `Fyler` | 257 | Codes from a congenital-heart-defect registry | | `CL` | 112 | The cell type the abnormality occurs in | | `MA` | 85 | The mouse anatomical structure that is abnormal | | `GO` | 76 | The biological process the phenotype perturbs | | `MGI` | 70 | **Kept.** MGI phenotype-slim terms | | `FMA` | 30 | Human anatomy | | `https`, `http` | 18 | Wikipedia, Medscape and other web pages | | `MPATH` | 4 | **Kept.** Mouse pathology lesions | | `NLX` | 4 | NeuroLex cell types | | `UMLS` | 2 | **Kept.** Phenotype concepts | | `PMID` | 2 | Literature citations | | `HP` | 2 | **Kept.** Genuine phenotype equivalences | | `NBO` | 1 | A behavior | For example `MP:0009873` "abnormal aorta tunica media morphology" xrefs `MA:0002903` (the structure that is abnormal) and `MP:0002998` "abnormal bone remodeling" xrefs `GO:0046849` "bone remodeling" (the process it perturbs). **Compendium output is unchanged by the filtering.** Every dropped prefix is either absent from all other disease concords — so `NodeFactory` drops it at `write_compendium`, since none of them is in `biolink:PhenotypicFeature`'s `id_prefixes` — or, in `Fyler`'s case (221 of its 257 rows), reaches only HP terms that the `[HP, MP]` split separates regardless. What changes is that the concord stops carrying anatomy/process/citation noise, and that a namespace MP newly starts emitting is now **rejected until reviewed** rather than silently trusted. This is implemented as a new `allowed_prefixes` parameter on `build_sets()` — the fail-closed complement of the existing `ignore_list`, which fails open. Both are matched against `Text.get_prefix_or_none()`, which upper-cases, so a non-upper-case entry would silently never match; `build_sets()` now raises rather than documenting that landmine. See #907. One row survives the allowlist and is still wrong: `MP:0009203` "external male genitalia hypoplasia" → `UMLS:C0341787` "Bifid scrotum" (a specific malformation that UMLS, HP and SNOMED agree on). It is dropped explicitly in the new `input_data/mp_badxrefs.txt`. The `[HP, MP]` split already masks it today, so the bad-xref entry is what protects the clique if that policy is ever relaxed — hence the BabelTest in #906, which asserts both that `MP:0009203` stays out and that `HP:0000048`/`UMLS:C0341787` stay in (a fix that over-splits would be a worse bug). Upstream reporting to the MP maintainers is tracked in #905. ## Taxa — a change to existing HP output Two new rules (`disease_hp_taxa`, `disease_mp_taxa`) derive `babel_downloads/<PREFIX>/taxa` from each prefix's already-built ids file, via `write_phenotype_taxa` and the new `disease_phenotype_taxa` config block. Every HP term Babel ingests (the `HP:0000118` "Phenotypic abnormality" subtree) describes a human phenotype and every MP term a mammalian one, so the mapping is a fixed `HP → NCBITaxon:9606`, `MP → NCBITaxon:40674`. **MP's taxa are new output; HP's are a change to output that already exists.** `main` writes no `babel_downloads/HP/taxa`, so today every HP-bearing clique has an empty `taxa` and `taxon_specific: false`. After this PR, `write_compendium` sets each HP identifier's `t` field and unions the taxa onto the clique, so: - every `PhenotypicFeature.txt` clique containing an HP identifier gains `taxa: ["NCBITaxon:9606"]` and flips `taxon_specific` to `true`; - so does every `Disease.txt` clique that happens to contain an HP identifier — `create_typed_sets` trusts MONDO's type first, so a MONDO disease clique with an HP member stays a `biolink:Disease` and now claims taxon = human. That second case is intended, and is worth stating explicitly: HP only ever refers to human phenotypes, so any MONDO (or other) clique an HP identifier is merged into *should* be human-specific. If one isn't, the clique is wrong and we want to find out. `babel-clique-diff` compares clique *membership* only, so **none of this appears in the impact artifacts below** — the change is invisible to every number on that page, which is why it gets its own section here. Downstream exposure is low for now: the taxon fields are really only consumed for genes and proteins today, and the new NameRes filters by taxon without excluding `taxon_specific: false` entries. If the annotation turns out to cause trouble, it can be adjusted after the next full build. ## Impact — how this PR changes Babel Measured by `babel-clique-diff` comparing a **`main` build (Babel without this PR — no MP)** against **this branch (MP added, disjoint)**. Both were built the same day from the same cached `babel_downloads`, so the UberGraph-derived concords match and the *only* difference is this PR's code. (An earlier revision of this page diffed an overlap-allowed build against the disjoint build; both already contained MP, so the additions were invisible — this baseline avoids that.) See `docs/sources/MP/disjointness.md` and `disjointness/clique-diff.{csv,summary.json}`. - **`PhenotypicFeature.txt`: 60,718 → 75,469 cliques (+14,751, +24%)** — dominated by the 14,750 wholly new MP-only phenotype cliques. Because they have no `main` counterpart they appear as the summary's `clique_count.diff`, **not** as per-clique change rows (the diff iterates before-cliques) — which is why the change-row counts below look small next to this number. This reconciles with the impact report's ~14,750: the two artifacts just report additions in different fields. - **`Disease.txt`: 365,466 → 365,466 (count unchanged)** — MP is a phenotype ontology and adds no disease cliques. - **Disjointness holds:** 0 cliques contain both HP and MP. In fact **MP never coexists with any non-MP identifier** — all 14,750 MP cliques are MP-only singletons, because MP's surviving xrefs (`MGI`, `MPATH`) point outside the disease id space and its `HP`/`UMLS` xrefs are separated by the split. So MP never becomes the preferred leader of a mixed clique. - **Disjointness change rows:** 26 MP members `regrouped` out of HP/EFO phenotype cliques that `main` had leaked them into (via unfiltered HP→MP / EFO→MP xrefs this PR removes); 8 `Disease.txt` cliques released an MP member that `moved` to `PhenotypicFeature.txt`; 1 `dropped` — the stray untypeable `MP:0005555` (`create_typed_sets` now drops an untypeable clique with a warning instead of aborting the build). - **Incidental reshuffle (not MP membership):** 7 `Disease.txt` cliques (96 members, no MP) `regrouped`. These are cross-references contested between two near-synonymous MONDO cliques that `unique_prefixes` keeps separate (e.g. angiomyolipoma vs kidney angiomyolipoma); MP's *presence* in the build — not its position (verified: MP-last gives a byte-identical diff), and not MP membership — shifts which one claims the shared member. Deterministic; creates/deletes no cliques. Pre-existing `glom` behavior, tracked in #894. - **Not captured above:** the taxa change. `babel-clique-diff` diffs membership, not the `t`/`taxa` fields, so the HP/MP taxon annotation described under "Taxa" is invisible to every number on this page. Read that section alongside these. ## What's in this PR - **MP ingestion** (cherry-picked from #790): MP prefix + `write_mp_ids`, SPARQL MP prefix, pipeline wiring + xref concords, config lists, `disease` registered in the source-impact `PIPELINE_CONFIG` with `classify_disease_clique`, MONDO_close hardening, tests/fixtures, MP docs. - **HP/MP taxa** (cherry-picked from #790): `write_phenotype_taxa`, the `disease_hp_taxa` / `disease_mp_taxa` rules, and the `disease_phenotype_taxa` config block. Changes existing HP output — see "Taxa" above. - **Disjointness** (new): `split_mutually_exclusive_cliques` + constant + the untypeable-clique fix, with unit tests. - **MP xref allowlist** (new): `allowed_prefixes` on `build_sets()`, `MP_XREF_ALLOWED_PREFIXES` in `diseasephenotype.py`, and `input_data/mp_badxrefs.txt` wired into `DEFAULT_BAD_XREFS` and the `disease_compendia` rule. Unit tests for both; `docs/sources/MP/mappings.md` documents the per-prefix reasoning. - **clique-diff enhancement** (new): `--before-label`/`--after-label`/`--note` write a self-describing `about` block, and the per-compendium counts nest `clique_count` (before/after/diff/diff_percent). Docstring + `docs/tools/README.md` now spell out that wholly new cliques appear only in `clique_count.diff`, never as change rows. - **Impact artifacts**: regenerated `docs/sources/MP/impact-report*` and the `docs/sources/MP/disjointness/` clique-diff CSV+JSON (now the overall main-vs-branch diff). ## Follow-up issues (out of scope here) Pre-existing behaviors surfaced during review; none blocks this PR: - **#893** — `get_config()` ignores Snakemake `--config` overrides, so redirecting `output_directory` to build into an isolated directory (useful for these side-by-side diffs) doesn't fully take effect; compendia still write to `babel_outputs/`. - **#894** — `glom`'s `unique_prefixes` tie-break for a contested cross-reference is sensitive to the input set (the incidental `Disease.txt` reshuffle above). Not introduced by this PR. - **#907** — `Text`'s three incompatible prefix conventions make a prefix comparison that can never match easy to write and impossible to notice. Guarded at `build_sets()`'s boundary here; the general fix (and the rest of the audit — `norm()`'s `other_prefixes` keys, `build_sets`'s `concordfiles` keys, `ignore_list=["ICD"]`) is tracked there. ## Review fixes Addressed in follow-up commits after Copilot and manual review passes: - Copilot: replaced bare `print()` progress output with the module logger, removed leftover hard-coded debug scaffolding, gave `split_mutually_exclusive_cliques()` a safe (non-mutable) default argument, narrowed a test's exception handling to the specific transient UberGraph/network error types, and fixed a stale comment claiming HP/MP-mixed cliques carry both taxa (they can't — that's exactly what this PR's split prevents). - Manual: `DISEASE_UNIQUE_PREFIXES` and `OVERUSE_FILTERED_CONCORDS` didn't include MP, unlike MONDO/HP/EFO, so MP identifiers were missing the same-prefix overmerge guard and overused-xref filtering; `compute_cliques_for_impact_report()`'s `MONDO_close` basename-discovery bypassed `excluded_sources` filtering (a `--source MONDO` impact-report run would still apply MONDO's close-match data); and `split_mutually_exclusive_cliques()` scanned every clique in the whole disease/phenotype dict instead of just the ones touching HP/MP. All fixed with regression tests. - Second manual pass: `split_mutually_exclusive_cliques()` peeled every prefix after `group[0]` rather than every prefix after the earliest *occupied* one, which for a group whose first prefix is absent would strand the clique's out-of-group members in a singleton (unreachable with `[[HP, MP]]`, but wrong); it also compared CURIE prefixes with a raw `split(":")` while `build_sets()` compares against the upper-casing `Text.get_prefix_or_none()`, so a lower-case group constant (`prefixes.ORPHANET`) would have silently failed open. Both fixed and tested. `create_typed_sets()` now logs a total alongside the per-clique warnings, so a mass drop is visible; `DEFAULT_BAD_XREFS` is anchored at the repo root so the impact-report CLI works from any directory; `EFOgraph` upper-cases the excluded-prefix set once per query rather than once per result row (and one `logging.warning` there is now the module logger); `disease_hp_ids` gained the `retries: 3` its MP twin already had; and `clique-diff`'s `diff_percent` is `null`, not `0.0`, when the before build had no cliques — now documented and tested. - Prefix-comparison guard: `build_sets()` now raises `ValueError` on an `ignore_list` / `allowed_prefixes` entry that isn't upper-case, instead of silently never matching it. This is the narrow, boundary-only slice of #907 — it covers the `allowed_prefixes` parameter this PR introduces, and every existing caller already satisfies it. Unifying `Text`'s three prefix conventions (`get_prefix_or_none` upper-cases, `get_prefix` doesn't, `recurie` lower-cases into `prefixmap`) touches ~30 call sites across anatomy/protein/process and needs its own build-vs-build diff, so it stays in #907. - Re-verification: deleting `babel_downloads/MP` and rebuilding from scratch surfaced `pull_uber_synonyms()` (`src/datahandlers/obo.py`) writing empty per-prefix `synonyms` files (with a spurious "not found" warning) for every UberGraph-derived prefix, MP included — its internal dict was keyed by full CURIE instead of by prefix, so the per-prefix membership check never matched. Fixed by grouping by prefix (mirroring the already-correct `pull_uber_labels()`) and correcting the output to 3 columns; regression test added. Practical impact was muted since `SynonymFactory` also falls back to the (correctly-built) common synonyms file. Closes #897. Note this touches **every** prefix in `generate_dirs_for_labels_and_synonyms_prefixes`, not just MP, and turns the old "prefix not found" warning into a `ValueError` — again mirroring `pull_uber_labels()`, so a future UberGraph hiccup fails the rule loudly instead of silently writing an empty synonyms file. ## Test plan - `uv run pytest -m unit -q` — green (287 passed), incl. the split + untypeable-drop tests, the clique-diff self-describing/`clique_count` and new-clique-only-in-count-delta tests, and the review-fix regression tests above, and the MP xref allowlist / bad-xref tests. - `uv run ruff check` / `ruff format --check` / `snakefmt --check .` / `rumdl check .` — clean. - Local `disease` build + disjointness scan: 0 HP+MP cliques. - Rebuilt `concords/MP` against UberGraph and confirmed the allowlist shape: 78 rows (`MGI` 70, `MPATH` 4, `UMLS` 2, `HP` 2), down from 663. Regenerated `impact-report/` shows the MP identifier count untouched (14,750 new cliques, 0 existing cliques modified or merged); every changed `new-cliques.csv` row loses a junk member and empties its `needs_biolink_registration` column. - `babel-clique-diff` `main`-vs-branch shows the additions (as the count delta) plus only MP split/move/drop and the one incidental MONDO tie-break reshuffle (#894) — no unrelated churn, so it doubles as a completeness check on the reimplementation. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Collaborator
Author
|
Should be superceded by #886, which has much more control over MP CURIE mappings anyway, so I'm closing this. We can reopen it if needed. |
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
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.
Summary
Adds MP (Mammalian Phenotype Ontology, prefix
MP) as a new disease/phenotype source, mirroring how EMAPA was added as an anatomy source in #742. UberGraph xrefs only — SSSOM is deferred and documented. Along with PR #781, this closes #733.Also extracts
compute_cliques_for_impact_reportfromdiseasephenotype.build_compendiumso the source-impact report's section 4 (clique impact) populates for any disease/phenotype source from now on. Productionbuild_compendiumnow delegates to that shared function, so production clique-building behaviour is unchanged.Finally, this PR tags every ingested HP and MP identifier with a taxon in the compendia (HP →
NCBITaxon:9606"Homo sapiens", MP →NCBITaxon:40674"Mammalia") and fixes a source-impact-report bug where disease/phenotype cliques were left untyped and led by the wrong identifier (e.g. aDOIDleader instead ofMONDO/HP). See the two dedicated sections below.Partial takeover of PR #300
This partially takes over from the unmerged
add-mammal-phenotype-ontologyPR. That work stalled on the question of which MP↔HP/NCIT bridges to keep when SSSOM mappings were added — concrete examples documented there:MP:0003342("accessory spleen") was cliqued withHP:0001748("Polysplenia"); the correct partner wasHP:0001747("Accessory spleen").MP:0001914↔NCIT:C26791("hemorrhage") was missed because the bridge runs through EFO, and EFO is not in UberGraph.This PR resolves the immediate blocker by:
docs/sources/MP/impact-report.md) that we can hand to SMEs to drive the SSSOM-or-not decision in a follow-up PR.SSSOM is intentionally not loaded in this PR. The two PR-#300 failure cases are documented in
docs/sources/MP/mappings.mdas the rationale.Impact report highlights
From the committed
docs/sources/MP/impact-report.md, produced byuv run source-impact-report --source MPagainst a full localuv run snakemake -c all diseasebuild:Two follow-up items the report surfaces (out of scope here, not blocking):
https/httpraw-URL rows in the partner-prefix breakdown — a CURIE-normalisation gap inbuild_sets().Disease.txtrather thanPhenotypicFeature.txt(they glom with a MONDO/HP disease partner; 14,657 land inPhenotypicFeature.txt) — worth SME review case-by-case.Open questions for reviewers / SMEs
Phenotype taxon tagging (HP + MP)
Every HP term Babel ingests describes a human phenotype and every MP term a mammalian phenotype, but phenotype identifiers previously carried no taxon. This PR adds per-identifier taxa to the disease/phenotype compendia:
NCBITaxon:9606"Homo sapiens"; MP →NCBITaxon:40674"Mammalia". The prefix→taxon map lives inconfig.yaml(disease_phenotype_taxa), adjacent todisease_ids.write_phenotype_taxa()helper derivesbabel_downloads/{HP,MP}/taxadirectly from each prefix's ids file, so the taxa cover exactly the identifiers Babel ingests and never drift from them (no extra ontology walk, and no dependence on the HP/MP root-inclusion subtlety). New rulesdisease_hp_taxa/disease_mp_taxafeed these files intodisease_compendia.TaxonFactorythen populates each identifier'stfield atwrite_compendiumtime. A clique that mixes HP and MP members carries both taxa via the existing per-clique union — verified against a fresh localdiseasebuild (pure-MP clique → Mammalia; HP clique → Homo sapiens; mixed clique → both; HP/MP terms that land inDisease.txtcliques are tagged too, while pure non-phenotype disease cliques stay untagged).Source-impact report clique-typing fix
The
diseaseentry added to the source-impactPIPELINE_CONFIGin this PR was missing theclique_classifier/biolink_types/compendium_prefixeskeys that the anatomy entry carries. Without a classifier the report typed every disease/phenotype clique as blank and fell back to the lexicographically-smallest CURIE for the preferred id — so cliques showed aDOID(or evenFyler) leader instead ofMONDO(Disease) /HP(PhenotypicFeature), and labels were blank.Fixed by extracting
classify_disease_clique()fromcreate_typed_sets()(mirroring anatomy'sclassify_anatomy_clique) and wiring it intoPIPELINE_CONFIG, so the report classifies and orders identifiers exactly like the real build.create_typed_sets()now raises on an untypable clique instead ofprint()+exit(). The committeddocs/sources/MP/impact-report*were regenerated against a fresh local build: every modified clique is now typed (Disease/PhenotypicFeature) and led by MONDO/HP/EFO, with zero DOID/Fyler leaders.What's in this PR
MPprefix constant andwrite_mp_ids()(subClassOf walk fromMP:0000001, typedbiolink:PhenotypicFeature).compute_cliques_for_impact_reportfrombuild_compendium; production behaviour unchanged.build_disease_obo_relationshipsMP xref traversal, Snakefiledisease_mp_idsrule + MP concord/metadata outputs,create_typed_setspriority (MONDO, HP, MP).diseaseregistered in the source-impactPIPELINE_CONFIG.disease_phenotype_taxaconfig,write_phenotype_taxa()handler,disease_hp_taxa/disease_mp_taxarules, and HP/MP taxa wired intodisease_compendia(see the taxon-tagging section above).classify_disease_clique()+ the missingdiseasePIPELINE_CONFIGkeys, so the report types and orders cliques like the build (see the clique-typing section above).disease_mp_idsandget_disease_obo_relationshipsfromretries: 10toretries: 3, per the repo convention (UberGraph already retries per-request insideTripleStore).MP:SPARQL prefix declaration to the relevant queries (bug fix found during the local build).docs/sources/MP/(README, download, filtering, mappings, and the SME-facing impact report).MPprefix constant, hardenedMONDO_closeparsing against blank/malformed rows, and tidied the MP pipeline tests (existence-check ordering, use theMPconstant for the root CURIE).MONDO_closecolumn-count fix — the hardened reader assumed 2 columns, butMONDO_closeis a 3-column concord (subject, predicate, object); the wrong check aborteddisease_compendia. Now validates 3 columns, with a regression test that runs the real format throughcompute_cliques_for_impact_report. The report numbers above come from a fresh full localdiseasebuild with this fix applied.Out of scope
docs/sources/MP/mappings.mdfor revisit after SME review of the impact report).https/httpraw-URL issue inbuild_sets()partner-prefix output.pull_via_urllib(default Python user-agent is gated; fixed separately in Fix OMIM download with an improved User Agent string #797).MONDO_closeclose-match guard (the latentx[1]→x[2]bug): it changes disease clique merging broadly and is handled separately in Activate glom's MONDO close-match guard, with concord-reader symmetry tests #888, which applies the same fix directly tomain'sbuild_compendium. Activate glom's MONDO close-match guard, with concord-reader symmetry tests #888 ismain-based and may merge first; if it does, this PR picks up the fix (in the refactoredcompute_cliques_for_impact_report) whenmainis merged in, so the guard change is not duplicated. The full before/after impact analysis lives in Activate the MONDO close-match guard in disease glom (with build-vs-build clique-diff tool) #883.Test plan
uv run ruff check— clean.uv run ruff format --check— clean.uv run snakefmt --check --compact-diff .— clean.uv run rumdl check .— clean.uv run pytest -m unit -q— green (247 passed).docs/sources/MP/impact-report.mdnumbers reviewed and look sensible (regenerated against a fresh local build after the clique-typing fix).diseasebuild: HP→9606, MP→40674, mixed HP+MP cliques carry both.Disease.txt.🤖 Generated with Claude Code