Skip to content
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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Code status proposal #46

wants to merge 15 commits into from

Conversation

jraffa
Copy link
Contributor

@jraffa jraffa commented Jul 31, 2018

done by @psy01212

require("RPostgreSQL")
drv <- dbDriver("PostgreSQL")
con <- dbConnect(drv, dbname = "eicu",
host = "localhost", port = 5432,
Copy link
Member

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'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea.

Copy link

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")
Copy link
Member

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

Copy link

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.

@@ -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.
Copy link
Member

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?

Copy link

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.

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.
Copy link
Member

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.

Copy link

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"
Copy link
Member

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?

Copy link

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.

@@ -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.
Copy link
Member

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?

Copy link
Contributor Author

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.
Copy link
Member

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?

Copy link
Contributor Author

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)
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link

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.

@tompollard
Copy link
Member

tompollard commented Feb 1, 2019

Thanks for the updates. I'm okay to merge, but I've listed a few thoughts below:

R vs SQL

I'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 files

Following 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 careplan_getpatientcode.rmd

careplan_getpatientcode.rmd finishes with the following chunk:

Example of a patient. This patient was strange with both full therapy 
and no CPR at the same time.

{r, connection = con,eval=FALSE}
patientCodeStatus %>%
  filter(patientunitstayid == 266999)

This example returns a single row for me, which seems inconsistent with the comment:

patientunitstayid   266999
cplitemoffset       21041
cplitemvalue        No intubation

If the R notebook is going to be included with this example, it would be helpful to dig a little deeper. Are there any explanations for the inconsistency?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants