Skip to content

Add DrugRecommendationOMOP task - #1203

Open
lehendo wants to merge 1 commit into
sunlabuiuc:masterfrom
lehendo:drugleak
Open

Add DrugRecommendationOMOP task#1203
lehendo wants to merge 1 commit into
sunlabuiuc:masterfrom
lehendo:drugleak

Conversation

@lehendo

@lehendo lehendo commented Aug 25, 2026

Copy link
Copy Markdown
Collaborator
  • drug_recommendation_omop_fn never excluded the current visit's own drugs from drugs_all, unlike every sibling drug-recommendation function/class, so the last history entry was identical to the prediction target. Fixed to match the established pattern (this is not necessary in retrospect, but the fix is fine)
  • Add DrugRecommendationOMOP, a current-API, leak-free class-based replacement, since drug_recommendation_omop_fn (and its mimic3/mimic4 siblings) cannot actually run through set_task() under the current Patient/Visit API
  • Document the legacy function family's set_task() incompatibility and flag the same issue in the one live example that hits it

…onOMOP

- drug_recommendation_omop_fn never excluded the current visit's own
  drugs from drugs_all, unlike every sibling drug-recommendation
  function/class, so the last history entry was identical to the
  prediction target. Fixed to match the established pattern.
- Add DrugRecommendationOMOP, a current-API, leak-free class-based
  replacement, since drug_recommendation_omop_fn (and its mimic3/mimic4
  siblings) cannot actually run through set_task() under the current
  Patient/Visit API -- OMOP drug recommendation had no working path at
  all before this. Verified end-to-end against real OMOP demo data.
- Document the legacy function family's set_task() incompatibility and
  flag the same issue in the one live example that hits it.
@AxelNoun

Copy link
Copy Markdown
Contributor

Hello ! I spent some time reviewing this against the current codebase and running it locally.

The gap is real: MortalityPredictionOMOP, LengthOfStayPredictionOMOP and ReadmissionPredictionOMOP all exist, and drug recommendation is the one missing. Your diagnosis checks out too — I verified that Visit is a deprecated stub with no get_code_list, that Patient has neither __len__ nor __getitem__, and that set_task() reads task.task_name / input_schema / output_schema, so the legacy *_fn genuinely cannot run.

A few things I'd raise before this lands.

The test never exercises set_task(). It calls DrugRecommendationOMOP()(patient) directly, so the nested_sequence and multilabel processors are never run which is the central claim of the PR. tests/core/test_omop_readmission_prediction.py is a good precedent.

The fixture can't detect the regression it targets. In test-resources/omop, only person_id = "1" has ≥2 visits with condition + procedure + drug together, and all four of its visits carry concept_id = "1" across all three tables. The drugs_hist[j] == ["1"] assertions would pass with a shifted, misattributed or reordered history the only real assertion is that slot i is empty.

No cache_dir, so the test writes to the default platformdirs cache and can pick up a stale build between runs.

Smaller points:

  • __call__'s docstring says "one sample per visit (after first visit)", but a sample is emitted for every qualifying visit including the first (drugs_hist == [[]]). Inherited from the eICU copy.
    The code filter is is not None, but CSV loading is all-string via pyarrow with strings_can_be_null=False, so a blank cell arrives as "" and becomes a code. Concept id 0 ("no matching concept") also becomes a label while _is_missing_ndc in the same file already treats "0" as missing.
  • samples[i]["drugs_hist"][j] is the same list object as samples[j]["drugs"]. Nothing mutates it today, but it's a sharp edge worth a defensive copy.
  • Three get_events per visit is O(V·N). One pass grouped by visit_occurrence_id is equivalent and much cheaper on OMOP, where patients tend to have many more visits than in eICU.
  • The example change documents the breakage rather than fixing it DrugRecommendationMIMIC4() is a two-line fix and nothing currently demonstrates the new task.

I have a local branch addressing all of the above: 17 tests written before the fixes (red on the aliasing and the 0 handling beforehand), a throwaway fixture built in a TemporaryDirectory so test-resources/omop stays untouched, and set_task() covered. Happy to push it to a fork and open it against your branch, or paste the diff here whichever you prefer.

@AxelNoun

Copy link
Copy Markdown
Contributor

I made a mistake because I had the commands ready, and I ended up pushing to your fork. I'm closing the PR. Let me know how you'd prefer to proceed. Sorry again for the mix-up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants