1324 feature request varying decimal precision in a summary #1356
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
3 tasks
Unit Tests Summary 1 files 84 suites 1m 39s ⏱️ Results for commit 648d5b6. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 9ebc3bc ♻️ This comment has been updated with latest results. |
|
Code Coverage SummaryDiff against mainResults for commit: 648d5b6 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
Melkiades
reviewed
Nov 19, 2024
| #' | ||
| #' @details | ||
| #' Currently only available for usage within `a_summary`, `analyze_vars`, and hope to extend to at least `a_ancova` and `summarize_ancova`. | ||
| #' Question to Roche: is there an intention to refactor a_ancova and summarize_ancova to not use make_afun, but same approach of in_rows as in a_summary? |
There was a problem hiding this comment.
I think questions should not be in the documentation
There was a problem hiding this comment.
open an issue/ask us via email
Melkiades
reviewed
Nov 19, 2024
| #' ) %>% | ||
| #' build_table(dt2) | ||
| #' | ||
| get_formats_from_stats_custom <- function(stats, |
There was a problem hiding this comment.
why a new function? _custom should be reserved for local code only
Melkiades
reviewed
Nov 19, 2024
| if (any(grepl("xx.d", str, fixed = TRUE))){ | ||
| checkmate::assert_integerish(d) | ||
| str <- | ||
| gsub("xx.d", paste0("xx.", paste0(rep("x", times = d), collapse = "")), str, fixed = TRUE) |
There was a problem hiding this comment.
is not better using sprintf directly?
|
@iaugusty which issue is this PR addressing? |
issue #1324:
|
Melkiades
reviewed
Nov 21, 2024
| #' @param stats (`character`)\cr statistical methods to get defaults for. | ||
| #' | ||
| #' @details | ||
| #' Currently only available for usage within `a_summary`, `analyze_vars`, and hope to extend to at least `a_ancova` and `summarize_ancova`. |
There was a problem hiding this comment.
Fix all lint error please. Thanks!
Melkiades
reviewed
Nov 21, 2024
Comment on lines
+89
to
+95
| #' get_formats_from_stats_custom( | ||
| #' stats = c("mean_pval", "mean", "sd" ), | ||
| #' formats_in = c("mean" = "xx.xxxx"), | ||
| #' fmts_specs = list( | ||
| #' fmts_df = tern_formats_custom_df(), | ||
| #' fmts_df_var = "default" | ||
| #' ))$fmt |
There was a problem hiding this comment.
all formatting is wrong here
Melkiades
reviewed
Nov 21, 2024
| if (!("formatting_function_exclude" %in% ls)) {formatting_function_exclude <- NULL} | ||
| if (!("formatting_function" %in% ls)) {formatting_function <- NULL} | ||
| if ("fmts_df_var" %in% ls && fmts_df_var != "default" &&!("fmts_df" %in% ls)) { | ||
| stop("fmts_df should be added to fmts_specs")} |
Melkiades
reviewed
Nov 21, 2024
| extra_args <- list(.stats = .stats, na.rm = na.rm, na_str = na_str, ...) | ||
| .indent_mods = NULL, | ||
| # varying precision arguments | ||
| fmt_specs = default_fmt_specs) { |
There was a problem hiding this comment.
Everything should pass by .formats. Having this is in addition is confusing. We can do already everything from .formats at the moment. Hence, I do not think we need this. It could be helpful to take a look again at the automatic formatting that we provide here in {tern}
Melkiades
reviewed
Dec 12, 2024
| fmts_specs = fmt_specs | ||
| ) | ||
| .formats <- .formats_all$fmt | ||
| .formats_char <- .formats_all$fmt_char |
There was a problem hiding this comment.
.formats_char is not used?
pawelru
reviewed
Dec 13, 2024
| #' our_custom_fmts <- tern_formats_custom_df() | ||
| #' | ||
| #' | ||
| tern_formats_custom_df <- function(){ |
There was a problem hiding this comment.
if I understood everything correctly, that's the function that you aim to overwrite to provide your custom data.
If the scope is really big (which I am still not quite sure about) - I would propose a configuration object. It would be the best to make it an environment class of the object so that the <- assign operator would work. It should have some meaningful defaults and allow for overwrittes. (Currently it's not very clear to me how the overwrites works in the current approach).
My idea is as follows:
(with defaults)
x <- (...) |> tern::foo(...) |> tern::bar(...)
(if wants to change the defaults)
tern::default_config$formats$foo <- ...
tern::default_config$formats$bar <- ...
x <- (...) |> tern::foo(...) |> tern::bar(...)
This approach would allow for more customisation in the future.
Please note that this is quite fundamental change to the package structure that would probably require more follow-up changes throughout the package to make it consistent as a whole (if it's working in one place - why this is not working in another). Let's do this only if the scope is really big.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request
This is a draft proposal on how to use customized formats with varying decimal precision.
Any feedback is more than welcome.
For now, only analyze_vars is using this approach, whereas I'd like to at least handle ancova analysis results in a similar way.