Projections - #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.
| { | ||
| target.modifier_field_concept_id(): target | ||
| for _, target in _CLINICAL_EVENT_TARGETS | ||
| } |
There was a problem hiding this comment.
Two things together. First, this replaces the old dynamic Base.registry.mappers scan with a hardcoded _CLINICAL_EVENT_TARGETS tuple, removing the "new CDM view auto-registers" guarantee the old code had. A forgotten registration now fails silently, since resolved_event just returns None. Second, CLINICAL_EVENT_TARGETS_BY_FIELD_CONCEPT_ID (lines 44-47) also has no uniqueness check across the six ModifierFieldConcepts constants it's keyed by. A future collision would silently drop one target with no error.
Proposing a ModifierTargetMixin.__init_subclass__-based auto-registration in modifier_interface.py to fix both at once. Each *View would register itself into a class-level dict the moment it's defined. No import of the concrete classes is needed from cdm/base, so there's no import-direction problem either, and this removes the hand-maintained tuple, both derived dicts, and the possibility of a forgotten or colliding registration. The comment right above _CLINICAL_EVENT_TARGETS already draws the line by hand, "Episode itself is a valid modifier target, but it is not a clinical event." Rather than a boolean flag, that line could become a second mixin, ClinicalEventMixin(ModifierTargetMixin), that every clinical-event *View inherits instead of the bare mixin, while EpisodeView stays on ModifierTargetMixin directly. Each mixin keeps its own __init_subclass__ registry, so "all modifier targets" and "just the clinical-event subset" are two real class hierarchies instead of one plus a flag. It would also give toolkit/core/events/'s resolvers, clinical_event_model_spec, canonical_event_projection, a home as classmethods on ClinicalEventMixin, symmetric to how ModifierSourceMixin/ModifierTargetMixin already host the modifier-only helpers.
Why a third mixin specifically, not just a flag on the existing one. ModifierSourceMixin/ModifierTargetMixin already form a matched pair for OMOP's polymorphic link, one role for a row that points at a target, one role for a row that can be pointed at. Clinical events are a strict subset of ModifierTargetMixin subclasses that additionally share a second, real connecting schema, the event_field_concept_id shape and the ClinicalEventColumn/ClinicalEventRow projection contract that toolkit/core/events/ resolves against. A subset that shares its own schema is what a mixin is for, not a boolean. It also gives the event-only helpers an actual home. clinical_event_model_spec, canonical_event_projection, and their ClinicalEventModelSpec/UnsupportedClinicalEventModelError types currently sit in toolkit/core/events/ with no structural connection to cdm/base at all, duplicating the same resolver shape modifiers/metadata.py already wraps for the source side. Landing them on ClinicalEventMixin closes that gap the same way ModifierSourceMixin/ModifierTargetMixin already close it on the modifier side, and removes event_metadata.py as a module rather than leaving it as a smaller flag-check file.
| targets without mutating the stable Core registry. | ||
| """ | ||
| return _modifier_target_classes_by_field_concept_id() | ||
| return dict(CLINICAL_EVENT_TARGETS_BY_FIELD_CONCEPT_ID) |
There was a problem hiding this comment.
Episode_EventView.resolved_event resolves its polymorphic link generically, using a field-concept-id dict to pick the class and then session.get() to fetch the row. Measurement/Observation modifiers have no equivalent. The only resolution path is the SQL join in toolkit/core/modifiers/targets.py, which requires the caller to already know which target table to check. Was a generic resolved_target left off Measurement/Observation deliberately, for example to avoid N+1 session.get() calls against bulk fact tables, or is it just not built yet? If deliberate, it's worth a docstring note saying so, since right now the asymmetry reads as accidental.
| @modifier_of_field_concept_id.inplace.expression | ||
| @classmethod | ||
| def _modifier_of_field_concept_id(cls) -> SQLColumnExpression[Optional[int]]: | ||
| return getattr(cls, cls.__modifier_field_concept_id_col__) |
There was a problem hiding this comment.
Three things, all in this file.
ModifierSourceMixinis only ever applied to bare ETL-facing classes,MeasurementandObservation.ModifierTargetMixinis only ever applied to*Viewclasses, and every pair in_CLINICAL_EVENT_TARGETSresolves to the View, never the bare class. I checkedModifierTargetMixin's own implementation and it's exactly as self-contained asModifierSourceMixin. It's plaingetattronClassVarstrings, with no dependency onReferenceContextorDomainValidationMixin, so nothing technically requires it to live on the View. Was that a deliberate choice, so a resolved modifier link always comes back enriched, or did it just fall out of how*Viewwas built? If deliberate, it's worth a docstring note, since right now nothing would catch a future PR addingModifierTargetMixinto a bare table.- The docstring, "helpers for OMOP tables that can modify another CDM row," reads as causal mutation. This is a polymorphic reference using
event_idandfield_concept_id, not a mutation of the target row. Suggest rewording to something like "tables that record a supplementary fact about another CDM row via OMOP's polymorphic modifier link." ModifierSourceMixin'sClassVars are prefixed,__modifier_event_id_col__, because they name a link.ModifierTargetMixin's are unprefixed,__event_id_col__, because they name self-identity. That's not a bug, but nothing signals which mixin either came from. A one-line comment above each block would fix that without renaming either scheme. Renaming to__source_event_id_col__would actually be less accurate, since it names the target's event ID, not the source's own.
| event_datetime_column=_datetime_column_name(model, event_date_column), | ||
| event_field_concept_id=field_concept_id, | ||
| event_source_table=metadata_model.modifier_target_table(), | ||
| ) |
There was a problem hiding this comment.
_has_complete_event_metadata only touches ModifierTargetMixin, already in cdm/base, plus getattr/issubclass. Recommend moving it to ModifierTargetMixin.has_complete_metadata() directly, with no import trick needed. clinical_event_model_spec, ClinicalEventModelSpec, and UnsupportedClinicalEventModelError only depend on cdm/base and event_metadata.py's own registries. Recommend relocating these to sit next to the registry they resolve against, importing whatever's needed inside the function body, or under TYPE_CHECKING for hints, if that crosses today's import direction. That leaves this file with only the actual SQL builders, canonical_event_projection and canonical_event_union, and lets modifiers/metadata.py's source half follow the same move. Proposing this as the boundary going forward: cdm/ resolves what metadata a model has, and toolkit/ turns resolved metadata into SQL. Today's boundary is closer to "cdm/ is ORM classes and toolkit/ is everything else," which is why this ended up here by default rather than by design.
…/episode derivation and layering, and align oncology stage selection, documentation, and tests.
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 .)