EHR queries - #103
EHR queries#103jeremyestein wants to merge 65 commits into
Conversation
Introduces new queries for airway and sputum/secretions data, and refactors the flow sheet query to consolidate multiple values into a wider format. A new README documents the overall data extraction goal and the current set of scripts.
Transforms specific lab test results (e.g., CRP, WCC) from a long format into distinct columns using `MAX() FILTER`. This enables direct consumption of lab data in a wider format, simplifying downstream analysis. Additionally, the query is updated to use parameters for `hospital_visit_id` and to include date range filtering, improving its flexibility.
…Claude Sonnet 5.0
jeremyestein
left a comment
jeremyestein
left a comment
There was a problem hiding this comment.
Some initial comments. I think this review is mostly a to-do list for me now :)
| # and reruns if the underlying data for this csn/day changes. | ||
| pseudonymised_parquets = pseudonymised_parquet_files_for_date_and_hashed_csn | ||
| output: | ||
| WAVEFORM_PSEUDONYMISED_EHR / (EHR_STEM_PATTERN_HASHED + "_ehr.csv") |
There was a problem hiding this comment.
Everywhere else we use dots to separate parts of the file so I think we should do that here too. (ie. .ehr.csv)
Also get_ehr_lookup should probably be the single source of truth for what this file looks like.
| WAVEFORM_ORIGINAL_PARQUET = WAVEFORM_EXPORT_BASE / "original-parquet" | ||
| WAVEFORM_HASH_LOOKUPS = WAVEFORM_EXPORT_BASE / "hash-lookups" | ||
| WAVEFORM_PSEUDONYMISED_PARQUET = WAVEFORM_EXPORT_BASE / "pseudonymised" | ||
| WAVEFORM_PSEUDONYMISED_EHR = WAVEFORM_EXPORT_BASE / "pseudonymised_ehr" |
There was a problem hiding this comment.
do_upload_multiple will refuse to upload anything that's not in the pseudonymised directory. So either that restriction would have to be expanded, or we should put this under pseudonymised. I lean to the latter, although perhaps in an EHR subdir to make it clear to the user that these files are in a different format to the waveform data? This is more a usability question than a functionality one.
There was a problem hiding this comment.
I think in a separate directory under pseudonymised would be best.
| "DateTimeRecorded": [0], | ||
| "Units": ["None"], | ||
| "Abnormal_result": ["No"], | ||
| "Comments": ["None"], | ||
| "C-reactive protein 1": ["-"], | ||
| "CSF WCC TUBE 1": ["-"], | ||
| "CSF WCC TUBE 2": ["-"], | ||
| "CSF WCC TUBE 3": ["-"], | ||
| "C-reactive protein 2": ["-"], |
There was a problem hiding this comment.
What do all these values mean? "None", "No", "-" are superficially similar-sounding. Do they correspond to values in the real database?
There was a problem hiding this comment.
They don't mean anything. I haven't seen enough real data to know what would be more representative.
There was a problem hiding this comment.
Ok, that will have to be changed later, and we'll need to know types when we define a parquet scheme.
|
|
||
| def connect(self) -> None: | ||
| """Set up connection to the database.""" | ||
| self.fake_caboodle = True if settings.CABOODLE_TESTING == "TRUE" else False |
There was a problem hiding this comment.
This would reject "1", "True", etc, which are allowed for other variables (the ones that Snakemake processes as config?)
There was a problem hiding this comment.
I did that because I wanted to make it as hard as possible to accidentality set CABOODLE_TESTING, as it would cause silent failures in production if used by mistake. It's probably overkill, in which case we could use pipeline.utils.config_bool to interpret other true like values.
| ON r.lab_order_id = o.lab_order_id | ||
|
|
||
| WHERE | ||
| r.result_status LIKE 'FINAL' |
There was a problem hiding this comment.
Was a wildcard intended? (otherwise this is the same as using =)
| filename = WAVEFORM_PSEUDONYMISED_EHR / f"{stem}_ehr.csv" | ||
| filename.parent.mkdir(exist_ok=True, parents=True) | ||
|
|
||
| df.to_csv(filename, index=False) |
There was a problem hiding this comment.
This should be output as parquet for consistency with the other parquet outputs, and disk space, typing, etc reasons.
There was a problem hiding this comment.
Should we have an intermediate step (i.e. to_csv, then a second process converts csv to parquet, or go straight to parquet?)
There was a problem hiding this comment.
No, we should go straight to parquet. The only purpose for the CSV intermediate for waveform data is that it's easy to append to. (Hence the desire to switch to a better appendable format #15)
But this is a batch operation so it can all be dumped in one go to parquet.
settings.SQL_PATH. Refactor SQL query loading code. Increase log dumping in integration test.
query results to a DataFrame, then access it as needed
| safe_columns = [ | ||
| "DateTimeRecorded", | ||
| "PlacementInstant", | ||
| "RemovalInstant", | ||
| "TubeSize", | ||
| "Repositioned", | ||
| "Position frequency", | ||
| "Temperature", | ||
| "Noradrenaline", | ||
| "Metaraminol", | ||
| "PaO2", | ||
| "PaCO2", | ||
| "Secretions", | ||
| "Sputum", | ||
| "Units", | ||
| "CRP", | ||
| "WCC", | ||
| "Comments", # Free text comments could contain sensitive information. Should we hash it? |
There was a problem hiding this comment.
I've updated these to match the columns in the queries, but can we rename them a bit so it's clearer which query they relate to? Eg. "Units" is not clear as to what it's the units for.
| ) -> pd.DataFrame: | ||
| """Retrieve airflow data from database.""" | ||
|
|
||
| airway_query = get_sql_query_text("private/airway.sql") |
There was a problem hiding this comment.
DateTimeRecorded comes out as a timezoned ISO-style timestamp. This is inconsistent with the waveform files that use seconds since epoch. The zoned timestamps start run midnight to midnight local time, whereas the waveform files run midnight to midnight UTC, which will produce some massive gotchas for the user.
Shall we try and standardise on something? And check what the postgres queries do, bearing in mind that mssql and postgres handle timezones quite differently.
3 participants
I have linearised the previous branches somewhat and removed all merges from dev (or janitoring), then done a big merge at the end. This takes us up to the equivalent of commit 061dca7 in the other branch.