Add DrugRecommendationOMOP task - #1203
Conversation
…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.
Drop empty/OMOP-0 concept ids, copy nested history lists so samples do not alias, and replace the mock leakage test with a discriminating set_task fixture. Co-authored-by: Cursor <cursoragent@cursor.com>
Keep TemporaryDirectory/close() failures visible on Linux CI, and document why new task re-exports use the redundant X as X form. Co-authored-by: Cursor <cursoragent@cursor.com>
|
I made a mistake because I had the commands ready, and I ended up pushing to your fork. I'm closing the PR. Sorry again for the mix-up! |
|
Hi. 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've put a branch addressing all of the above on my fork, |
|
Hey @AxelNoun, I appreciate the comments and pointing out the issues. I have gone ahead and added your branch commits to this PR. Feel free to review this PR again, and let me know if it good to merge to master. |
|
looks good. |
Sounds good to me, thank you for taking my recommendations into account! |
Also addresses the comments made by @AxelNoun.