-
-
Notifications
You must be signed in to change notification settings - Fork 3
kmg plot #141
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?
kmg plot #141
Conversation
|
try and use exisitng structure and surv functions, refactoring will be addressed in #142 |
Unit Tests Summary 1 files 71 suites 1m 30s ⏱️ Results for commit 1c66e79. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 75185b2 ♻️ This comment has been updated with latest results. |
Code Coverage SummaryDiff against mainResults for commit: 1c66e79 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
need examples and export |
|
my styler is failing, @edelarua , i was wondering how you guys check style here https://github.com/insightsengineering/crane/actions/runs/19792793759/job/56708402248?pr=141 |
Missing link(s) in Rd file 'annot_surv_med.Rd': Missing link(s) in Rd file 'h_tbl_coxph_pairwise.Rd': Missing link(s) in Rd file 's_coxph_pairwise.Rd': See section 'Cross-references' in the 'Writing R Extensions' manual.
|
|
hi davide, can you fix the test please https://github.com/insightsengineering/crane/actions/runs/19936180249/job/57161536238 thanks |
Signed-off-by: Davide Garolini <[email protected]>
Melkiades
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.
There is an issue with alignment that I still need to work on. It was already an issue in the first version of the template ;)
for the risk table? I think that is minor, we can either create an issue to fix this, or move to ggsurft in the refacorting |
|
hi @Melkiades , thanks for the changes, i think it looks great! |
Melkiades
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.
I think it is good to go. There are other updates to be done but minor changes
| gg_coxph <- suppressMessages( | ||
| gg_coxph + | ||
| ggplot2::scale_x_continuous(expand = c(0.025, 0)) + | ||
| ggplot2::scale_y_continuous(labels = rev(rownames(coxph_tbl)), breaks = seq_len(nrow(coxph_tbl))) |
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.
This may benefit a refactoring as happened for annotate_risk
|
@shajoezhu we could use add_* instead of annotate_*? might be clearer |
honestly, i have no strong opinion on this |
probably not because it would create confusion with ggsurvfit (add_risktable function for example) |
|
I think it is good to go @shajoezhu |
|
hi @Melkiades , can you fix the cicd thanks! |
What changes are proposed in this pull request?
NEWS.md. (#, @)Provide more detail here as needed.
Reference GitHub issue associated with pull request. e.g., 'closes #'
Pre-review Checklist (if item does not apply, mark is as complete)
usethis::pr_merge_main()devtools::test_coverage()Reviewer Checklist (if item does not apply, mark is as complete)
pkgdown::build_site(). Check the R console for errors, and review the rendered website.devtools::test_coverage()When the branch is ready to be merged:
NEWS.mdwith the changes from this pull request under the heading "# cards (development version)". If there is an issue associated with the pull request, reference it in parentheses at the end update (seeNEWS.mdfor examples).