-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle analysis of categorical concepts #32
Conversation
…t set up access to test data with populated observation and measurement tables
…to filter NA because CDM specifies NULL or 0
…cept_id) in first step
…et Error in UseMethod("select") : no applicable method for 'select' applied to an object of class "NULL" Probably due to {{ concept }} tidy evaluation ?
…gic bug, simplified logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name_of_id_col <- paste0(name_of_table, "_concept_id")
It could not work as we want with other tables (e.g. DEVICE_EXPOSURE, because the column is named "device_concept_id").
Thanks @BaptisteBR, measurement and observation are the only tables that have a 'value_as_concept_id' field, DEVICE_EXPOSURE does not, so I don't think this is a problem ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice to see this coming along, when I ran this script quickly it seemed to fail, am I doing something wrong or do we need to change the dataset?
> source(here::here("dev/omop_analyses/analyse_omop_cdm.R"))
Error in dbAppendTable(conn, name, value) :
Column `value` does not exist in target table.
trying commit directly from Github Co-authored-by: Stef Piatek <[email protected]>
Seems that the error "Column |
Think the correct name here is |
Cleaning up should be left to the caller
@UCLH-Foundry/safehr-dev can I ask for another review 🙏 Thinking more about this, it might make sense to move the helper functions from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks nice, thanks for polishing this off Milan
Works on an omop cdm extraction (for HIC) but not yet tested on the package test data
Fixes #12
Fixes #33