Skip to content

using centralised concept filtering functionality from omop-alchemy - #37

Merged
gkennos merged 2 commits into
mainfrom
concept
Aug 13, 2026
Merged

using centralised concept filtering functionality from omop-alchemy#37
gkennos merged 2 commits into
mainfrom
concept

Conversation

@gkennos

@gkennos gkennos commented Aug 12, 2026

Copy link
Copy Markdown
Member

Summary

stop re-implementing concept filters that already exist on omop-alchemy

SearchConstraintConcept and the hand-rolled standard_concept/invalid_reason predicates in queries.py are replaced by OMOP Alchemy's ConceptFilter, Concept.is_standard_expr(), and Concept.is_valid_expr(). Most of the replaced predicates were already equivalent to the canonical ones; the observable changes are listed below.

Breaking changes

  • ConceptView.standard_concept widens from 'S' to 'S' or 'C'. It was previously standard_concept == "S", which deliberately reported classification concepts as non-standard, and is now the canonical standardness expression. The is_standard columns of q_concept_name()/q_concept_synonym() already used ('S', 'C') and are unaffected.
  • ConceptFilter rejects a non-positive limit where SearchConstraintConcept silently accepted it.

Behaviour preserved

Activity and standardness now follow Concept.is_valid_expr()/is_standard_expr(), which agree with the predicates they replace for every value CDM v5.4 permits ('S'/'C'/NULL and 'D'/'U'/NULL). They additionally normalise the blank/whitespace-only strings some real-world loads produce, so a blank standard_concept is now treated as unset rather than as standard, and a blank invalid_reason as unset rather than as a reason. Out-of-spec non-blank values rank as inactive rather than active.

ConceptFilter also adds require_active, which defaults to False and so leaves existing filters unchanged.

Note: EmbeddingResolver does not thread require_active through to omop-emb, whose EmbeddingConceptFilter has no equivalent field, so setting it filters the SQL resolvers but not the embedding resolver. Tracked separately against omop-emb.

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 breaking Incompatible API change. MAJOR: x+1.y.z label Aug 12, 2026
@gkennos
gkennos requested a lite review from Copilot August 12, 2026 14:16

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR centralizes concept filtering and concept “standard/active” semantics by replacing SearchConstraintConcept with OMOP Alchemy’s ConceptFilter, ensuring the graph queries and resolvers rely on a single canonical implementation.

Changes:

  • Replaced internal SearchConstraintConcept with omop_alchemy.cdm.query.ConceptFilter across resolvers, grounding, KG APIs, benchmarks, and tests.
  • Updated concept query projections/orderings to use OMOP Alchemy’s canonical “standard” and “active” expressions.
  • Added integration tests to lock in canonical concept-filter semantics and updated documentation examples accordingly.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_grounding.py Switches grounding tests to use ConceptFilter.
tests/test_embedding_optional.py Switches embedding-related tests to use ConceptFilter.
tests/test_concept_queries.py Adds integration coverage for canonical concept filtering / flags / ordering semantics.
src/omop_graph/reasoning/resolvers/resolvers.py Updates resolver APIs and embedding filtering to accept ConceptFilter.
src/omop_graph/reasoning/resolvers/resolver_pipeline.py Updates pipeline constraint typing/docs to ConceptFilter.
src/omop_graph/reasoning/grounding.py Updates grounding constraints typing to ConceptFilter and validation callsites.
src/omop_graph/oaklib_interface/omop_implementation.py Uses ConceptFilter when constructing grounding constraints from Oaklib inputs.
src/omop_graph/graph/queries.py Delegates filtering to ConceptFilter and projects canonical standard/active flags in query outputs.
src/omop_graph/graph/nodes.py Updates ConceptView standardness handling and introduces is_active property.
src/omop_graph/graph/kg.py Updates KG search constraint typing to ConceptFilter.
src/omop_graph/graph/constraints.py Removes the now-redundant internal SearchConstraintConcept implementation.
scripts/benchmarks/trace_example.py Migrates benchmark tracing helpers to ConceptFilter.
scripts/benchmarks/benchmark.py Migrates benchmark constraint construction to ConceptFilter.
docs/reasoning/resolvers.md Updates documentation examples to use ConceptFilter.
docs/reasoning/grounding.md Updates documentation to describe ConceptFilter fields/semantics.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/omop_graph/graph/queries.py
Add the missing blank lines between the import block and
`_concept_is_standard_expr`, which landed flush against the imports where the
deleted `from .constraints import ...` line used to be.

Correct the `_concept_match_order_terms` docstring, which still described the
previous `invalid_reason in ("D", "U")` test after the code moved to OMOP
Alchemy's `is_valid_expr()`. The two agree on every value CDM v5.4 permits
("D", "U", NULL) and on the blank strings some real loads produce; they differ
only for out-of-spec values, which now rank as inactive rather than active.
@gkennos
gkennos merged commit 2ab826c into main Aug 13, 2026
2 checks passed
@gkennos
gkennos deleted the concept branch August 13, 2026 01:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Incompatible API change. MAJOR: x+1.y.z

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants