Skip to content
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

Empty levels missing when reference column is set #323

Closed
gmbecker opened this issue May 16, 2022 · 3 comments
Closed

Empty levels missing when reference column is set #323

gmbecker opened this issue May 16, 2022 · 3 comments

Comments

@gmbecker
Copy link
Contributor

Hi @gmbecker, I think it is an issue with VarLevWBaselineSplit, i.e.

library(rtables)


df <- data.frame(
  val = 1:10,
  grp = factor(rep("a", 10), levels = c("a", "b"))
)

And

basic_table() |>
  split_cols_by("grp") |>
  analyze("val") |>
  build_table(df)

results in

        a     b 
————————————————
Mean   5.50   NA

and

basic_table() |>
  split_cols_by("grp", ref_group = "a") |>
  analyze("val") |>
  build_table(df)

results in

        a  
———————————
Mean   5.50

Can you please check that the level with no observation does not get dropped when setting the ref_group?

Originally posted by @waddella in #322 (comment)

@gmbecker
Copy link
Contributor Author

gmbecker commented May 16, 2022

So this is interesting. The code is very old (and, I think, not hit very often), but looking now at what is in there, this was at one point intentional. droplevels is being called explicitly when the global child-value-order is determined in the split-that-has-a-reference-group case.

I think that its ok to remove this. That already happened in the no reference-group case, from the looks of it. In both cases, teh columns are degenerate, but having a reference group doesn't make that more wrong in any way that i can think of. Note, however, that this is technically a user-facing behavior change, rather than a bugfix.

I'll bring it up in the meeting tomorrow and go ahead and make the change provided there's no pushback.

@danielinteractive fyi

@danielinteractive
Copy link
Contributor

@gmbecker how did the meeting go? can you make the change?

@gmbecker
Copy link
Contributor Author

Yes we will be making the change and it will be in the CRAN release that will be going out in the next couple days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants