Refactoring of `analyze_vars` with specifics for .stat_names #1361

Melkiades · 2024-12-11T14:14:28Z

Pull Request

…default handlings

tests/testthat/_snaps/analyze_variables.md

Melkiades · 2024-12-12T16:49:31Z

tests/testthat/_snaps/analyze_variables.md

+                   all obs 
+      —————————————————————
+      n               4    
+      Mean (SD)   2.5 (1.3)


It should be NAs

Melkiades · 2024-12-12T16:49:45Z

tests/testthat/_snaps/analyze_variables.md

-      n       5   
-      a    2 (40%)
-      b    1 (20%)
-      NA   2 (40%)


Melkiades · 2024-12-12T16:50:07Z

tests/testthat/_snaps/analyze_variables.md

-      8          mean_ci       (4.57, 5.69)          0                 Mean 95% CI
-      9         mean_sei       (4.89, 5.38)          0               Mean -/+ 1xSE
-      10        mean_sdi       (4.35, 5.91)          0               Mean -/+ 1xSD
+      2              sum               48.7          0                         Sum


Why this calc changed?

R/analyze_variables.R

tests/testthat/_snaps/analyze_variables.md

R/utils_default_stats_formats_labels.R

Melkiades · 2024-12-19T23:21:20Z

R/utils_factor.R

 #'
-#' @return A `list` with modified elements `x`, `.formats`, `.labels`, and `.indent_mods`.
+#' @return A `list` with modified elements `stat_out`, `.formats`, `.labels`, `.levels`, and `.indent_mods`.


to fix: ungroup_stats should be removed in favor of get_*_from_stats

Melkiades · 2024-12-19T23:21:59Z

R/utils_factor.R

+  empty_pval <- "pval" %in% names(stat_out) && length(stat_out[["pval"]]) == 0
+  empty_pval_counts <- "pval_counts" %in% names(stat_out) && length(stat_out[["pval_counts"]]) == 0
+  stat_out <- unlist(stat_out, recursive = FALSE)

  # If p-value is empty it is removed by unlist and needs to be re-added
-  if (empty_pval) x[["pval"]] <- character()
-  if (empty_pval_counts) x[["pval_counts"]] <- character()


these are if pval is NULL?

pval handling to be refactored

Melkiades · 2024-12-19T23:24:00Z

tests/testthat/_snaps/analyze_variables.md

-      n          5   
-      FALSE   1 (20%)
-      TRUE    2 (40%)
-      NA      2 (40%)


Melkiades · 2024-12-26T17:13:21Z

R/analyze_variables.R


-  if (na.rm) {
+  if (na_rm) {


na_rm can be reverted into na.rm if you think it is just creating issues downstream

I have always thought it would be nicer for it to be na_rm but na.rm was the standard so we never changed. I'm not sure if this will cause any issues but if not it would be nice to change, or at least start a deprecation cycle.

Melkiades · 2024-12-26T17:13:52Z

R/analyze_variables.R

+  # Compare with reference group
+  if (isTRUE(compare_with_ref_group)) {


compare functions have been deprecated and merged in here

Maybe add a clearer error message/check for the presence of a reference group?

Melkiades · 2024-12-26T17:15:02Z

R/analyze_variables.R

  )

-  y$n_blq <- sum(grepl("BLQ|LTR|<[1-9]|<PCLLOQ", x))
+  y$n_blq <- list("n_blq" = c("n_blq" = sum(grepl("BLQ|LTR|<[1-9]|<PCLLOQ", x))))


useful to get_stat_names

Melkiades · 2024-12-26T17:16:09Z

R/analyze_variables.R

+  dots_extra_args <- list(...)
+
+  # Check if there are user-defined functions
+  default_and_custom_stats_list <- .split_std_from_custom_stats(.stats)


it is possible to add custom stats

Melkiades · 2024-12-26T17:16:36Z

R/analyze_variables.R

-                      .N_col, # nolint
-                      .N_row, # nolint
-                      .var = NULL,
-                      .df_row = NULL,
-                      .ref_group = NULL,
-                      .in_ref_col = FALSE,
-                      compare = FALSE,


all these vars are broadcasted from analyze through ...

Melkiades · 2024-12-26T17:17:21Z

R/analyze_variables.R

-  # Auto format handling
-  .formats <- apply_auto_formatting(.formats, x_stats, .df_row, .var)
+  # Get and check statistical names from defaults
+  .stat_names <- get_stat_names(x_stats, .stat_names_in) # note is x_stats


core stat_names modification

Melkiades · 2024-12-26T17:18:11Z

R/riskdiff.R

@@ -140,7 +141,7 @@ afun_riskdiff <- function(df,
    stat <- ifelse("count_fraction" %in% names(s_x), "count_fraction", "unique")
    if ("flag_variables" %in% names(s_args)) {
      var_nms <- s_args$flag_variables
-    } else if (!is.null(names(s_x[[stat]]))) {
+    } else if (is.list(s_x[[stat]]) && !is.null(names(s_x[[stat]]))) {


now statistics are character vectors

*named numeric vectors

Melkiades · 2024-12-26T17:19:44Z

R/utils_default_stats_formats_labels.R

@@ -351,6 +387,72 @@ get_indents_from_stats <- function(stats, indents_in = NULL, row_nms = NULL) {
  out
 }

+# Function to loop over each stat and levels to set correct values
+.adjust_stats_desc_by_in_def <- function(levels_per_stats, user_in, tern_defaults) {


This is only to be extra sure that you can change the labels in anyway you want (to do add this for formats and indentation)

Melkiades · 2024-12-26T17:21:25Z

tests/testthat/_snaps/analyze_variables.md

@@ -1361,7 +1603,7 @@
      ----------------------------
         row_name formatted_cell indent_mod     row_label
      1      pval        <0.0001          3        pvalue
-      2 median_ci   -0.41 - 1.10          3 Median 95% CI


why changed?

edelarua

This looks great!! Just a few comments from me regarding some improvements that could be made to the documentation, but the code all seems good!

edelarua · 2025-01-09T21:55:59Z

R/analyze_variables.R

+  # Compare with reference group
+  if (isTRUE(compare_with_ref_group)) {


Maybe add a clearer error message/check for the presence of a reference group?

edelarua · 2025-01-09T21:58:26Z

R/analyze_variables.R


-  if (na.rm) {
+  if (na_rm) {


I have always thought it would be nicer for it to be na_rm but na.rm was the standard so we never changed. I'm not sure if this will cause any issues but if not it would be nice to change, or at least start a deprecation cycle.

edelarua · 2025-01-09T22:34:17Z

R/analyze_variables.R

-                      .var,
-                      ...) {
-  checkmate::assert_flag(na.rm)
+s_summary <- function(x, denom, control, ...) {


... is still described as "additional parameters passed to s_summary" which I think should be updated with more detail since a lot of the additional parameters were collapsed into .... Maybe move the s_summary methods to their own file since there are a lot of them and their param descriptions no longer align very well with analyze_vars and the help file is getting pretty long? What do you think?

Also, since the description for na.rm was in the argument_convention.R file you should also add one for na_rm there.

edelarua · 2025-01-09T22:53:02Z

R/analyze_variables.R

-                              .N_col, # nolint
-                              ...) {
-  if (na.rm) x <- x[!is.na(x)]
+s_summary.logical <- function(x, denom = c("n", "N_col", "N_row"), ...) {


Is there a reason why denom wasn't put into the ... here but was in the other methods?

Melkiades added 3 commits December 10, 2024 12:48

initial rework of analyze_variables to allow custom stats and better …

4c83153

…default handlings

add of initial stats

c9b8a82

Fix: docs

b647e66

Melkiades added enhancement sme labels Dec 11, 2024

Melkiades marked this pull request as draft December 11, 2024 14:14

github-actions bot and others added 8 commits December 11, 2024 14:16

[skip style] [skip vbump] Restyle files

b2b2969

reducing unneeded complexity

c113b9e

merge

b4e6920

up

0ac4cae

fix multiple issues with logicals

14815f7

fixing all errors and test

bf128e8

fix docs

c343262

minor fixes

783039a

Melkiades commented Dec 12, 2024

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Show resolved Hide resolved

Melkiades commented Dec 12, 2024

View reviewed changes

tests/testthat/_snaps/analyze_variables.md

n 5

a 2 (40%)

b 1 (20%)

NA 2 (40%)

Copy link

Contributor Author

Melkiades Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lost NA

Melkiades commented Dec 12, 2024

View reviewed changes

R/analyze_variables.R Show resolved Hide resolved

Melkiades commented Dec 12, 2024

View reviewed changes

R/analyze_variables.R Outdated Show resolved Hide resolved

add: more tests

b2b2a5c

Melkiades marked this pull request as ready for review December 16, 2024 16:00

Melkiades commented Dec 16, 2024

View reviewed changes

tests/testthat/_snaps/analyze_variables.md Show resolved Hide resolved

Melkiades and others added 3 commits December 20, 2024 00:11

minor fixes

e1df39b

[skip style] [skip vbump] Restyle files

1dd2019

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

461ea9a

Melkiades commented Dec 19, 2024

View reviewed changes

R/utils_default_stats_formats_labels.R Outdated Show resolved Hide resolved

Melkiades commented Dec 19, 2024

View reviewed changes

tests/testthat/_snaps/analyze_variables.md

n 5

FALSE 1 (20%)

TRUE 2 (40%)

NA 2 (40%)

Copy link

Contributor Author

Melkiades Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lost NA

Melkiades added 5 commits December 26, 2024 12:57

fixes and simplifications

fb2b916

fixing changes in labels handling

b5caa9d

adaptation for count occurrences

ba3e107

tie loose ends

553bdd3

final fixes

db5b018

Melkiades commented Dec 26, 2024

View reviewed changes

[skip roxygen] [skip vbump] Roxygen Man Pages Auto Update

c2f8ca2

Melkiades commented Dec 26, 2024

View reviewed changes

shajoezhu requested review from edelarua and BFalquet January 9, 2025 01:11

Merge branch 'main' into 1352_fix_stats@main

956842d

edelarua requested changes Jan 10, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring of `analyze_vars` with specifics for .stat_names #1361

Refactoring of `analyze_vars` with specifics for .stat_names #1361

Melkiades commented Dec 11, 2024

Melkiades Dec 12, 2024

Melkiades Dec 12, 2024

Melkiades Dec 12, 2024

Melkiades Dec 19, 2024

Melkiades Dec 19, 2024

Melkiades Dec 26, 2024

Melkiades Dec 19, 2024

Melkiades Dec 26, 2024

edelarua Jan 9, 2025

Melkiades Dec 26, 2024

edelarua Jan 9, 2025

Melkiades Dec 26, 2024

Melkiades Dec 26, 2024

Melkiades Dec 26, 2024 •

edited

Loading

Melkiades Dec 26, 2024

Melkiades Dec 26, 2024

Melkiades Dec 26, 2024

Melkiades Dec 26, 2024

Melkiades Dec 26, 2024

edelarua left a comment

edelarua Jan 9, 2025

edelarua Jan 9, 2025

edelarua Jan 9, 2025

edelarua Jan 9, 2025

		# Compare with reference group
		if (isTRUE(compare_with_ref_group)) {

Refactoring of analyze_vars with specifics for .stat_names #1361

Are you sure you want to change the base?

Refactoring of analyze_vars with specifics for .stat_names #1361

Conversation

Melkiades commented Dec 11, 2024

Pull Request

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Melkiades Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edelarua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Refactoring of `analyze_vars` with specifics for .stat_names #1361

Refactoring of `analyze_vars` with specifics for .stat_names #1361

Melkiades Dec 26, 2024 •

edited

Loading