-
Notifications
You must be signed in to change notification settings - Fork 50
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
update ref_group column ordering #748
Conversation
Code Coverage Summary
Diff against main
Results for commit: 8ca8ef5 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
test snap changing at insightsengineering/scda.test#67 |
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 this is too big of a breaking change right before the release. All of tern and TLG-C examples using ref_group
will be affected.
Ideally the ref_group
column should continue to be placed first by default. We just need to offer support to users who do not wish to use the default.
I fully agree, I think it is better to give the possibility in tern to put the baseline somewhere else instead of the default |
after simple evaluation, the Honestly I think this change should happen much earlier. With this update more flexibility is introduced and we finally have a consistent way of working with columns. Now factor levels is the only thing that changes the column order. |
I understand your point of view, but I think it is better to keep the standard like it is and then have a split_fun in tern that takes care of any possible change. Here, we added 3 ways to do this https://github.com/insightsengineering/tern/pull/1087/files. @anajens mentioned the possibility of having a lot of columns, in that case having the ref group as standard would be very useful, but I do not know if this is in line with the standards (also last is the same practically). Maybe @barnett11 can help us with this issue! |
Thanks @Melkiades, I would expect Control group to be either first or last, but fixing it one place could be difficult. The standards do not mandate that control arm must come first, so i think we should allow flexibility for this change in order as we do for all of our templates. It is common, and STREAM has allowed this flexibility to users for some time |
Signed-off-by: Davide Garolini <[email protected]>
Just added blocked because I want to add the split functions at the same time so not to have changes in the TLG-c |
Signed-off-by: Davide Garolini <[email protected]>
close #747