Canonical event and modifier projections across CDM tables - #56
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
It makes broad foundational changes across core query contracts, CDM model/view metadata, tests, and docs, so a final human review is needed even though the issues found here are small.
Pull request overview
This PR introduces a new canonical query layer intended to be consumed by downstream omop-constructs, centralizing cross-table OMOP mechanics (canonical event projections/unions, modifier projections/selection/target validation, runtime concept predicates, and episode derivation helpers) and updating the toolkit/docs/tests accordingly to support a 1.0-ready base.
Changes:
- Added canonical “clinical event” projections/unions and associated contracts to standardize cross-table event querying.
- Added canonical modifier projections, deterministic selection, and target validation/diagnostics for Measurement/Observation modifiers.
- Added episode-derivation SQL builders (hierarchy projections, temporal ranking/window utilities, repeated-observation selection) plus expanded contract tests and documentation.
File summaries
| File | Description |
|---|---|
| tests/test_temporal_queries.py | Adds contract tests for portable temporal SQL expressions (window bounds, deltas, ranking). |
| tests/test_runtime_concept_queries.py | Adds runtime concept predicate/select tests against Concept_Ancestor. |
| tests/test_modifier_selection.py | Adds tests for deterministic modifier selection and oncology stage preference policy. |
| tests/test_event_timeline.py | Adds timeline behavior tests for canonical event identity + Observation support. |
| tests/test_event_projections.py | Adds tests for canonical event projections/unions and stable metadata resolution. |
| tests/test_episodes_basic.py | Updates episode tests for revised target-class registry/cache behavior and formatting. |
| tests/test_episode_structure_queries.py | Adds tests for canonical episode projection and hierarchy projections. |
| tests/test_concept_mapping_queries.py | Adds tests for standard concept mapping query contracts. |
| tests/test_concept_groups.py | Updates concept group tests for SemanticUnitRef and formatting. |
| tests/test_clinical_event_union_deprecation.py | Adds test ensuring deprecated import emits a DeprecationWarning. |
| tests/fixtures/query_contract_cases.py | Introduces shared, inspectable counterexample fixtures for query contracts. |
| tests/fixtures/init.py | Adds fixtures package marker and description. |
| pyproject.toml | Bumps orm-loader and omop-semantics minimum versions. |
| omop_alchemy/toolkit/episodes/handling/exposure_series.py | Clarifies explicit-link vs opt-in date-window fallback behavior in comments/docstring. |
| omop_alchemy/toolkit/episodes/derivation/temporal.py | Adds dialect-portable temporal SQL utilities and deterministic ordering helpers. |
| omop_alchemy/toolkit/episodes/derivation/structure.py | Adds episode/episode-event hierarchy projections supporting pre-shaped sources. |
| omop_alchemy/toolkit/episodes/derivation/observations.py | Adds repeated-observation ranking + eligibility predicates (portable SQL). |
| omop_alchemy/toolkit/episodes/derivation/contracts.py | Adds typed contracts for attachment identity/policy, temporal ranking, observation selection. |
| omop_alchemy/toolkit/episodes/derivation/init.py | Exposes derivation public API via re-exports and __all__. |
| omop_alchemy/toolkit/core/timeline/init.py | Updates timeline docs/exports to include Observation_Event. |
| omop_alchemy/toolkit/core/modifiers/targets.py | Adds canonical target validation + diagnostics for modifier-to-target links. |
| omop_alchemy/toolkit/core/modifiers/selection.py | Adds deterministic modifier ranking/selection over canonical modifier rows. |
| omop_alchemy/toolkit/core/modifiers/projections.py | Adds canonical modifier projections/unions with typed NULLs for missing value cols. |
| omop_alchemy/toolkit/core/modifiers/metadata.py | Adds explicit metadata allow-lists/specs for modifier source/target models. |
| omop_alchemy/toolkit/core/modifiers/contracts.py | Adds canonical modifier contracts (row vocab, identities, selection spec, diagnostics). |
| omop_alchemy/toolkit/core/modifiers/init.py | Adds modifiers public API surface via re-exports. |
| omop_alchemy/toolkit/core/events/projections.py | Adds canonical clinical-event projection + union and model spec resolution. |
| omop_alchemy/toolkit/core/events/contracts.py | Adds canonical event column/row/identity contracts. |
| omop_alchemy/toolkit/core/events/init.py | Adds events public API surface via re-exports. |
| omop_alchemy/toolkit/core/concepts/semantics.py | Adds SemanticUnitRef for lazy governed semantics unit references. |
| omop_alchemy/toolkit/core/concepts/runtime.py | Adds RuntimeConceptSetSpec and database-side runtime concept predicates/selects. |
| omop_alchemy/toolkit/core/concepts/relationships.py | Adds standard concept mapping query contracts/select. |
| omop_alchemy/toolkit/core/concepts/lookup.py | Improves docs/comments and adds docstrings for resolver/index utilities. |
| omop_alchemy/toolkit/core/concepts/groups.py | Refactors descendant expansion to reuse descendant_concept_select + typed anchors. |
| omop_alchemy/toolkit/core/concepts/init.py | Expands concepts public API with runtime/relationships/semantics exports. |
| omop_alchemy/toolkit/core/_ranking.py | Adds shared deterministic row_number helper for consistent ranking behavior. |
| omop_alchemy/toolkit/core/init.py | Updates core package docs to include events/modifiers and boundary notes. |
| omop_alchemy/toolkit/analytics/oncology/condition_modifiers.py | Adds stage-basis preference policy and preferred_stage_select over modifier rows. |
| omop_alchemy/toolkit/analytics/oncology/concept_sets.py | Switches to SemanticUnitRef and adds governed stage/grade/metastasis concept specs. |
| omop_alchemy/toolkit/analytics/oncology/init.py | Exposes new oncology exports (stage selection + additional concept specs/helpers). |
| omop_alchemy/toolkit/analytics/body_metrics/weight_trajectory.py | Adds/expands docstrings and reformats some long expressions. |
| omop_alchemy/cdm/query.py | Clarifies ConceptFilter.apply docstring semantics. |
| omop_alchemy/cdm/model/structural/episode.py | Makes EpisodeView a ModifierTargetMixin with explicit event metadata. |
| omop_alchemy/cdm/model/structural/episode_event.py | Switches Episode_Event target resolution to stable metadata map (no mapper scan). |
| omop_alchemy/cdm/model/clinical/observation.py | Adds ModifierSourceMixin + ObservationView/Context + modifier metadata. |
| omop_alchemy/cdm/model/clinical/measurement.py | Adds ModifierSourceMixin + MeasurementView/Context + modifier metadata. |
| omop_alchemy/cdm/model/clinical/event_metadata.py | Adds stable clinical-event/modifier-target metadata registries and helpers. |
| omop_alchemy/cdm/model/clinical/drug_exposure.py | Removes ModifierTargetMixin from base Drug_Exposure table class. |
| omop_alchemy/cdm/model/clinical/device_exposure.py | Adds Device_ExposureView/Context + modifier metadata; removes base mixin usage. |
| omop_alchemy/cdm/model/clinical/clinical_event_union.py | Adds DeprecationWarning pointing to canonical_event_union. |
| omop_alchemy/cdm/model/clinical/init.py | Re-exports new *View/*Context classes and reforms import structure. |
| omop_alchemy/cdm/base/modifier_interface.py | Introduces ModifierSourceMixin (common modifier link vocabulary). |
| omop_alchemy/cdm/base/cdm_constants.py | Extends ModifierFieldConcepts with MEASUREMENT/OBSERVATION/DEVICE_EXPOSURE constants. |
| omop_alchemy/cdm/base/init.py | Exports ModifierSourceMixin and updates public base exports. |
| mkdocs.yml | Updates toolkit navigation labels and adds new toolkit pages. |
| docs/toolkit/materialized-views.md | New documentation for materialized-view lifecycle boundary with orm-loader. |
| docs/toolkit/integrations.md | Rewrites heading/casing and consolidates long lines. |
| docs/toolkit/index.md | Refreshes toolkit overview, usage examples, and import-surface guidance. |
| docs/toolkit/episodes.md | Major refresh: episode retrieval, hierarchy traversal, attachment policy docs. |
| docs/toolkit/analytics.md | Major refresh: oncology + body metrics + adverse events narrative docs. |
| docs/getting-started/installation.md | Clarifies guidance around schema-level operations / independent sessions. |
| docs/api/typing.md | Reflows prose for consistency. |
| docs/api/relationships.md | Reflows prose for consistency. |
| docs/api/query.md | Reflows prose for consistency. |
| docs/api/index.md | Reflows prose for consistency. |
| docs/api/columns.md | Reflows prose for consistency. |
| docs/api/base.md | Reflows prose for consistency. |
| docs/api/architecture.md | Updates architecture diagram/text to mention materialized-view lifecycle in orm-loader. |
| docs/advanced/timelines.md | Updates timeline docs to include Observation and EventMapping.from_model usage. |
| docs/advanced/fulltext.md | Reflows prose and clarifies operational notes section heading. |
| .gitignore | Adds _design/ and fixes indentation for site/. |
| .github/CONTRIBUTING.md | Adds explicit ownership boundary guidance re: materialized views / orm-loader. |
Review details
- Files reviewed: 78/80 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
nicoloesch
left a comment
There was a problem hiding this comment.
Summary comment tying the whole review together, since the individual comments above are spread across roughly 40 locations. This restates every category below in one place. See the inline comments for file and line detail on each.
1. Correctness (2 items, highest priority). preferred_stage_select's default concept_code_column doesn't match any column this PR's own projections produce, and I can't find a production caller that would satisfy it. Separately, this PR replaces a dynamic Base.registry.mappers scan with a hardcoded _CLINICAL_EVENT_TARGETS tuple in event_metadata.py, silently dropping the old "new CDM view auto-registers" guarantee, plus a CLINICAL_EVENT_TARGETS_BY_FIELD_CONCEPT_ID dict with no uniqueness check across the field-concept constants it's keyed by. Both are detailed with a concrete fix below.
2. Architecture and design questions (7 items). The clearest structural finding in this PR: Episode_Event resolves its polymorphic modifier link generically (a field-concept-id dict plus session.get()), while Measurement/Observation modifiers have no equivalent and require the caller to already know which target table to check. ModifierSourceMixin is only ever applied to bare ETL tables while ModifierTargetMixin is only ever applied to *View classes, despite being equally self-contained, an asymmetry that looks deliberate but is undocumented. ModifierTargetModelSpec is a renamed 3-field subset of the 6-field ClinicalEventModelSpec it could reuse instead. A few smaller open questions round this out: a bare warnings.warn() instead of typing_extensions.deprecated on clinical_event_union.py, a _ranking.py module that's named private but used as public API by three sibling subpackages, and whether concepts/lookup.py's concept resolver is meant to absorb the resolution role omop-graph currently plays, since the two read as semantically close.
3. Duplication (5 distinct patterns, hand-copied rather than shared, commented at every occurrence). A 6-line coercion function, _as_source/_as_from_clause, copied into 5 files. A "missing required columns, raise" check copied into 4 places, already drifted between sorted and unsorted error output. A null-column-casting and union combinator duplicated between modifiers/projections.py and events/projections.py. Policy-to-sort-direction resolution reinvented independently 3 times (lower priority, since the three policy enums genuinely differ). Two functions in structure.py with byte-identical bodies differing only in a type hint.
4. Schema restated instead of derived (3 confirmed instances, 1 blocked, of the same fix). ClinicalEventColumn, ModifierColumn, and ModifierTargetDiagnosticColumn each independently retype a schema that a Protocol or dataclass two lines away already declares as typed fields, confirmed as exact field-for-field matches, fixable by deriving the tuples from __annotations__ instead. AttachmentDiagnosticColumn looks like the same pattern but isn't a clean match, its dataclass renames one field and nests two others, so it needs a small refactor first rather than a drop-in fix.
5. Error class inconsistency (1 item). Three different shapes, UnsupportedClinicalEventModelError, UnsupportedModifierSourceModelError, UnsupportedModifierTargetError, for what functions as one kind of error across the codebase.
6. from_mapping bypasses its own StrEnum (1 pattern, 2 occurrences). Both diagnostic from_mapping classmethods index into their row mapping with raw string literals instead of the stable enum labels that exist specifically for this.
7. Completeness gaps (5 items). ObservationView.__expected_domains__ only declares one column where sibling Views declare two. Measurement_Event.value_fields includes a field Measurement doesn't have, copy-pasted from Observation, currently silent by luck rather than design. Person_Timeline.events still issues one query per event table, and its sibling timeline property sorts the merged result in Python, even though this same PR adds a canonical_event_union that looks built to replace exactly that. A diagnostics function silently skips 2 of 4 checks for one input shape with only an inline comment, not a docstring, saying so. Device_ExposureContext declares relationships that sibling Measurement/Observation Contexts omit despite having the same underlying columns.
8. Docstrings and conventions (3 items), naming (1 item), and documentation (13 items, one per file and line above). Markdown bold in a docstring that renders as literal asterisks outside a doc viewer. Several multi-parameter functions missing the NumPy-style sections this codebase otherwise uses. A three-stage resolution design explained across 7 inline comment blocks but never stated in the function's own docstring. One pointless-looking rename in a test fixture. The documentation items are mostly pre-existing drift this PR touches without fixing, covering stale links, downstream-package name-dropping in the toolkit docs, an unlisted mkdocs.yml page, and an unusable autodoc-generated table of contents.
The core structural finding, tying categories 2 through 6 together into one root cause rather than five unrelated complaints. This PR contains two genuinely different engineering styles, split cleanly along subpackage lines. It is not the case that the toolkit is uniformly over-engineered. toolkit/core/concepts/*, with its byte-bounded LRU registry, vocabulary-identity-scoped caching, and dual Python/SQL concept groups from one spec, and episodes/handling/ plus analytics/oncology/ plus analytics/body_metrics/, with a shared group_and_summarize generic used unmodified across three unrelated domains and real subclassing like SACTDoseSummary(DrugExposureSummary), both show real engineering judgment and correctly avoid repeating themselves. toolkit/core/events/, toolkit/core/modifiers/, and toolkit/episodes/derivation/ do not. Each is stamped from the same template: a Column StrEnum, then a Row Protocol restating the same fields, then an Identity dataclass, then a model-metadata resolver, then a SQL builder with its own coercion helper, with the ceremony repeated 4 to 6 times across files rather than factored once. The clearest evidence is modifiers/metadata.py itself, commented on above: it imports and wraps events' clinical_event_model_spec for its source half, proving the resolver shape is already known to be shared, then hand-writes a from-scratch second copy of that same shape for the target half in the same file.
Recommendation. Apply the discipline already demonstrated elsewhere in this same PR, in _LazyBoundedRegistry[T] and group_and_summarize, to this template. One generic resolver, one generic coercion helper, and one generic projection-contract builder, used three or four times instead of hand-cloned, addresses categories 3, 4, and 5 directly, and removes the reimplemented half of category 2's modifiers/metadata.py finding. Separately, cdm/base currently has zero downstream imports, which is worth preserving as an architecture invariant, but that shouldn't be read as blocking metadata-resolution logic, as opposed to SQL-construction logic, from living closer to the ModifierSourceMixin/ModifierTargetMixin it resolves against. TYPE_CHECKING-guarded imports and ModifierTargetMixin.__init_subclass__-based auto-registration, proposed in the event_metadata.py comment above, are standard, safe ways to do that without inverting the layering. A further step worth considering alongside that refactor: a third mixin, ClinicalEventMixin(ModifierTargetMixin), giving the clinical-event subset of modifier targets its own self-registering class and a home for the toolkit/core/events/ resolvers, instead of one mixin plus a hand-maintained tuple carve-out for Episode. Category 6's from_mapping fix is small and independent of this refactor, worth taking regardless of whether the larger restructure happens.
…/episode derivation and layering, and align oncology stage selection, documentation, and tests.
nicoloesch
left a comment
There was a problem hiding this comment.
Most of the original 52 comments are genuinely resolved, verified against the code and by running the test suite. What's left, attached as comments below:
- two direct questions that didn't get a real answer,
- one promised PR-description correction that wasn't made,
- one decline whose "the tests cover it" reasoning is contradicted by the PR's own test suite, and
- three new observations from this pass (Measurement/Observation's start/end date columns representing "no interval" two different ways, and smaller suggestions on
ReferenceContextand theCANONICAL_*_COLUMNSconstants).
I re-opened some comments from the previous review to hopefully make them surface again here.
|
can't see how to respond to the PR description item so putting it here: I corrected the PR description to say that the timeline derives shared identity, concept and time mappings from CDM metadata while |
nicoloesch
left a comment
There was a problem hiding this comment.
All changes have been implemented!
Summary
This PR is focused on omop-constructs readiness for 1.0 base
Adds the canonical query layer that omop-constructs will consume, so generic OMOP mechanics live here rather than being reimplemented downstream.
Suggested review order:
Checklist
breaking,feature,fix,dependencies, orchore)uv run pytest -q)uv run ruff check .)