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

Support core split overriding split functions in column space #785

Closed
gmbecker opened this issue Nov 15, 2023 · 6 comments
Closed

Support core split overriding split functions in column space #785

gmbecker opened this issue Nov 15, 2023 · 6 comments

Comments

@gmbecker
Copy link
Collaborator

gmbecker commented Nov 15, 2023

This will involve refactoring how how, and more importantly where, column subset expressions are generated.

@Melkiades @edelarua I'm not able to assign labels currently but please create "gabe" and "sme_review" labels (or similar) and apply them here. Also apply whatever I called the "collaborator request" type label.

I know how to do this and will implement it.

@shajoezhu
Copy link
Collaborator

Hi @gmbecker , can you please help me to better understand the rationale for this request? Thanks

@gmbecker
Copy link
Collaborator Author

@shajoezhu Certain complex tables apparently require columns along the lines of "patients enrolled and then so and so happened later", which is a subset of "patients enrolled". And these subsets can be overlapping, or interacting in other ways.

As I understand it, its something along the lines of:


         All Patients              Pats With A      Pats w/ A and B       Pats w/ B and not A 
----------------------------------------------------------------------------------------------
n            75                        50                 30                      15
mean                                            ...

etc etc.

We have something that fully supports this in row space, but it currently isn't supported in column space because of differences in how the tabulation machinery handles row and column subsetting (the former is done by subsetting the actual dataframe, the latter is carried around as unevaluated expressions).

I have something working locally in my fork that is passing all the direct rtables tests including a new one for this functionality.

@Melkiades
Copy link
Contributor

But if you do split_cols_by and then add a split function that does this?

@gmbecker
Copy link
Collaborator Author

@Melkiades it depends on the split function. If you use the provided combo functionality (e.g., add_combo_facet), it will work, but general custom splitting functions, specifically ones that override core splitting, are disallowed in column space.

You can see that from the current tests:

lyt4a <- basic_table() %>%
split_cols_by("ARM", split_fun = nonsense_splfun) %>%
analyze("AGE")
## not supported in column space, currently
expect_error(build_table(lyt4a, DM), "override core splitting")

That is what I have fixed locally by changing how column subsetting expressions are handled. I'll create a PR, probably today, based on my fork.

@gmbecker
Copy link
Collaborator Author

Sorry for the delay, this got buried in other things and then lost over the thanksgiving break in america. Anyway, PR is up now

@gmbecker
Copy link
Collaborator Author

gmbecker commented Jun 7, 2024

This was closed by #875

@gmbecker gmbecker closed this as completed Jun 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants