Skip to content

Make k the sole KNN result-count control, remove EmbeddingConceptFilter.limit (breaking, v2.0) #48

Description

@nicoloesch

Pre-flight

  • I searched existing issues and this has not been requested before.

Problem or motivation

EmbeddingConceptFilter.limit is currently overloaded: it's read as a fallback for KNN result count (k) and used for CDM-only row-capping (get_concepts_without_embedding and friends). This ambiguity was the root cause of a k-vs-limit precedence bug fixed in PR #40.

PR #40 prototyped a full fix: splitting into two types [EmbeddingConceptFilter (KNN-only, no limit) and a new CDMConceptFilter (CDM-only, keeps limit + apply()] but reverted it before merging, because:

  • It's a breaking change to a released public type.
  • omop-graph directly constructs EmbeddingConceptFilter(..., limit=...) in 3 places (reasoning/resolvers/resolvers.py:251, extensions/emb.py:169, extensions/emb.py:195).
  • It wasn't judged worth a v2.0 bump on its own right now

Instead, PR #40 shipped a backward-compatible interim fix (see "Alternatives considered") so this issue tracks doing the real, breaking fix properly, on its own release.

Proposed solution

  1. Implement the real split
  2. Remove the deprecation/consistency-check shim in EmbeddingReaderInterface._resolve_effective_k
    • EmbeddingConceptFilter no longer has .limit, so there's nothing left to check.
  3. Update omop-graph's 3 call sites:
    • reasoning/resolvers/resolvers.py:251: drop limit=constraints.limit; thread constraints.limit through as k= to get_neareast_concepts instead.
    • extensions/emb.py:169 and :195: split the one filter currently reused across both a CDM call (get_concepts_without_embedding) and a KNN call (get_neareast_concepts) into a CDMConceptFilter for the CDM call and a plain EmbeddingConceptFilter + explicit k= for the KNN call.

Alternatives considered

Shipped in PR #40 as the interim, non-breaking fix: keep EmbeddingConceptFilter untouched (still has limit + apply()), and instead add a runtime check in EmbeddingReaderInterface._resolve_effective_k

  • warn (DeprecationWarning) whenever concept_filter.limit is used for KNN, and raise ValueError if it's given alongside a conflicting explicit k.

Works, and needs zero changes in omop-graph right now, but is a permanent shim, not the real fix.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions