Add DrugRecommendationOMOP task - #1203
Conversation
lehendo
commented
Aug 25, 2026
- 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.
|
Hello ! I spent some time reviewing this against the current codebase and running it locally. The gap is real: A few things I'd raise before this lands. The test never exercises The fixture can't detect the regression it targets. In No Smaller points:
I have a local branch addressing all of the above: 17 tests written before the fixes (red on the aliasing and the |
|
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! |