-
-
Notifications
You must be signed in to change notification settings - Fork 22
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
1301 feature request add confidence intervals for quantiles in surv time #1306
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 cc1f58e. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit c94458d ♻️ This comment has been updated with latest results. |
Code Coverage Summary
Diff against main
Results for commit: cc1f58e 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
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.
Looks very good! Thanks. Just a couple of comments (could you also add NEWS entry ;))
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
…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.
|
tests pass!! @iaugusty could you rerun the documentation to solve conflict? thanks :) then we merge |
…s-for-quantiles-in-surv_time
…s-for-quantiles-in-surv_time
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.