-
Notifications
You must be signed in to change notification settings - Fork 3
add influence function calculation #37
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
base: main
Are you sure you want to change the base?
Conversation
mbannick
left a comment
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.
Hi @dbackenroth, thanks for submitting this PR! Could you please add some brief documentation for this in the function?
|
Hi @mbannick I added in some documentation, please let me know what else I can provide. |
R/adjust-logrank.R
Outdated
| se <- sqrt(var_CSL) | ||
| statistic <- U_CSL / se | ||
|
|
||
| if ("id" %in% colnames(df)) { |
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.
@dbackenroth Thanks for adding documentation explaining this PR!
If someone has a covariate that is named id (that would be a strange name for a covariate, but it's possible, particularly if they are doing stratification by some type of id), then this may produce unintended behavior. I don't want anything to break for current users since this is a stable package.
Could you make the following change please:
- Modify
robincar_logrankso that it takes areturn_influenceargument that is by default FALSE, and then propagate that argument tomodeland then it can be accessed here and below, asif model$return_influencerather thanif "id" %in% ...? This way, users won't see any change in behavior based on default usage.
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.
@dbackenroth it also looks like some tests fail with the current PR. Could you please ensure the test suite runs through successfully? Thanks! See e.g., https://github.com/mbannick/RobinCar/actions/runs/20396466576/job/58744492353?pr=37.
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.
@mbannick hi, i added in unit tests. two of the tests are failing for me but I don't think that has to do with my code (test-glm-calibration.R: test calibration and test-glm-legacy.R: GLM legacy), it's related to functions not being found, e.g., str_detect. I added in two columns to robincar_tte signature, one is return_influence and the other is id_col. i may have gone overboard with the error checking, please let me know if you'd like me to make any further changes
calculates influence function for CALRT and returns to user (if user includes an "id" column in input dataset)