Skip to content

Add MP, kept disjoint from HP, with an xref allowlist and HP/MP taxa - #886

Merged
gaurav merged 78 commits into
mainfrom
mp-hp-disjoint
Jul 10, 2026
Merged

Add MP, kept disjoint from HP, with an xref allowlist and HP/MP taxa#886
gaurav merged 78 commits into
mainfrom
mp-hp-disjoint

Conversation

@gaurav

@gaurav gaurav commented Jun 30, 2026

Copy link
Copy Markdown
Collaborator

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.

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 glom: unique_prefixes tie-break for a contested cross-reference ignores evidence strength #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 Add MP (Mammalian Phenotype Ontology) as a disease/phenotype source #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 Add MP (Mammalian Phenotype Ontology) as a disease/phenotype source #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:

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 Prefix comparisons that can never match: unify Text's three prefix conventions #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 Prefix comparisons that can never match: unify Text's three prefix conventions #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 pull_uber_synonyms() silently wrote empty per-prefix UberGraph synonym files #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 (glom: unique_prefixes tie-break for a contested cross-reference ignores evidence strength #894) — no unrelated churn,
    so it doubles as a completeness check on the reimplementation.

🤖 Generated with Claude Code

gaurav and others added 22 commits June 30, 2026 17:58
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>
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>
- 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>
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>
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>
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>
SMEs require that no clique contain both a Human Phenotype (HP) and a Mammalian
Phenotype (MP) identifier. glom()'s unique_prefixes cannot enforce this (it only
forbids duplicate same-prefix ids), and dropping MP's own concord is insufficient
because the HP and EFO concords emit direct HP/EFO -> MP xref rows (plus transitive
bridges via MESH/SNOMED/MONDO).

Add split_mutually_exclusive_cliques(dicts), driven by the new
MUTUALLY_EXCLUSIVE_PREFIX_GROUPS = [[HP, MP]] constant: after glom, any clique holding
both HP and MP is split so the HP side (plus all non-group members like MONDO/MESH)
stays intact and MP is peeled into its own clique. MP may still merge with non-HP
disease ids; only HP triggers a split.

Call it as the last step of compute_cliques_for_impact_report so BOTH the real build
and the source-impact report (which diffs these dicts) see identical, already-split
cliques. Typing is unaffected: the peeled MP-only clique types as PhenotypicFeature and
the HP/MONDO side keeps its pre-split type.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The HP/MP split can strand a lone identifier that is referenced in a concord but
absent from every ids file (e.g. an obsolete MP not under MP:0000001) — in the local
build, exactly one such MP. Before the split it rode along inside a typed HP clique;
after the split it becomes an untypeable singleton, and create_typed_sets' RuntimeError
guard aborted the entire disease build over that one stray.

Make create_typed_sets log a warning and skip an untypeable clique instead of raising:
a clique with no member carrying a declared type cannot be assigned a Biolink type and
cannot be emitted anyway, so dropping one stray is the right behavior and far less
brittle than failing the whole build. Adds a module logger via get_logger.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Reran source-impact-report --source MP after the HP/MP split. Modified (expanded)
cliques drop from 223 to 15 -- and none are HP-led -- because MP no longer merges
into HP cliques; the 15 remaining are MP joining non-HP phenotype cliques (e.g. EFO),
which "separate from HP only" still allows. 14,641 pure-new MP-only cliques are added.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tern

Update AddingNewSources.md and CLAUDE.md with what this session established:

- When to use babel-clique-diff (restructuring changes: split/move/delete) vs the
  source-impact report (add-a-source: new/expanded/merged), the build-both-from-the-
  same-intermediates discipline, and the commit-location convention for its outputs
  (docs/sources/<SOURCE>/<change>/ or docs/pipelines/<pipeline>/<change>/; always the
  summary JSON, the CSV when reasonably sized).
- glom()'s unique_prefixes does NOT keep two different prefixes apart; use a post-glom
  split (split_mutually_exclusive_cliques / MUTUALLY_EXCLUSIVE_PREFIX_GROUPS).
- create_typed_sets drops untypeable cliques (concord-only, out-of-date ids) with a
  warning instead of aborting the build.
- PIPELINE_CONFIG entries need clique_classifier/biolink_types/compendium_prefixes,
  not just compute_fn; the HP/MP fixed-taxon-from-ids pattern; index the HP/MP docs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Base automatically changed from add-clique-diff-tool to main July 2, 2026 04:51
gaurav and others added 2 commits July 2, 2026 00:52
write_mp_ids duplicated the walk write_obo_ids already does (same
pattern write_hp_ids uses), so switch to it and drop the now-unused
UberGraph import. Also avoid a redundant set copy per peel in
split_mutually_exclusive_cliques.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HP:0000048 and UMLS:C0341787 already clique correctly via the HP and UMLS
concords, independent of MP -- no explicit xref is needed. What is worth
pinning is that MP:0009203 stays out of that clique while the other three
members stay in it.

Issue #906 carries the executable assertions (DoesNotResolveWith for the MP
term, ResolvesWith for HP/UMLS so a fix cannot over-split), which babel-validation
runs against live NodeNorm well after this PR closes the issue. The unit test and
mp_badxrefs.txt now point at it.

Closes #906

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@gaurav gaurav changed the title Add MP, kept disjoint from HP Add MP, kept disjoint from HP, with an allowlist for its xrefs Jul 10, 2026
Comment thread docs/sources/MP/mappings.md Outdated
gaurav and others added 2 commits July 10, 2026 00:25
"Why an allowlist rather than an ignore_list" was an H3 under "## Bad-xref
file", which it has nothing to do with. Move it up under the allowlist section
it explains.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment thread input_data/mp_badxrefs.txt
Comment thread input_data/mp_badxrefs.txt
Three lessons from the MP xref review, written down so the next source ingest
doesn't relearn them:

- CLAUDE.md: many ontologies use oboInOwl:hasDbXref to mean "is about", not "is
  equivalent to"; audit a new source's target-prefix breakdown before trusting
  it. Records build_sets()' ignore_list (fails open) vs allowed_prefixes (fails
  closed), the upper-casing footgun in Text.get_prefix_or_none(), and that
  bad-xref files are keyed by concord basename in two places that must agree.
- AddingNewSources.md: a clean section 4 ("0 cliques merged") does not mean the
  xrefs are good -- an xref can be inert rather than correct. Adds the two-gate
  recipe (does the target prefix appear in another concord? is it in the clique
  type's Biolink id_prefixes?) for deciding whether filtering a prefix is a safe
  cleanup or a behavioral change needing a build-vs-build diff.

Both shell/Python snippets verified against babel_outputs/intermediate/disease.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 35 out of 37 changed files in this pull request and generated 3 comments.

Comment thread src/datahandlers/obo.py
Comment thread src/datahandlers/efo.py Outdated
Comment thread tools/clique_diff/diff.py Outdated
gaurav and others added 2 commits July 10, 2026 00:57
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@gaurav gaurav changed the title Add MP, kept disjoint from HP, with an allowlist for its xrefs Add MP, kept disjoint from HP, with an xref allowlist and HP/MP taxa Jul 10, 2026
gaurav and others added 5 commits July 10, 2026 01:14
… split

split_mutually_exclusive_cliques() peeled every prefix after group[0] rather than
every prefix after the earliest occupied one. For a group whose first prefix is
absent from a clique, that peeled all the occupied prefixes and stranded the
clique's out-of-group members in a singleton. Unreachable with [[HP, MP]], but
wrong; keep the earliest occupied prefix with the remainder instead.

It also compared CURIE prefixes with a raw split(":"), while build_sets()
compares against Text.get_prefix_or_none(), which upper-cases. A lower-case
group constant (prefixes.ORPHANET is "orphanet") would have silently never
matched and failed open -- the worst direction for a disjointness rule. Match
case-insensitively, as build_sets() effectively does.

create_typed_sets() now logs a total alongside its per-clique warnings, so a
mass drop (an ids file that silently failed to build) is visible at a glance
rather than only by counting log lines.

DEFAULT_BAD_XREFS is anchored at the repo root: Snakemake always runs from
there, but the source-impact report CLI need not.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
_is_excluded_target() rebuilt the upper-cased set on every result row; it is
called once per xref/exactMatch across all of EFO. Hoist it into the callers,
which run it once per query.

Also switch a stray logging.warning() to the module logger, per CLAUDE.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Both call UberGraph the same way; only MP's rule had retries: 3.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…uple shim

diff_percent is None (JSON null) when the before build had no cliques but the
after build has some -- the percentage is undefined there, and 0.0 would read as
"unchanged". Neither the module docstring nor docs/tools/README.md said so, and
no test covered it. Both now do.

LoadedCompendium.__iter__ existed to preserve a 2-tuple unpacking that no caller
outside its own test performs any more.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
ignore_list and allowed_prefixes are compared against Text.get_prefix_or_none(),
which upper-cases, so a non-upper-case entry silently never matches. For
allowed_prefixes that fails closed (an empty concord, which you notice); for
ignore_list it fails open, and the junk it was meant to block is written and
glommed as an equivalence. Nothing warns.

The mistake is easy to make because 13 of the canonical constants in prefixes.py
are not upper-case (orphanet, UniProtKB, NCBIGene, ...), so using the named
constant -- which CLAUDE.md tells you to do -- is precisely what breaks. Raise
ValueError at the call instead of documenting the landmine.

Every live caller already satisfies this. Narrow, boundary-only version of
#907; the Text redesign belongs
in its own PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t it

_write_mp_concord() called build_sets() without allowed_prefixes, so the MP
pipeline test wrote the raw 663-row xref dump -- anatomy, processes, PMIDs,
Wikipedia URLs -- to babel_outputs/intermediate/disease/concords/MP, the same
stable path the real build uses. A later Snakemake run would treat that
unfiltered file as up to date and glom category-error xrefs as equivalences.
It also meant the allowlist, the whole point of MP's ingest, went untested by
the one test that talks to a live UberGraph.

Pass MP_XREF_ALLOWED_PREFIXES, matching build_disease_obo_relationships(), and
assert in test_mp_concords_include_external_mappings that every target prefix is
allowlisted -- which is what will catch an upstream MP release that starts
emitting a new namespace.

Verified against a live UberGraph: 663 rows -> 78 (MGI 70, MPATH 4, UMLS 2,
HP 2), content-identical to the concord the real build produced.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@gaurav
gaurav merged commit d27670d into main Jul 10, 2026
5 checks passed
@github-project-automation github-project-automation Bot moved this from Backlog to Done in Babel sprints Jul 10, 2026
@gaurav
gaurav deleted the mp-hp-disjoint branch July 10, 2026 05:34
gaurav added a commit that referenced this pull request Jul 10, 2026
main (#886) rewrote the clique-diff while this branch was moving it, so the
conflicts are all "same code, two places". Resolved by keeping main's logic and
re-applying this branch's split:

- src/model/compendium_diff.py: take main's `moved`-destination resolution
  (after_location maps each CURIE to (filename, leader), DROPPED_KEY) and its
  null diff_percent. Drop MOVED, now unused. Hoisted CSV_COLUMNS gains
  destination_label/destination_compendium; the load_cliques docstring loses the
  stale 2-tuple note, since main deleted LoadedCompendium.__iter__.
- src/tools/clique_diff/cli.py: main's write_csv changes (sort key now includes
  destination_compendium) land in the CLI, where write_csv now lives.
- docs/tools/CliqueDiff.md: absorbs the "Reading a row" and diff_percent prose
  main added to the old combined docs/tools/README.md.
- docs/sources/MP/disjointness.md: main's same-concords Impact rewrite, with this
  branch's link retargeted to CliqueDiff.md.
- tests: main's diff_percent tests go to tests/model/, its CLI test is already
  covered by tests/tools/clique_diff/test_cli.py.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gaurav added a commit that referenced this pull request Jul 10, 2026
PR #886 (MP ingestion) refactored build_compendium() into
compute_cliques_for_impact_report() and, in doing so, deliberately
carried the MONDO close-match bug forward with a NOTE deferring the fix
to this PR. Resolve by keeping all of #886's structure and applying this
PR's fix inside it:

- Keep compute_cliques_for_impact_report()'s excluded_sources handling,
  MONDO_close basename discovery, DISEASE_UNIQUE_PREFIXES,
  OVERUSE_FILTERED_CONCORDS (now including MP), the post-glom
  HP/MP split, and the logger calls.
- Route its two inline parse loops through this branch's
  load_close_mondos() and load_concord_pairs() helpers, so the
  close-match reader keys on the object (column 3), not the predicate.
- Drop the NOTE deferring the fix; this is that follow-up.
- Preserve #886's None-guard: an absent MONDO_close (impact-report CLI,
  or --source MONDO) still yields an empty close map rather than
  opening a missing file.

Both sides' tests are retained; the test file gains 164 lines and loses
none. Reword the now-stale "Related work" bullet in
docs/sources/MP/disjointness.md, which described this fix as still
deferred.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
gaurav added a commit that referenced this pull request Jul 21, 2026
At the end of a full Babel run, this generates a tabular comparison of
CURIE and clique counts in the current build against the previous
release — reproducing in-repo what was previously done by hand in
babel-validation's `PrefixComparator.vue`.

Two things happen:

1. **Revive the combined `prefix_report.json` (efficiently).** A single
`generate_prefix_report` rule now emits the combined `{name,
count_curies, count_cliques, by_clique, by_curie_prefix}` report — the
schema the committed `releases/prefix_reports/*.json` baselines already
use. This is the *output* of the old `generate_prefix_report` (PR #363),
which was removed because it OOM-killed the full graph on two
non-spillable operators (`COUNT(DISTINCT)` and `STRING_AGG`). We rebuild
it with only spillable `GROUP BY <prefix>` + `approx_count_distinct`
(the technique its successors already used), and the rule replaces both
`generate_curie_report` and `generate_clique_leader_report`, so the Edge
Parquet is scanned **once instead of twice**. Occurrence counts stay
exact; only distinct/clique counts are approximate (HLL).

2. **Compare against the previous release.**
`generate_prefix_comparison` (wired into `all_reports`) diffs the build
against the baseline pinned by `previous_release` in `config.yaml`,
writing to `babel_outputs/reports/tables/`:
- `prefix_comparison_overall.csv` — All CURIEs, All cliques, and
per-filename CURIE totals, each with absolute and percentage change.
- `prefix_comparison_by_clique_prefix.csv` — one row per `(filename,
clique-leader prefix, CURIE prefix)`, sorted by absolute change (largest
first), e.g. *"1,108,267 fewer INCHIKEY identifiers in SmallMolecule
cliques led by PUBCHEM.COMPOUND"*.
- `prefix_comparison.md` — names the exact baseline compared against and
flags notable drifts (removals, or beyond the configured abs/pct
thresholds).

The baseline is pinned explicitly (no date heuristic). A unit test fails
if a newer baseline was committed to `releases/prefix_reports/` without
bumping `previous_release`, so the weekly unit run catches drift before
the next build starts. Drifts are **flagged, not gated** — the build
never fails on them.

## Notes for the release manager

- Set `release_name` in `config.yaml` to the actual release date before
cutting a release.
- After a healthy run, follow the new archiving steps in
`docs/RunningBabel.md`: copy `prefix_report.json` to
`releases/prefix_reports/<release_name>.json` and bump
`previous_release` in the same commit.

## Issues

- Closes #443 (move the prefix-count comparison in-repo, auto-comparing
against the most recent release).
- Substantially addresses #743 (per-release reports tracked in-repo + a
structured fresh-vs-prior diff). The "hard-gate publication on a
threshold breach" part is intentionally left as flag-only; leaving the
issue open to track that.
- Partially addresses #764 (per-prefix/per-clique diff + threshold
flagging). The broader per-compendium regression baseline and
`RELEASE.md` checklist remain; leaving it open.

## Base branch

Stacked on `mp-hp-disjoint` (#886); the diff here is only the two
prefix-comparison commits.

## Verification

- 280 unit tests pass (incl. new
`tests/reports/test_prefix_comparison.py` and updated
`test_duckdb_reports.py`).
- `ruff check`, `ruff format --check`, `snakefmt --check`, `rumdl check
.` all clean.
- Ran the comparison on the real committed baselines (2025mar31 →
2025sep1) and confirmed the expected two-table output.

🤖 Generated with [Claude Code](https://claude.com/claude-code)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

2 participants