Add medical imaging and biological imaging methods value sets

[cmungall]

Summary

This PR adds comprehensive value sets for medical imaging data management and biological imaging methods, supporting DICOM data extraction, validation, de-identification, and research workflows.

Key Changes

New Medical Imaging Value Sets

src/valuesets/schema/medical/dicom.yaml - DICOM Standard Value Sets

• DICOM acquisition modality codes (AR, CT, MR, US, etc.) from PS3.16 Context ID 29
• Value Representations (VRs) defining DICOM attribute data types and encoding
• Attribute requirement types (Type 1, 1C, 2, 2C, 3) for IOD conformance checking
• Registered transfer syntaxes for byte ordering and pixel data compression
• DIMSE and DICOMweb network services for PACS query and retrieval
• Open-source DICOM toolkits (pydicom, dcmread, etc.)

src/valuesets/schema/medical/imaging_deidentification.yaml - De-identification Value Sets

• DICOM PS3.15 de-identification profiles and options (Basic, Clean, Retain variants)
• De-identification action codes (D, Z, X, K, C, U) specifying attribute treatment
• Image de-identification targets (header metadata, private attributes, pixel data, overlays)
• Regulatory frameworks (HIPAA, GDPR, PIPEDA)
• Defacing methods and tools for removing facial features from head CT/MRI

src/valuesets/schema/medical/imaging_data_management.yaml - Data Management Value Sets

• Imaging data lifecycle stages (extraction, integrity verification, conformance checks, validation, de-identification)
• Source system types (PACS, VNA, departmental archives, research platforms, EHR, cloud)
• DICOM validation issue categories (structural, semantic, plausibility)
• Image quality metrics (SNR, CNR, spatial resolution, artifact detection)
• Interoperability standards (DICOM, HL7 FHIR, LOINC, SNOMED CT)
• Bridge2AI data generation projects

New Biological Imaging Value Sets

src/valuesets/schema/bio/biological_imaging_methods.yaml - FBbi-based Imaging Methods

• Dynamic enumerations populated from the Biological Imaging Methods Ontology (FBbi)
• Nine top-level branches: sample preparation, visualization, imaging method, illumination, detection, imaged parameter, contrast enhancement, resolution enhancement, source of contrast
• Specialized value sets for microscopy methods (general, light, fluorescence)
• Tissue fixation methods and sample preparation techniques
• Visualization methods (stains, probes, labels)
• Detection and illumination methods
• Contrast and resolution enhancement techniques

Schema Integration

• Updated src/valuesets/schema/valuesets.yaml to import new medical imaging modules
• Updated src/valuesets/validators/oak_config.yaml to add FBbi ontology adapter for dynamic enum validation

Implementation Details

• All DICOM codes preserved in standard form (not uppercased) per DICOM conventions
• Extensive use of ontology mappings: NCIT for medical concepts, DCM for DICOM codes, FBbi for imaging methods
• Compound de-identification action codes use underscores instead of slashes for YAML compatibility
• Dynamic enumerations for biological imaging methods use reachable_from to populate from FBbi ontology
• Comprehensive descriptions and see_also links to DICOM standards and ontology resources
• Contributor attribution and status tracking (DRAFT) for all enums

https://claude.ai/code/session_01JqKeWA6q8xdJhgpuzHE4Ym

[claude]

Claude encountered an error after 0s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @cmungall's task in 6m 47s —— View job

---

Code review: medical imaging + biological imaging value sets

• Read the four new schema files and the two config changes
• Check against repo conventions (CLAUDE.md, existing dynamic-enum files)
• Trace how just validate / enum_evaluator treats the new prefixes and mappings
• Attempt to verify CURIEs (blocked — see note at the end)
• Post review

Overall this is a well-organized, well-documented addition. Structure, headers, contributors/status/instantiates blocks and the obo:/rdfs: usage in reachable_from all match existing files (bio/bio_entities.yaml, medical/imaging_platforms.yaml). Enum and slot names don't collide with anything else in the merged schema, YAML is well-formed, and the DICOM content I can check from memory (VR list is complete, transfer-syntax UIDs, PS3.15 action-code semantics, CID 7050 codes 113100–113112) looks right. Comments below, roughly in priority order.

---

1. The new ontology IDs are not verified by anything in this repo

CLAUDE.md says "ALWAYS check IDs, NEVER guess them", and here the automated safety net doesn't apply:

• reachable_from.source_nodes is never validated. enum_evaluator.validate_enum only looks at pv.meaning (src/valuesets/validators/enum_evaluator.py:428-441). The 13 FBbi branch roots in bio/biological_imaging_methods.yaml (lines 78, 96, 114, 132, 151, 167, 186, 205, 224, 243, 259, 275, 294) are therefore unchecked — a wrong or obsolete root silently produces an empty or wrong value set at doc-generation time rather than an error.
• exact_mappings is never validated either. The FBbi IDs added to medical/dicom.yaml (lines 97, 104, 139, 240, 277) sit in exact_mappings, so the FBbi: sqlite:obo:fbbi entry added to oak_config.yaml:26 doesn't actually cause any FBbi ID in this PR to be checked. Worth stating explicitly, since the PR description implies the adapter enables validation of the FBbi work.
• meaning: NCIT:... is checked, strictly. For a configured prefix, both an unresolvable term and a label mismatch are ERROR (enum_evaluator.py:451-485), and the comparison is against the PV name + title + aliases. Several titles here intentionally differ from the NCIt label (DICOMModalityEnum even documents that behaviour, dicom.yaml:54-59), so just validate needs to be run and the NCIt label added as an alias wherever it isn't already. I'd double-check the less-commonly-seen ones by hand: NCIT:C38101 (RG), NCIT:C18001 (DX), NCIT:C20828 (OCT), NCIT:C17753 (CFM), and the data-management set C44285, C45970, C142477, C142500, C142529, C17624, plus the C190xxx block.

Suggested follow-up beyond this PR: a test that resolves every reachable_from.source_nodes CURIE through the configured adapter would close this gap permanently — the tests/validators/ files today only unit-test the validator machinery, and just validate isn't in CI (.github/workflows/main.yaml runs just test only). Fix this →

2. DCM prefix is missing from oak_config.yaml

DCM: is used as meaning: in ~25 places (dicom.yaml:78,114,132,171,175,201,205,221,233,244,248,283,287,291 and all of imaging_deidentification.yaml:66-138), but oak_config.yaml only gained FBbi. With no entry, the validator falls into dynamic mode, tries sqlite:obo:dcm, fails, and records an unknown prefix (enum_evaluator.py:325-337) — noisy output, and no validation. The repo convention for non-OBO vocabularies is an explicit key with an empty value (SNOMED:, LOINC:, HL7:), which makes the "deliberately unvalidated" intent readable. Fix this →

3. Inconsistent mapping structure in DICOMModalityEnum

Two different shapes appear in the same enum: CT (dicom.yaml:91-97) uses meaning: NCIT:C17204 with DCM:CT in exact_mappings, while BI (dicom.yaml:76-78) uses meaning: DCM:BI. So the "what is the DICOM code for this value" question is answered by a different field depending on whether an NCIt mapping happened to exist. Since this value set is CID 29, the more defensible rule is DCM as meaning throughout with NCIT as exact_mappings — or keep the current fallback but state it in the enum description.

4. Modality↔method exact_mappings are broader than "exact"

GM → FBbi:00000241 (microscopy), RG → FBbi:00001001, CT → FBbi:00001002 map a DICOM acquisition modality code to an FBbi imaging method class. close_mappings (or broad_mappings for GM) carries the right SSSOM semantics; these do get exported by just gen-sssom, so the predicate matters downstream.

5. DICOMModalityEnum can't represent values real archives actually put in Modality (0008,0060)

CID 29 is acquisition modalities only, but PACS/VNA extracts routinely contain OT, SR, PR, SEG, REG, KO, DOC, RTSTRUCT/RTPLAN/RTDOSE. Given the stated extraction/validation use case, anyone validating a real study list against this enum will get false failures. Either add them (annotated as non-acquisition) or say so explicitly in the description — right now the description only excludes waveform modalities.

6. Smaller things

• Ordering: stage_number annotations in ImagingDataLifecycleStageEnum (imaging_data_management.yaml:58,64,69,75,80,86) reimplement LinkML's rank on permissible values.
• Annotation value typing is inconsistent: retired: 'true' (quoted string, dicom.yaml:603), stage_number: 1 (int), preserves_skull: 'true'/'false'/partial (mixed boolean-and-not, imaging_deidentification.yaml:305-323), and comma-joined pseudo-lists like example_tools: PyDeface, FreeSurfer mri_deface, mydeface (:311) / modality: MRI (T1w, T2w). Worth settling on one style before these get consumed programmatically.
• Tool duplication: ITK_SNAP and SLICER_3D are defined in both DICOMSoftwareToolEnum (dicom.yaml:1046,1053) and DefacingToolEnum (imaging_deidentification.yaml:438,445) with different descriptions/annotations. Two sources of truth for the same tools, plus volatile license:/url: metadata to maintain. Consider one shared imaging-software value set.
• DeidentificationRegulatoryFrameworkEnum mixes kinds: regulations (HIPAA, GDPR), a technical standard (DICOM PS3.15) and community guidance (MIDI Task Group) in one enum with no kind annotation — unlike the kind: profile / clean option / retain option annotation used one enum earlier. Also, the PR description advertises PIPEDA but it isn't in the file.
• Scope creep: Bridge2AIDataGenerationProjectEnum (imaging_data_management.yaml:326) is program-level, not imaging-specific; in its own module it'd be reusable by non-imaging schemas.
• Naming: the medical files use the ...Enum suffix, the new bio file doesn't. The repo is already mixed (imaging_platforms.yaml has it, bio_entities.yaml doesn't), so this is cosmetic, but it's inconsistent within one PR.
• Versioning: see_also links all point at .../current/..., while the content (transfer-syntax registry, CID 7050) is edition-specific. An annotation recording the DICOM edition used would make future drift detectable.
• Unmapped concepts: ImagingInteroperabilityStandardEnum, most of ImageQualityMetricEnum (only SNR is mapped) and ImagingDataSourceTypeEnum have no meaning:. EDAM is already configured in oak_config.yaml and covers several of the formats/standards — the pv-mapping skill could fill these in.

No security concerns; nothing here executes. Performance is a non-issue except that 13 new reachable_from enums add ontology-resolution work to doc generation, and bio/biological_imaging_methods.yaml deliberately includes overlapping branches (MicroscopyMethod ⊂ BiologicalImagingMethod, etc.), which is fine but does multiply the expansion work.

---

What I couldn't run: just site and just validate per CLAUDE.md — neither just nor uv is installed in this runner, network access is blocked, and the ols MCP server failed to start (uvx not on PATH), so I could not verify a single CURIE against OLS or expand the dynamic enums. Every ID assessment above is from reading, not from lookup. CI (just test) will cover schema build and pytest, but ontology-mapping validation is manual — please run just validate-schema src/valuesets/schema/medical/dicom.yaml and the equivalents for the other three files locally before merge.
• claude/valuesets-to-add-t64z7y

[claude]

Claude encountered an error after 2s —— View job

---

I'll analyze this and get back to you.

[claude]

Claude finished @cmungall's task in 3m 55s —— View job

---

Code review: medical imaging + biological imaging value sets (round 2)

• Review new commit 29d07ed (validator changes + label fixes)
• Re-check the four schema files against prior review points
• Check test coverage for the new validator code
• Try to run just test / just validate — blocked, see note at the end
• Post review

The response to the previous round is solid: _is_skipped_prefix removes the duplicated skip logic, DCM is registered as a deliberately-unvalidated prefix with a comment (matching the SNOMED:/LOINC: convention), over-broad NCIT terms moved from meaning to broad_mappings rather than being deleted, the FBbi modality mappings were correctly downgraded to close/broad, and DICOMModalityEnum now documents both the NCIT-or-DCM meaning rule and the non-acquisition Modality values it excludes. The new tests are properly isolated (cache_dir=tmp_path/"cache", adapters injected before first lookup) so they don't pollute the committed cache/ tree.

Comments below on the new validator code, roughly in priority order.

---

1. Mapping-slot validation is a repo-wide behaviour change and hasn't been run repo-wide

validate_enum now resolves every CURIE in exact/close/broad/narrow/related_mappings for every enum in every schema, not just the new files. There are ~836 mapping-slot entries under src/valuesets/schema/, a large share of them NCIT: — a configured (strict) prefix, so any that fails to resolve is now an ERROR, and main() returns 1 on any error (enum_evaluator.py:702, :748).

Nothing in this PR demonstrates that the pre-existing 800+ mappings pass. Please run just validate src/valuesets/schema (the whole tree, not just the four new files) and paste the summary — if untouched files start failing, that's a decision to make deliberately (fix them, or downgrade mapping-slot failures to WARNING) rather than discover at merge. Fix this →

2. Negative results are cached permanently, and --no-cache doesn't clear them

get_ontology_label calls _save_to_cache(prefix, curie, label) for configured prefixes even when label is None, writing an empty label (enum_evaluator.py:237, label or ''). _load_cache reads it back and get_ontology_label returns label if label else None (:284). _save_to_cache early-returns when the CURIE is already present (:222), so the failure is never retried. cache/ is git-tracked, so a failed lookup gets committed.

This already happened once: cache/ncit/terms.csv:378 holds NCIT:C40997,,2025-12-19T12:48:59 — a permanently-poisoned entry from a previous run. Under the new code that would be a hard ERROR anywhere NCIT:C40997 appears.

Two compounding problems:

• The new checks multiply lookups (836 mapping slots + 13 source nodes on top of meaning), so one offline/partial run bakes in a lot of false ERRORs.
• --no-cache only gates self._label_cache (:128); the file cache at :278-291 isn't gated by config.cache_labels at all, so there is no supported way to bust a poisoned entry short of hand-editing the CSV.

Minimum fix: don't persist None labels (only cache successes). Better: make --no-cache bypass _prefix_caches too. Fix this →

3. Obsolete terms pass the new checks silently

_check_resolvable treats "resolves to a non-None label" as success (:446). An obsoleted OBO class keeps a label — typically obsolete <original name> — so it resolves fine. For meaning: the label-match path catches this incidentally (obsolete foo won't match the PV name/title/aliases), but mapping slots and reachable_from.source_nodes have no label comparison, which is exactly where an obsolete term is most damaging: an obsolete FBbi root yields an empty value set with no diagnostic. Cheap guard: flag labels matching ^obsolete\b. Fix this →

4. An unresolvable reachable_from root is only INFO for unconfigured prefixes

_check_resolvable downgrades to INFO when the prefix isn't in oak_config.yaml (:449-454). That's a reasonable default for meaning, where a bad mapping is cosmetic, but a bad source node silently produces an empty enum — a worse failure than a bad meaning, and invisible until someone reads generated docs. FBbi is configured so this PR's 13 roots do get ERROR treatment, but the asymmetry will bite the next dynamic enum added against an unconfigured ontology. Consider WARNING (rather than INFO) as the floor for source_nodes.

5. include / minus / concepts aren't covered

validate_reachable_from only reads enum_def.reachable_from. EnumDefinition also carries include and minus (each an AnonymousEnumExpression with its own reachable_from.source_nodes) and concepts (a plain CURIE list). Neither is used in the repo today — I checked — so this is future-proofing, not a bug. Worth a one-line comment in validate_reachable_from noting the deliberate scope, so the next person doesn't assume coverage that isn't there.

6. Nits

• MAPPING_SLOTS placement (:421-427): a class attribute wedged between extract_aliases and _is_skipped_prefix. Easy to miss; belongs at the top of the class body with the other class-level state.
• total_mappings_checked now conflates two things (:572-583): reachable_from source nodes are counted as "mappings", so print_summary's Mappings checked: line silently changes meaning. A separate total_source_nodes_checked would keep the summary honest.
• GDPR_ANONYMISATION title regression (imaging_deidentification.yaml:263): retitling to Anonymization to match the NCIt label loses the GDPR framing that made the value self-describing in a regulatory framework enum. Since extract_aliases matches against title and aliases, the inverse works equally well for the validator and reads better: keep title: GDPR anonymisation and put Anonymization in aliases. Same for GDPR_PSEUDONYMISATION (:271).
• PR description still advertises PIPEDA, which isn't in DeidentificationRegulatoryFrameworkEnum. Add it or drop it from the description.

7. Test coverage

The three new tests cover the paths that matter — configured-prefix → ERROR, unconfigured-prefix → INFO, reachable_from on an enum with no permissible values, and "a resolvable mapping needs no label match". Three gaps:

• No test that an empty-adapter prefix (DCM:) is skipped in mapping slots. That's the branch this PR added DCM: to oak_config.yaml for, and _is_skipped_prefix is now on the hot path for both meaning and mappings — worth pinning.
• No test for an enum with both reachable_from and permissible_values; the issues = validate_reachable_from(...) seeding at :486 only ever gets exercised in isolation.
• Nothing verifies the 13 FBbi roots actually resolve. That's the specific claim this PR makes, and it's still unverified by CI (.github/workflows/main.yaml runs just test, not just validate). A network-gated, skip-if-offline test that walks every reachable_from.source_nodes in the tree would close this for good.

---

What I could not run: Bash is heavily restricted in this runner — just, uv and python3 invocations were all denied, and the ols MCP server failed to start (uvx not on $PATH). So just site, just validate and just test were not executed, and no CURIE in this PR was verified against OLS. Every ID judgement here and in my previous review is from reading the diff, not from lookup. Items 1 and 2 above are the ones I'd most want confirmed by an actual local just validate run before merge.
• claude/valuesets-to-add-t64z7y