-
Notifications
You must be signed in to change notification settings - Fork 6
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
extract events from the discharged_concept_id field of the visit table with the correct time #42
extract events from the discharged_concept_id field of the visit table with the correct time #42
Conversation
…act from for the event time
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.
Thanks! This looks good with just one minor suggestion for improvement, but you can ignore it if you want
src/meds_etl/omop.py
Outdated
@@ -188,6 +194,8 @@ def write_event_data( | |||
for option in options | |||
if table_name + option in schema.names() | |||
] | |||
# We prefer user defined time field options over the default time options if available |
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.
Why not:
options = table_details.get("time_field_options", []) + ["_start_datetime", "_start_date", "_datetime", "_date"]
options = [
cast_to_datetime(schema, table_name + option, move_to_end_of_day=True)
for option in options
if table_name + option in schema.names()
]
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.
@EthanSteinberg I like your suggestion cos it's concise. The fields defined in time_field_options
already contain the table name e.g. visit_end_datetime
and visit_end_date
, I can change it to _end_datetime
and _end_date
so your suggestion would work, what do you think?
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.
Hmm. Yeah that would work. I would also be down with changing the code above to:
ptions = table_details.get("time_field_options", []) + [table_name + "_start_datetime", table_name + "_start_date", table_name + "_datetime", table_name + "_date"]
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.
All looks good. Thanks especially for the unit test. We really need tests for all the logic in here, but not in this PR
@ChaoPang Simply merge this when you are ready |
We want to extract discharged_concept_id as a separate event from the visit table. This will be important for identifying hospital discharge events when using ACES.