Skip to content

Canonical event and modifier projections across CDM tables - #56

Merged
nicoloesch merged 30 commits into
mainfrom
projections
Sep 17, 2026
Merged

nicoloesch merged 30 commits into
mainfrom
projections

Conversation

@gkennos

@gkennos gkennos commented Aug 31, 2026 •

Copy link
Copy Markdown
Member

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.

  • New: toolkit/core/events:
    • canonical clinical-event contracts and projections
    • One stable column set (person_id, event_id, event_date, event_concept_id, event_field_concept_id, event_source_table, …) across the event tables, plus canonical_event_union
    • Supported models are resolved from ModifierTargetMixin metadata
  • New: toolkit/core/modifiers
    • canonical Measurement/Observation modifier projections, target validation with typed diagnostics, and deterministic earliest/latest selection.
    • Fixes three real defects in the current Constructs implementation (these mostly are not an issue because of how the mapping was delivered but cleanup important regardless):
      • ranking that loses target-table identity (Condition 7 vs Procedure 7 sharing a window)
      • non-deterministic same-date ties
      • modifier joins that never compare person_id
  • New: toolkit/core/concepts
    • runtime concept resolution, relationships and semantics helpers
    • concept lookups stop happening at module import
  • New: toolkit/episodes/derivation
    • episode attachment, structure and temporal query builders with typed diagnostics
    • replaces reading Episode_Event directly.
  • New: toolkit/analytics/oncology/condition_modifiers.py
    • stage-basis preference (pathological → clinical) as overridable policy, kept out of the generic core.
  • Changed:
    • toolkit/core/timeline/event_timeline.py now derives shared event identity, concept and time mappings from CDM metadata; Person_Timeline retains one ORM query per configured event class
    • Episode/EpisodeView and the clinical models gained explicit modifier metadata
    • shared deterministic ranking extracted to toolkit/core/_ranking.py.

Suggested review order:

  1. toolkit/core/events/contracts.py (column vocabulary)
  2. cdm/model/clinical/event_metadata.py (which models are events vs modifier targets, and why Episode is deliberately in the second set only)
  3. toolkit/core/modifiers/ - contracts → metadata → projections → selection → targets
  4. toolkit/episodes/derivation/attachments.py
  5. tests/fixtures/query_contract_cases.py - shared expectations for the builders

Checklist

  • Applied exactly one label (breaking, feature, fix, dependencies, or chore)
  • Tests pass locally (uv run pytest -q)
  • Lint passes (uv run ruff check .)

@gkennos gkennos added the feature New backwards-compatible functionality. MINOR: x.y+1.z label Aug 31, 2026
@gkennos
gkennos requested review from nicoloesch and a lite review from Copilot September 7, 2026 02:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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.

Comment thread omop_alchemy/cdm/model/clinical/__init__.py Outdated
Comment thread omop_alchemy/toolkit/core/modifiers/contracts.py Outdated
Comment thread omop_alchemy/toolkit/episodes/derivation/observations.py

@nicoloesch nicoloesch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Comment thread omop_alchemy/toolkit/analytics/oncology/condition_modifiers.py Outdated
Comment thread omop_alchemy/cdm/model/clinical/event_metadata.py
Comment thread omop_alchemy/cdm/model/structural/episode_event.py
Comment thread omop_alchemy/cdm/base/modifier_interface.py
Comment thread omop_alchemy/toolkit/core/events/projections.py Outdated
Comment thread docs/toolkit/index.md Outdated
Comment thread docs/toolkit/core.md
Comment thread docs/toolkit/materialized-views.md Outdated
Comment thread docs/toolkit/integrations.md Outdated
Comment thread docs/toolkit/core.md
…/episode derivation and layering, and align oncology stage selection, documentation, and tests.
@gkennos
gkennos requested a review from nicoloesch September 16, 2026 01:49

@nicoloesch nicoloesch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 ReferenceContext and the CANONICAL_*_COLUMNS constants).

I re-opened some comments from the previous review to hopefully make them surface again here.

Comment thread docs/assets/images/oa-configure.png
Comment thread omop_alchemy/toolkit/core/events/projections.py Outdated
Comment thread omop_alchemy/toolkit/core/modifiers/contracts.py
Comment thread omop_alchemy/cdm/base/modifier_interface.py
Comment thread omop_alchemy/toolkit/core/timeline/event_timeline.py
Comment thread omop_alchemy/cdm/model/clinical/event_metadata.py
Comment thread omop_alchemy/cdm/model/clinical/measurement.py
Comment thread omop_alchemy/cdm/model/clinical/observation.py
Comment thread omop_alchemy/toolkit/core/events/contracts.py Outdated
Comment thread omop_alchemy/cdm/model/clinical/measurement.py Outdated
@gkennos

gkennos commented Sep 17, 2026

Copy link
Copy Markdown
Member Author

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 Person_Timeline retains one ORM query per configured event class.

@gkennos
gkennos requested a review from nicoloesch September 17, 2026 04:46

@nicoloesch nicoloesch left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All changes have been implemented!

@nicoloesch nicoloesch changed the title Projections Canonical event and modifier projections across CDM tables Sep 17, 2026
@nicoloesch
nicoloesch merged commit 54cb20d into main Sep 17, 2026
4 checks passed
@nicoloesch
nicoloesch deleted the projections branch September 17, 2026 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature New backwards-compatible functionality. MINOR: x.y+1.z

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants