refactor colsubset exprs tracking so it an be overridden in split funs #795
Conversation
gmbecker
requested review from
edelarua,
Melkiades and
ayogasekaram
as code owners
|
Hi @gmbecker , thanks a lot for the changes. We will need to create PRs and trigger CICD pipelines to investigate if any downstream break changes. Will get back to this as I know more. |
R/make_split_fun.R
Outdated
| #' | ||
| #' @examples | ||
| #' splres <- make_split_result( | ||
| #' values = c("hi", "lo"), | ||
| #' datasplit = list(hi = mtcars, lo = mtcars[1:10, ]), | ||
| #' labels = c("more data", "less data") | ||
| #' labels = c("more data", "less data"), | ||
| #' subset_exprs = list(expression(TRUE), expression(seq_along(wt) <= 10)) |
There was a problem hiding this comment.
so this would do a subset only in the column space, while it is ignored in the row space, right?
There was a problem hiding this comment.
Correct. Faceting in column space is tracked via expressions, while faceting in row space is immediately materialized during tabulation. This is largely for a combination of performance and legacy reasons.
R/make_split_fun.R
Outdated
| validate_split_result(splres) | ||
| newstuff <- make_split_result(values, datasplit, labels, extras) | ||
| newstuff <- make_split_result(values, datasplit, labels, extras, subset_exprs = list(sub_expr)) |
There was a problem hiding this comment.
why sub_expr is not defaulting to the splres values if sub_expr is NULL?
There was a problem hiding this comment.
So there are two cases that can happen here:
- values are strings (this is more common as this is being called in custom code), in which case value_expr(values[.]) will always be NULL, so that wouldn't change anything.
- values are already SplitValue objects, in which case they (should) already have their expressions set (if they are not the default, which will still work in most cases), so they don't need to be changed.
Currently, as written, subset_expr ends up being ignored when values are already SplitValue objects.
This is defensible behavior, I think (as the construction time of the SplitValue is where the custom expression "should" be set), but probably not optimal. I'll change things slightly to make it easier to override subset expressions if (for some reason) you're reusing the same value object but want different subsetting behavior (which would be weird but I can think of a a corner case you might want to). As written originally you'd have been expected to reset the expressions on the value objects before passing to make_split_result
R/make_split_fun.R
Outdated
| } | ||
| } | ||
|
|
||
|
|
||
| .or_combine_exprs <- function(ex1, ex2) { |
There was a problem hiding this comment.
I remember us talking about making expression handling more general and contained in a single system of functions. Do you think it is worth it?
There was a problem hiding this comment.
I mean adding this to make_subset_expr (or is this too specific?)
R/00tabletrees.R
Outdated
| sub = .combine_subset_exprs( | ||
| pos_subset(parpos), | ||
| make_subset_expr(newspl, nsplitval) | ||
| pos_subset(parpos), | ||
| ## this will grab the value's custom subset expression if present | ||
| make_subset_expr(newspl, nsplitval) |
There was a problem hiding this comment.
this is the core effect of altering column col splits
tests/testthat/test_utils.R
Outdated
| @@ -1,6 +1,7 @@ | |||
| context("Checking utility functions") | |||
|
|
|||
| test_that("func_takes works with different inputs", { | |||
| func_takes <- rtables:::func_takes | |||
There was a problem hiding this comment.
This is not needed as tests access to all the namespace (with hidden functions)
| func_takes <- rtables:::func_takes |
This closes #785, allowing custom split functions, specifically functions which override the
corefunction, to specify subsetting expressions for the facets they generate, which is necessary for the custom split to function properly in column space.This is backwards compatible other than the fact that attempting to use a custom splitting function which overrides core splitting behavior in column space was previously an error, and any TableTree objects that have been serialized to rds/rda files would need to be refreshed as one of the underlying classes has changed. Tests have been added/updated for the new behavior