Across MI#2
Conversation
dates are correctly excluded by MICE without needing to have them removed from the dataframe due to the shift to defensive coding style, awkward workaround has been removed.
There was a problem hiding this comment.
I don't think it's as barmy as it looks. If tomorrow Kate said to change the number of imputed datasets from 10 to 20 for example, then we could change it in just one place instead of changing it manually across every other file.
There was a problem hiding this comment.
The weights function is another instance where it would need to be copy-pasted, but putting it here ensures consistency.
There was a problem hiding this comment.
see also the rule of three (https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming))
There was a problem hiding this comment.
ok - i retract my statement - good job thinking ahead
There was a problem hiding this comment.
updates to add function instead of absolute number
There was a problem hiding this comment.
addition of weights to each model to account for number of imputations.
There was a problem hiding this comment.
updates to add function instead of absolute number
There was a problem hiding this comment.
updates to add function instead of absolute number
There was a problem hiding this comment.
updates to add function instead of absolute number
| data = logistic_df, | ||
| weights = generate_weights( | ||
| sample_size = nrow(model_input_df), | ||
| num_imps = 10 |
There was a problem hiding this comment.
should this line take the new function for imputations as its argument instead so that you can update any weights at the same time as the number of imputations
i would place this in the generate_weights function as the default instead of null
There was a problem hiding this comment.
Hiya, sorry I think this might be an after effect of going one commit at a time, this line is updated on my end / the version as of yesterday's last commit. Good call r.e. having that as a default though, I'll get that actioned.
There was a problem hiding this comment.
actioned change, see below
| # Apply multiple imputation for sahhs outcome | ||
| imp_sahhs <- mice::mice( | ||
| data = df_no_vax_dates_sahhs, | ||
| m = 10, # hard-coded to match below |
There was a problem hiding this comment.
may have been updated later but check that m has been updated to use the new function here also
There was a problem hiding this comment.
This is updated in a later commit yes.
|
|
||
| # Re-assign unique identifiers to imputed dataset for outcome --- | ||
| print("Re-assign unique identifiers to imputed dataset for outcome ") | ||
|
|
There was a problem hiding this comment.
the below wont work if we update the number of imputations - I wouldn't bother changing at this point as Im fairly sure we will stick with 10 imputations - but need to be aware of this if we do - appreciate this suggestion is not in line with defensive coding
There was a problem hiding this comment.
i still dont understand why this is really needed
There was a problem hiding this comment.
The cox_ipw action requires unique patient id per row as part of data preparation (see: https://github.com/opensafely-actions/cox-ipw/blob/726915c1b57a1df1dac8e7fe37af782b876f3380/analysis/cox-ipw.R#L512)
There was a problem hiding this comment.
Good point that the patient_id reassignment is still stuck in the old way (hard coded to need 10, so still an improvement overall but this part isn't dynamic). This means we would strictly need to change the number of imputed datasets in two places (in utility.R and here where they're assigned) to make it work.
There is a part of me that knows that making this dynamic would be straightforward (generate vector of variable size using recursive pattern), I've just not done that (really trying to speed up on my own end)
There was a problem hiding this comment.
actioned change, see below
|
Hiya, I've implemented the new weights for the cox_ipw action (see most recent commit). I'm currently actioning your suggested change (having generate weights call the number of imputations function as a default) |
|
Hiya, I've implemented making the new patient ID script fully dynamic |
made the num_imps parameter optional, with default value taken from get number of imputed datasets function
|
Hiya, implemented the refactor of the generate_weights function suggested above (good call btw, it's much tidier now), I believe that makes everything and the code should be good to merge and run unless there's any further feedback |
|
Hi Paul, I've addressed all comments from the code review you provided Thursday evening, those being:
The other PR is an erroneous copy of this one (apologies) and has been deleted. The provided feedback all addressed, am I now good to merge and run? |
Updated the convert_terms_to_vars function to use a predefined list of variable names instead of relying on term splitting.
unable to do SEs, need to talk over
currently many are zero, to be discussed.
2 participants
No description provided.