insightsengineering / rtables

Reporting tables with R
https://insightsengineering.github.io/rtables/
Other
224 stars 48 forks source link

When should row_paths include "root"? #252

Open anajens opened 3 years ago

anajens commented 3 years ago

@gmbecker this example below is silly but it's just to illustrate some different layouts from tern where we need to know the row paths in order to use label_at_path. I mostly find ex (1) and (2) not very intuitive and I'd like to get your take on the rationale.

# Ex 1. No split function: row paths include root
t1 <- basic_table() %>%
  summarize_row_groups("ITTFL", label_fstr = "ALL")%>%
  build_table(ex_adsl)

# Ex 2. Add single split: row paths don't include root
t2 <- basic_table() %>%
  split_rows_by("ITTFL", split_fun = keep_split_levels("Y"), nested = FALSE) %>%
  summarize_row_groups("ITTFL")%>%
  build_table(ex_adsl)

# Ex 3. Add multiple (non-nested) splits: row paths include root again
t3 <- basic_table() %>%
  split_rows_by("ITTFL", split_fun = keep_split_levels("Y"), nested = FALSE) %>%
  summarize_row_groups("ITTFL")%>%
  split_rows_by("ITTFL", split_fun = keep_split_levels("Y"), nested = FALSE) %>%
  summarize_row_groups("ITTFL")%>%
  build_table(ex_adsl)
> row_paths(t1)
[[1]]
[1] "root"     "@content" "ALL"     

> row_paths(t2)
[[1]]
[1] "ITTFL"    "Y"        "@content" "Y"       

> row_paths(t3)
[[1]]
[1] "root"     "ITTFL"    "Y"        "@content" "Y"       

[[2]]
[1] "root"     "ITTFL"    "Y"        "@content" "Y"    
gmbecker commented 3 years ago

@anajens so this is strange, I agree.

The good news is that IIRC I made it so that including the "root" in paths is optional, so you should be able to assume you don't need it ever in practice, but having it there shouldn't hurt things either.

That inconsistancy is bad regardless of the above though, and I'll track it down.