1301 feature request add confidence intervals for quantiles in surv time #1306
Conversation
|
✅ All contributors have signed the CLA |
Unit Tests Summary 1 files 84 suites 1m 13s ⏱️ Results for commit e7bfd91. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit e9b6f9c ♻️ This comment has been updated with latest results. |
|
Code Coverage SummaryDiff against mainResults for commit: e7bfd91 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
…s-for-quantiles-in-surv_time
|
I have read the CLA Document and I hereby sign the CLA |
Merge remote-tracking branch 'refs/remotes/origin/1301-feature-request-add-confidence-intervals-for-quantiles-in-surv_time' into 1301-feature-request-add-confidence-intervals-for-quantiles-in-surv_time # Conflicts: # R/survival_time.R
R/survival_time.R
Outdated
| @@ -209,7 +245,6 @@ surv_time <- function(lyt, | |||
| var_labels = var_labels, | |||
| show_labels = show_labels, | |||
| table_names = table_names, | |||
| na_str = na_str, | |||
There was a problem hiding this comment.
it is already passed as extra_args into the analysis function . If the na_str is passed as a named list (to be able to show the formatted value as "NA (NA, NA)" when a stat is all missing), having na_str as extra_args and inside analyze gave an error.
There was a problem hiding this comment.
I see! good catch! thanks :)
There was a problem hiding this comment.
I think this may also be present elsewhere. Can you add an issue regarding this please?
There was a problem hiding this comment.
after looking at it again, it might be more easy to remove na_str from extra_args and keep it in analyze only. Then it is much easier to accomplish "NA (NA, NA)", and there would not be the need for a named list to na_str to accomplish this.
Would it be ok to remove it from in_rows call and keep it only in analyze?
There was a problem hiding this comment.
The use of na_str seems a bit duplicated. It is more straightforward to modify it from the analyze function instead of sending it down. Yet the statistical functions may be used and tested externally from the decided analyze. For simplicity, I would keep only the na_str on the top level. If users want to use custom analyze functions can provide their own NA handling directly there
For the moment, it looks good. I will check downstream if there are breaking changes due to using all possible values by default but in principle that should be changed. @shajoezhu @edelarua, what do you think about freely adding stats like this? @iaugusty, we are already trying to introduce a bit more flexibility in custom stats' addition, so for me, it is practically good to go. Lets hear the others ;) |
Sounds fine to me, I would just try to stick with naming conventions used throughout the package as much as possible for any added statistics :) |
…s-for-quantiles-in-surv_time
|
Lgtm anyway! @iaugusty could you just fix the checks? Thanks |
|
block this PR by #1311 |
…s-for-quantiles-in-surv_time
There was a problem hiding this comment.
Lgtm! Thanks @iaugusty for the great work. I took a bit to check the na_str behavior and we should go back to it and make an uniform choice at a certain point. Tested your additions and works very well ;) I am moving the discussion for NAs handling to the issue you created.
…s-for-quantiles-in-surv_time
|
@iaugusty could you update the snapshots please? Thanks!! |
When I try to run the tests, it seems I have to update the svg files for the graphs, for font changes? Should I have some special settings to ensure I match the fonts that are being used at your end? I added the long version also to s_summary.numeric, as median_long was added to utils_default_stats_formats_labels.R, and this is being utilized by s_summary.numeric as well. This triggered me to add this stat in s_summary.numeric, and as we want the same long version for mean there as well, I added it at the same time. |
…_mean) to s_summary.numeric
You can turn off the plot diff with the following (in test/setup.R): # expect_snapshot_ggplot - set custom plot dimensions
expect_snapshot_ggplot <- function(title, fig, width = NA, height = NA) {
testthat::skip()
testthat::skip_on_ci()
testthat::skip_if_not_installed("svglite")Looking now at mean_long etc I think it would be better to have mean_ci etc as it is exactly that. Thanks Ilse!! mean_ci is already in, which is just the CI of the mean, not including the mean, while mean_long is a 3-d stat: mean + CI |
|
@Melkiades : not sure if you saw my reply on your comment mean_ci is already in, which is just the CI of the mean, not including the mean, while mean_long is a 3-d stat: mean + CI |
I see! I would call it mean_and_ci or mean_ci_3d then ;) |
…s-for-quantiles-in-surv_time
|
hi @iaugusty , I was wondering if you could update and resolve this conflict please? the PR is mostly fine to be merged now. Thanks! |
|
hi @iaugusty , I was wondering could you do the following please.
|
Pull Request
Fixes #1301
@Melkiades
I'm not sure about the impact of adding new stats to an analyze function on the remainder of the code/packages.
Could you review and share your thoughts?
This is the first function for which we'd like to add extra stats, there will be more functions for which we'd like to combine the stat and it's confidence interval into 1 line.
Once we know the approach we can follow, more of these types of updates might follow.