-
Notifications
You must be signed in to change notification settings - Fork 219
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
Code status proposal #46
base: main
Are you sure you want to change the base?
Conversation
require("RPostgreSQL") | ||
drv <- dbDriver("PostgreSQL") | ||
con <- dbConnect(drv, dbname = "eicu", | ||
host = "localhost", port = 5432, |
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.
If possible avoid including your own username etc here. Might be better to include config settings at the top to make it easier for people to reuse the code. In other examples, we've included this at the top:
# Load configuration settings
dbdriver <- 'PostgreSQL'
host <- '127.0.0.1'
port <- '5432'
user <- 'postgres'
password <- 'postgres'
dbname <- 'mimic'
schema <- 'mimiciii'
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.
good idea.
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.
I have added the configuration settings at the top.
con <- dbConnect(drv, dbname = "eicu", | ||
host = "localhost", port = 5432, | ||
user = "josephpark", password = rstudioapi::askForPassword("Database password"), | ||
options = "--search_path=eicu") |
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.
For most people, I think the schema will be something like eicu_crd
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.
With the configuration setting, hopefully it is clearer despite the different schema name.
concepts/Code_status/Readme.md
Outdated
@@ -0,0 +1,9 @@ | |||
The files here create a patient table that tracks the changes to the patient code status. The careplangeneral table was used instead of careplaneol because careplaneol had much less recorded patients, and the type of code status change was recorded in careplangeneral. There are two versions of the files: .Rmd and .sql. The Rmd version is the supported version, while the SQL version is not supported and was added just as a template. |
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.
What does it mean for the version to be supported? If they are likely to get out of sync, it might be safer just to include one or the other?
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.
I have added a little more explanation to the Readme, stating that the SQL version was mostly autogenerated by using show_query(). I have included both versions, although I have done the analysis through R, for consistency since most of the MIMIC code repo is in SQL, so having an .sql file might be useful as a template.
concepts/Code_status/Readme.md
Outdated
The files here create a patient table that tracks the changes to the patient code status. The careplangeneral table was used instead of careplaneol because careplaneol had much less recorded patients, and the type of code status change was recorded in careplangeneral. There are two versions of the files: .Rmd and .sql. The Rmd version is the supported version, while the SQL version is not supported and was added just as a template. | ||
|
||
Rmd: | ||
- carePlan_getItemValues creates a csv that contains all the cplitemvalues for inspection, in order to choose the values that correspond to patient code status. |
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.
Unless there's a good reason to use camelcase, I'd suggest following current style and changing to lower case. Mixing upper and lower is awkward when typing things out.
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.
I have changed all filenames to lowercase instead of using camelcase.
DROP MATERIALIZED VIEW IF EXISTS patientCodeStatus; | ||
CREATE MATERIALIZED VIEW patientCodeStatus AS | ||
|
||
SELECT "patientunitstayid", "cplitemoffset", string_agg("cplitemvalue", ', ') AS "cplitemvalue" |
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.
It's difficult to make sense of this query, I guess because it was automatically generated. If we are going to include SQL, then ideally it should be readable. If the plan is to automatically generate it, then perhaps include an option to output SQL in the R Markdown?
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.
I have added comments in both the .Rmd and .sql files that tells people how the .sql file was generated (using show_query()) to clarify how this was produced.
concepts/Code_status/Readme.md
Outdated
@@ -0,0 +1,9 @@ | |||
The files here create a patient table that tracks the changes to the patient code status. The careplangeneral table was used instead of careplaneol because careplaneol had much less recorded patients, and the type of code status change was recorded in careplangeneral. There are two versions of the files: .Rmd and .sql. The Rmd version is the supported version, while the SQL version is not supported and was added just as a template. |
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.
Instead of ignoring the information in careplaneol
, could it be used to supplement the information in careplangeneral
?
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.
It seems careplaneol
is focused on discussion (e.g., family meeting), and contains little information about treatment limitations.
arrange(desc(n)) | ||
``` | ||
|
||
Example of a patient. This patient was strange with both full therapy and no CPR at the same 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.
If we add this code, then there is an expectation that it provides a reasonable summary of changing status. Do the numbers seem reasonable when checked with nursing staff, compared with MIMIC etc?
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.
@psy01212 created the list with Jerome. It's likely hospital dependent like most data in eicu.
except cplitemvalue = 'End of life', which was in cplgroup = 'Ordered Protocols'. | ||
|
||
```{r, connection = con} | ||
library(dplyr) |
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.
The SQL is only 4 lines long, so it seems overkill to include a whole RMarkdown file. Couldn't we just use the SQL, which is more consistent with the usual approach?
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.
We could do either. @psy01212 did it in R, I asked him to use the translate facility in dplyr
to have that option 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.
I have included both versions, although I have done the analysis through R, for consistency since most of the MIMIC code repo is in SQL, so having an .sql file might be useful as a template.
Thanks for the updates. I'm okay to merge, but I've listed a few thoughts below: R vs SQLI'm not sure that the R notebooks add anything beyond the SQL. Some clean SQL would be consistent with our general approach, and easier to interpret, update, and reuse. e.g. why not replace careplan_getpatientcode.rmd and careplan_getpatientcode.sql with (which I think is doing the same thing as the current auto-generated SQL, but in a more readable way): DROP MATERIALIZED VIEW IF EXISTS patientcodestatus;
CREATE MATERIALIZED VIEW patientcodestatus AS
SELECT patientunitstayid, cplitemoffset, string_agg(cplitemvalue, ', ') AS cplitemvalue
FROM careplangeneral
WHERE cplitemvalue IN ('Full therapy', 'Do not resuscitate', 'No CPR',
'No intubation', 'Comfort measures only', 'No cardioversion',
'No vasopressors/inotropes', 'No augmentation of care', 'End of life',
'No blood products', 'No blood draws', 'Advance directives')
GROUP BY patientunitstayid, cplitemoffset
ORDER BY patientunitstayid, cplitemoffset, cplitemvalue; If we are going to include the R notebooks, it feels like they should be doing more than simply generating SQL. Perhaps they could be expanded to visualise the tables generated by the SQL? names and dates in filesFollowing the style guide, please remove author names or dates in files (these are tracked in the commit history). Trying to fairly maintain these details in files is tricky. example patient in
|
done by @psy01212