Skip to content

feat(associations): LOINC clinical-measurement sections (#1349) - #1356

Draft
kevinschaper wants to merge 7 commits into
feat/flexible-association-section-matchingfrom
feat/loinc-clinical-measurement-sections
Draft

feat(associations): LOINC clinical-measurement sections (#1349)#1356
kevinschaper wants to merge 7 commits into
feat/flexible-association-section-matchingfrom
feat/loinc-clinical-measurement-sections

Conversation

@kevinschaper

Copy link
Copy Markdown
Member

Draft — stacked on #1355 (flexible section-matching enabler); base is that branch, so this diff is just the LOINC config. Retarget to main once #1355 merges.

What

Six AssociationTypeMapping entries for the LOINC clinical-measurement sections. Every LOINC edge carries the generic biolink:Association category, so each section is distinguished by predicate + object category (via the enabler):

key predicate object_category beta count
clinical_measurement_correlated_phenotypes correlated_with PhenotypicFeature 10,946
clinical_measurement_related_chemicals related_to ChemicalEntity 4,026
clinical_measurement_related_anatomy related_to AnatomicalEntity 5,025
clinical_measurement_related_organisms related_to OrganismTaxon 2,591
clinical_measurement_related_proteins related_to Protein 1,731
clinical_measurement_related_cells related_to Cell 444

All share category: [biolink:Association], subject_category: [biolink:ClinicalMeasurement].

  • Forward (on a clinical-measurement page): "Correlated Phenotypes", "Related Chemicals", etc.
  • Reverse (free, same mappings): "Correlated/Related Clinical Measurements" on the phenotype/chemical/anatomy/organism/protein/cell pages, via each mapping's object_label and the subject_category: ClinicalMeasurement filter that scopes the generic bucket.

No biolink enum changes needed — the composite filters use raw Solr clauses.

Verification

  • The exact composite filters each mapping produces were checked against api-beta and returned the edge-report counts above (e.g. correlated phenotypes = 10,946).
  • Mappings load with the right keys/labels/fragments; added a parser test proving clinical_measurement_correlated_phenotypes surfaces with its key + "Correlated Phenotypes" label. Counts-query fixtures regenerated; suite green (135).

Not in this PR (separate follow-ons)

  • biolink:ClinicalMeasurement entity-type support (regen category_enums.py + frontend color/icon) — sections work without it; only the node badge styling is default.
  • The LOINC subclass hierarchy widget (ontology structure, not an association section).
  • Per-row knowledge-level icons (related_to links are prediction-level).

https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL

Add six AssociationTypeMapping entries for the LOINC clinical-measurement
sections, built on the flexible section-matching enabler. All LOINC edges carry
the generic biolink:Association category, so each section is distinguished by
predicate + object category:

- clinical_measurement_correlated_phenotypes (correlated_with -> PhenotypicFeature)
- clinical_measurement_related_{chemicals,anatomy,organisms,proteins,cells}
  (related_to -> ChemicalEntity / AnatomicalEntity / OrganismTaxon / Protein / Cell)

Each mapping also drives the reverse "Related/Correlated Clinical Measurements"
section on the counterpart pages via its object_label. Composite fragments were
verified against api-beta (e.g. correlated phenotypes = 10946, matching the edge
report). Counts-query fixtures regenerated; added a parser test proving a section
surfaces with its key and label.

Sections render via the enabler without needing biolink enum changes;
biolink:ClinicalMeasurement entity-type styling + the hierarchy widget are
separate follow-ons.

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review: LOINC clinical-measurement sections

Reviewed the diff (backend/src/monarch_py/utils/association_type_mappings.yaml, regenerated test fixtures, and the new parser test) against the base branch's composite-matching enabler.

Overview

Adds 6 AssociationTypeMapping entries for LOINC clinical-measurement edges, all sharing the generic biolink:Association category and disambiguated by predicate + object_category. Clean, data-only change (no new code paths) that rides entirely on the existing composite-matching infrastructure.

Correctness

  • Verified get_solr_query_fragment/parse_association_counts (solr_parsers.py:170-242) key each result off the exact rebuilt fragment string, not just category, so the 6 new entries sharing biolink:Association don't collide with each other or the pre-existing single-category mappings — this is exactly the scenario the enabler was built for.
  • The 5 related_to mappings (chemicals/anatomy/organisms/proteins/cells) are disambiguated purely by object_category, and each value is distinct, so no fragment collisions there either.
  • Fixture diff checks out: the new clinical_measurement_correlated_phenotypes facet-query fragment is correctly interpolated at all suffix levels (direct_subject, closure_subject, direct_object, closure_object) in association_counts_query.py.

Minor observations (non-blocking)

  1. AssociationTypeMappings.get_mapping(category) (singular helper, used in solr_implementation.py:1091 for entity-grid direction inference) returns the first mapping matching a category. With 6 entries now sharing biolink:Association, a future call like get_mapping("biolink:Association") would silently resolve to clinical_measurement_correlated_phenotypes rather than None/an error. Not exercised today (grid configs don't use the generic category), but worth a docstring note or guard so a future caller doesn't get a surprising, arbitrary match.
  2. biolink:correlated_with isn't present in AssociationPredicate (category_enums.py), unlike biolink:related_to. Since predicate on AssociationTypeMapping is a plain list[str] (not enum-validated), this isn't a functional bug, but it's a small consistency gap if that enum is meant to be the canonical predicate list.
  3. Test coverage: the new test_parse_association_counts_clinical_measurement test is solid — it checks fragment construction, presence in the built query, and correct key/label/count parsing — but only covers clinical_measurement_correlated_phenotypes. The other 5 related_to mappings follow an identical code path differing only by object_category, so risk is low, but a quick parametrized assertion (e.g., all 6 keys present with expected object_category) would close the gap cheaply.

Performance

Each mapping added multiplies out across all 6 count-suffix variants in build_association_counts_query, so this bumps the per-node facet-query count in the association-counts request from ~138 to ~174 (23→29 mappings × 6 suffixes). That request fires on every node page, not just LOINC ones, so it's a ~26% increase in facet clauses for a section most entities won't have data for. Likely negligible for Solr, but worth keeping an eye on as more composite mappings get added to this same generic-category pattern.

Security

No concerns — config/data-only change, no new user input handling.

Process note

PR is correctly marked draft and stacked on #1355; just flagging that it shouldn't merge until the base is merged and the target is switched to main, as already noted in the description.

Overall: well-scoped, mechanically verified (counts checked against api-beta), and leans on solid enabler infrastructure. The two minor observations above are worth a look but don't block.

Add biolink:ClinicalMeasurement to the table-of-contents hierarchy allowlist
(and a heading label) so LOINC clinical-measurement pages render their
node_hierarchy (already returned by the backend) like other ontology terms.

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
…ection

Remove the clinical_measurement_related_organisms mapping. Those omop2obo
related_to -> OrganismTaxon edges only connect to a human-taxon node, which is
not a useful relationship to surface (and is slated for removal from the
ingest). Drops both the forward "Related Organisms" section and the reverse
section on the organism page. Fixtures regenerated.

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review: LOINC clinical-measurement association sections

Overview

Adds five AssociationTypeMapping entries for LOINC clinical-measurement edges (correlated phenotypes, related chemicals/anatomy/proteins/cells), each disambiguated from the shared generic biolink:Association category via predicate + object_category using the flexible list-valued matching from the stacked enabler PR (#1355). Includes a parser unit test, regenerated fixtures, and two small frontend additions (biolink:ClinicalMeasurement in the TOC category list and the hierarchy-widget label map).

Code quality / style

  • The YAML is clean and consistent with the file's existing conventions (the explanatory comment block mirrors the one already above the "Medical Action" entry).
  • Good use of the new key: field to give these generic-category entries a stable, human-readable section identifier instead of relying on category (which isn't unique here) — this is exactly the case the enabler was built for.
  • List-valued category/predicate/subject_category/object_category fields correctly match the schema introduced in feat(associations): flexible association-type section matching #1355.

Potential issue: ambiguous get_mapping() lookup by category

AssociationTypeMappings.get_mapping(category) in backend/src/monarch_py/utils/association_type_utils.py (~line 26) returns the first mapping whose category list contains the given value, with no tie-breaking on predicate/object_category:

@staticmethod
def get_mapping(category: str):
    """Get the first mapping that includes the given category."""
    ...
    for mapping in AssociationTypeMappings.__instance.mappings:
        if mapping.category and category in mapping.category:
            return mapping
    return None

This PR is the first to add multiple mappings that all share category: ["biolink:Association"]. It's called from the generic entity-grid path (solr_implementation.py, get_entity_grid, ~line 1091) to look up subject_category/object_category for direction inference. If that grid path is ever invoked with column_assoc_categories=["biolink:Association"], it will silently resolve to whichever LOINC mapping happens to be first in the YAML (clinical_measurement_correlated_phenotypes) regardless of which LOINC section was actually intended, and derive the wrong subject/object direction. It's not exercised by the association-table/counts code this PR actually adds (which goes through key-aware lookups), but it's a real sharp edge in shared global state that this PR is the first config to trigger. Worth a follow-up (disambiguate get_mapping by more than category, or document the limitation) even if it's out of scope for this PR itself.

PR description vs. diff mismatch

  • The description lists "The LOINC subclass hierarchy widget" under Not in this PR, but the third commit ("feat(node): show hierarchy widget for clinical measurements") adds biolink:ClinicalMeasurement to both CATEGORIES in TheTableOfContents.vue (which gates showHierarchy) and LABELS in SectionHierarchy.vue. As shipped, the hierarchy widget is enabled for ClinicalMeasurement nodes. Worth double-checking whether this was intended to land here — if so, the description should be updated; if not, it looks like an easy commit to split out.
  • The description's table still lists clinical_measurement_related_organisms (2,591 edges), but that mapping was dropped in the third commit (confirmed clean — no leftover references anywhere in the diff). The description/table should be updated to reflect the final 5 mappings before this comes out of draft.

Test coverage

  • The added test_parse_association_counts_clinical_measurement is solid — it verifies the built Solr fragment, checks it's actually part of the composite counts query, and asserts the parsed label/key. Good use of the shared code path, so one representative test covers the general mechanism.
  • That said, only 1 of 5 new mappings is exercised (the correlated_with predicate one); the other four all share related_to and differ only in object_category, and the reverse-direction label (object_label, e.g. "Correlated/Related Clinical Measurements" on the counterpart pages) isn't covered by any test. Since the reverse mapping is one of the two behaviors highlighted in the PR description, a small additional case asserting the object-side label surfaces correctly would strengthen confidence here.

Performance / security

  • Pure config change plus two one-line frontend additions; negligible performance impact (5 more facet queries in an already-large composite counts query). No security concerns — no user input involved, static YAML/label data only.

Summary

Solid, well-scoped config PR that clearly demonstrates the new flexible section-matching mechanism. Main things worth resolving before merge: reconcile the "not in this PR" hierarchy-widget claim with the actual diff, update the stale organisms row in the description, and consider a follow-up (or at least a comment) on the get_mapping() first-match ambiguity now that multiple sections share the generic biolink:Association category.

Add a knowledgeLevel.ts meta util (mirroring agentType.ts) mapping each biolink
KnowledgeLevelEnum value to an icon/label/description, and render a
knowledge-level icon next to the agent-type icon in the associations table
actions cell. Gives an at-a-glance curated-vs-predicted signal per row (e.g.
LOINC omop2obo related_to edges read as "Prediction" vs loinc2hpo's
"Knowledge Assertion").

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
@claude

claude Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review

Reviewed the diff for PR #1356 (gh pr diff 1356, base = feat/flexible-association-section-matching per the stacked-PR setup).

Overview

Adds 5 AssociationTypeMapping entries for LOINC clinical-measurement sections (correlated phenotypes, related chemicals/anatomy/proteins/cells), plus a getKnowledgeLevelMeta util and per-row knowledge-level icon in AssociationsTable.vue, plus a one-line SectionHierarchy/TheTableOfContents hookup for biolink:ClinicalMeasurement.

Code quality

  • association_type_mappings.yaml — the new entries correctly use explicit key: to disambiguate sections that all share the generic biolink:Association category (the field's docstring says it defaults to category when unset, which would collide across all 6 new entries without this). Good catch by the author.
  • frontend/src/util/knowledgeLevel.ts is a clean, near-exact mirror of the existing agentType.ts pattern (same shape: *_META record, *_KEYS, FALLBACK, get*Meta, format*). Nice consistency, and the AssociationsTable.vue wiring (icon + tooltip + aria-label) mirrors the existing agent_type block line-for-line.
  • All new FontAwesome icons (circle-check, sitemap, chart-bar, equals, eye, circle-question) are already registered in frontend/src/global/icons.ts, so nothing will silently fail to render.
  • KNOWLEDGE_LEVEL_META keys (knowledge_assertion, logical_entailment, prediction, statistical_association, observation, not_provided) match the Biolink KnowledgeLevelEnum exactly.

PR description is out of sync with the diff

  • The description's table lists 6 mappings including clinical_measurement_related_organisms (2,591 edges), but the actual diff only contains 5 entries — related_organisms isn't there. The commit log confirms it was intentionally dropped in a later commit ("drop the Related Organisms clinical-measurement section"), but the PR body/table wasn't updated to match.
  • The "Not in this PR" section explicitly lists "Per-row knowledge-level icons ... " as a separate follow-on, but the diff does add this (knowledgeLevel.ts + the AssociationsTable.vue wiring), per the commit "feat(associations): add knowledge-level icons to the association table". Worth updating the description so reviewers aren't misled about scope, or splitting that commit into its own PR if it's meant to be independent/follow-on work.

Potential correctness question (not blocking, but worth double-checking)

  • related_chemicals, related_proteins, and related_cells all use predicate related_to and are distinguished only by object_category (ChemicalEntity / Protein / Cell). If any LOINC edge's object_category field in Solr contains multiple ancestor categories (e.g. an entity tagged with both biolink:Protein and biolink:ChemicalEntity), the same edge could double-count across two sections. The PR notes counts were verified against api-beta, so this is likely already accounted for — just flagging as something to sanity-check if any of these counts look inflated relative to the underlying LOINC source data.

Test coverage

  • The new parser test (test_parse_association_counts_clinical_measurement) only exercises clinical_measurement_correlated_phenotypes. The other 4 mappings share the same shape modulo object_category, so risk is low, but a quick parameterized check over all 5 keys (or at least one more with related_to instead of correlated_with) would give more confidence that the predicate-based disambiguation works for the more common case too.
  • Good to see fixtures were regenerated and the suite is green per the description.

Frontend

  • TheTableOfContents.vue and SectionHierarchy.vue changes are minimal, additive, and don't affect existing categories — low risk.
  • Minor/pre-existing pattern (not introduced by this PR): getKnowledgeLevelMeta(row.knowledge_level) is called 4x per row in the template (label, description, aria-label, icon). Same as the existing agent_type block, so not a regression, just noting it's cheap to memoize per-row if this ever becomes a hot path.

Security

No concerns — no user input reaches the new Solr filters (mappings are static config), and no new external data handling was introduced.

Overall this is a clean, well-scoped, low-risk change that closely follows existing conventions. Main actionable item is syncing the PR description with the actual diff (dropped related_organisms section, and the knowledge-level icon feature that's actually included despite being listed as a follow-on).

Give biolink:ClinicalMeasurement a first-class type identity: a test-tube glyph
(category-clinical-measurement.svg) and a teal color in the category color map,
instead of the gray "CM" initials fallback. Applies on LOINC node pages and in
association-table node badges.

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review

Nicely scoped addition to the association-type-mapping system, and good to see fixture regeneration plus a parser test exercising the "shared biolink:Association category, disambiguated by predicate + object_category" case. A few things worth a look before this leaves draft:

🐛 Missing mapping: clinical_measurement_related_organisms

The PR description's table lists six mappings, including:

key predicate object_category beta count
clinical_measurement_related_organisms related_to OrganismTaxon 2,591

...but association_type_mappings.yaml only adds five entries (phenotypes, chemicals, anatomy, proteins, cells) — there's no OrganismTaxon entry anywhere in the diff. Confirmed by grepping the regenerated association_counts_query.py fixture too: it has facet queries for PhenotypicFeature, ChemicalEntity, AnatomicalEntity, Protein, and Cell, but nothing for OrganismTaxon. Looks like the sixth mapping was dropped somewhere between drafting the description and writing the YAML — worth adding before this comes out of draft, since the "Related Organisms" section (and its reverse on OrganismTaxon pages) currently won't appear at all despite being verified against api-beta.

📝 PR description vs. diff mismatch

Two items listed under "Not in this PR (separate follow-ons)" actually are wired up in this diff:

  • Per-row knowledge-level iconsfrontend/src/util/knowledgeLevel.ts (new) plus its integration into AssociationsTable.vue's row-icon column is fully implemented and unit-tested (frontend/unit/knowledgeLevel.test.ts). This looks like solid, general-purpose work (not LOINC-specific), but it contradicts the description's claim that it's deferred.
  • LOINC hierarchy widgetTheTableOfContents.vue adds "biolink:ClinicalMeasurement" to its CATEGORIES array, which directly flips on showHierarchy and renders SectionHierarchy.vue for clinical-measurement nodes; SectionHierarchy.vue's LABELS map is updated to match ("Clinical measurement"). The description says this widget is explicitly out of scope.

Neither of these looks broken, but the description should be updated (or the changes should be split out) so reviewers know the actual scope. Worth double-checking that node_hierarchy.super_classes/sub_classes are actually populated for LOINC nodes in api-beta — if not, the hierarchy widget will just render an empty parents/children panel for clinical-measurement pages, which is harmless but untested per the PR notes.

Nice touches

  • colors.teal.dark and the new category-clinical-measurement.svg are wired through the existing getCategoryIcon/getCategoryColor convention (category- + kebab-case category name) correctly, so the new icon/color will resolve at runtime.
  • Reusing the same mapping key to drive both the forward (clinical-measurement page) and reverse (phenotype/chemical/etc. page) sections is a clean way to avoid duplicating config, and the direction is safely disambiguated since subject_category (ClinicalMeasurement) and the various object_category values are disjoint — no risk of the two directions colliding for a single node.
  • symmetric correctly defaults to false here, which is right since subject (ClinicalMeasurement) and object (Phenotype/Chemical/etc.) categories aren't interchangeable, even though correlated_with/related_to are symmetric predicates at the biolink-model level.

Test coverage

Only clinical_measurement_correlated_phenotypes gets a dedicated parser test; the other four (and the still-missing organism one) rely on the same code path so risk is low, but a quick parametrized test over all six keys would make regressions in the YAML (like the missing entry above) much easier to catch automatically rather than by manual diff review.

Overall: solid mechanism, just needs the missing organism mapping added and the PR description reconciled with what's actually in the diff before merging.

Register monarch-initiative/loinc-ingest so `make fetch-docs` generates the
/Sources/loinc/ page. The association "Provided By" link for LOINC edges
(get_provided_by_link -> .../Sources/loinc/) currently 404s because there was
no manifest entry.

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
@claude

claude Bot commented Jul 10, 2026

Copy link
Copy Markdown

Review

Reviewed the diff (this PR is stacked on #1355 and marked as draft, so noting a couple of things that will matter before it is marked ready).

Bug: missing "Related Organisms" mapping

The table in the PR description lists six mappings, including clinical_measurement_related_organisms (predicate related_to, object_category OrganismTaxon, beta count 2,591).

But backend/src/monarch_py/utils/association_type_mappings.yaml only adds five entries (correlated_phenotypes, related_chemicals, related_anatomy, related_proteins, related_cells) — there is no clinical_measurement_related_organisms entry, and no other reference to OrganismTaxon/organism anywhere in the diff. If the 2,591-edge count is real, this section (and its reverse "Related Clinical Measurements" on OrganismTaxon pages) will not render. Worth double-checking whether this was intentionally dropped or just missed — the description should be updated either way so it matches what is actually shipped.

Description vs. diff mismatch

The "Not in this PR" section says the biolink:ClinicalMeasurement entity-type support (frontend color/icon) and per-row knowledge-level icons are out of scope for this PR.

But the diff actually includes both: frontend/src/api/categories.ts adds a biolink:ClinicalMeasurement -> colors.teal.dark entry plus a new category-clinical-measurement.svg icon (registered in TheTableOfContents.vue and SectionHierarchy.vue), and a new frontend/src/util/knowledgeLevel.ts (with frontend/unit/knowledgeLevel.test.ts) wired into a per-row knowledge-level icon in AssociationsTable.vue.

Neither of these looks broken — all the icon names used (circle-check, sitemap, chart-bar, equals, eye, circle-question) are already registered in frontend/src/global/icons.ts, and AppIcon.vue dynamic import (../assets/icons/${kebabCase(icon)}.svg) will correctly resolve category-clinical-measurement.svg. So this looks like a stale/inaccurate PR description rather than a code issue, but worth updating before merge so reviewers are not misled about scope.

Everything else looks solid

  • The five mappings that are present are internally consistent (category: biolink:Association + predicate + subject_category: biolink:ClinicalMeasurement + object_category), matching the "generic category, disambiguate via predicate + object_category" pattern already used elsewhere in the file (e.g. the Case* mappings above it).
  • The new parser test (test_parse_association_counts_clinical_measurement) is a good regression check — it validates both that the composite Solr fragment is actually part of the built counts query and that parsing surfaces the right key/label, which is exactly the thing that is easy to get subtly wrong with generic-category disambiguation.
  • knowledgeLevel.ts mirrors the existing agentType.ts pattern closely (same shape, same not_provided/fallback handling), so it is consistent with existing conventions rather than introducing a new one.
  • Regenerated fixtures (association_counts_query.py/_response.py) look like straightforward mechanical regen from the mapping change, not hand-edited.

Minor/nit

The YAML comment above the new mappings says these edges all carry the generic biolink:Association category — accurate for the 5 present, but if the organisms mapping is added back in, remember to update the comment/table together so they do not drift again.

No security or performance concerns — this is config/data plus additive, low-risk frontend rendering code.

getKnowledgeLevelMeta covers all callers (the association-row indicator); the
label-only formatKnowledgeLevel had no app usage. Remove it and its test.

Claude-Session: https://claude.ai/code/session_018c9UddtKsKtm35YNrxHWuL
@kevinschaper

Copy link
Copy Markdown
Member Author

Note for reviewers: this PR also introduces the knowledge-level indicator (icon column on every association row, frontend/src/util/knowledgeLevel.ts + getKnowledgeLevelMeta in AssociationsTable.vue). It's logically independent of the LOINC clinical-measurement sections but rides along here; flagging it explicitly so the scope is intentional rather than an oversight.

@claude

claude Bot commented Jul 11, 2026

Copy link
Copy Markdown

Review

Nice, focused PR — the mapping composite-filter approach for disambiguating LOINC's generic biolink:Association edges is a clean reuse of the flexible-matching enabler from #1355, and knowledgeLevel.ts is a tidy mirror of the existing agentType.ts pattern.

🐛 Likely bug: "Related Organisms" section is missing

The PR description's table lists six mappings, including:

key predicate object_category beta count
clinical_measurement_related_organisms related_to OrganismTaxon 2,591

But backend/src/monarch_py/utils/association_type_mappings.yaml only adds five entries — correlated_phenotypes, related_chemicals, related_anatomy, related_proteins, related_cells. There is no OrganismTaxon mapping anywhere in the file (verified via grep). If that's accurate, ~2,591 clinical-measurement↔organism edges from beta won't get a section at all. Worth double-checking this wasn't dropped accidentally before merge.

📝 PR description drifted from the diff

The "Not in this PR (separate follow-ons)" section says per-row knowledge-level icons and biolink:ClinicalMeasurement color/icon are out of scope, but the diff actually includes both:

  • frontend/src/util/knowledgeLevel.ts + wiring into AssociationsTable.vue (knowledge-level icon/tooltip per row)
  • colorMap["biolink:ClinicalMeasurement"], the new category-clinical-measurement.svg, and TOC/SectionHierarchy label entries

Neither addition looks wrong, but they read as unrelated scope creep relative to the stated LOINC-config-only intent. Might be worth splitting into separate PRs, or just updating the description so reviewers (and the eventual changelog) aren't misled.

⚠️ Latent ambiguity in AssociationTypeMappings.get_mapping()

association_type_utils.py's get_mapping(category) returns "the first mapping that includes the given category" and is used by the generic entity-grid builder (solr_implementation.py:1091) to infer subject/object direction. All five new LOINC mappings share the generic category: biolink:Association and are only actually distinguished by predicate/object_category. The counts-parsing path (solr_parsers.py) already had to work around this exact ambiguity by rebuilding the full composite fragment per mapping (nice comment there explaining why). get_mapping() has no equivalent safeguard — a future caller building an entity grid over biolink:Association would silently always resolve to clinical_measurement_correlated_phenotypes regardless of which section was actually meant. Not exercised by this PR's own code, but worth a note/TODO for whoever wires clinical measurements into the entity grid next.

Minor / performance

build_association_counts_query iterates all mappings × all direction suffixes unconditionally for every entity page (pre-existing pattern, not introduced here). Adding 5 more mappings adds ~30 more facet queries to every entity page's counts request app-wide, not just LOINC pages. Probably fine given Solr facet queries are cheap and this is already how ~30 other mappings work, but the regenerated fixture makes the growth pretty visible — might be worth revisiting if the mapping list keeps growing.

Test coverage

  • Good: the new parser test validates the correlated_with path end-to-end (fragment building → facet query → parsed result), and fixtures were regenerated with the suite green.
  • The related_to path (chemicals/anatomy/proteins/cells) shares the same generic code, so a dedicated test per section isn't strictly necessary — but note that a test asserting all six documented keys are actually loaded from the yaml would have caught the missing-organisms issue above.
  • No frontend test added for the knowledge-level icon rendering in AssociationsTable.vue, but that's consistent with how agent_type icon rendering isn't tested either, so not a new gap.

Nits

  • Icons used in knowledgeLevel.ts (circle-check, sitemap, chart-bar, equals, eye, circle-question) are all already registered in frontend/src/global/icons.ts — confirmed no missing-icon issue.
  • No security concerns: all new data flows through typed enum/model fields (knowledge_level, category mappings), not raw user input, and the new tooltip interpolates only fixed label/description strings — same pattern already used for agent_type.

Overall solid work; the organisms-mapping gap is the one thing I'd want resolved (or confirmed intentional) before this comes out of draft.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant