insightsengineering / teal.slice

Reproducible slice module for teal applications
https://insightsengineering.github.io/teal.slice/
Other
11 stars 5 forks source link

filter calls in `FilterState` #535

Closed chlebowa closed 8 months ago

chlebowa commented 8 months ago

The is.na flag is always added to the filter predicate in FilterState, which is redundant when specific values are selected. Did we mess something up in https://github.com/insightsengineering/teal.slice/pull/402? I remember the review was thorough, so this may be deliberate but I honestly forget. It doesn't break anything, the calls are valid.

filter_state <- ChoicesFilterState$new(
  x = c(LETTERS, NA),
  slice = teal_slice(varname = "var", dataname = "data", selected = "A", keep_na = FALSE)
)
isolate(filter_state$get_call())
!is.na(var) & var == "A"
m7pr commented 8 months ago

This is fine with what we stated. If we have a single selection, NAs in data na keep_na = FALSE, then !is.na(var) & x == var should be returned. Check this table https://github.com/insightsengineering/teal.slice/pull/402#issuecomment-1649204484 and this cell

image
m7pr commented 8 months ago

And we later decided to return !is.na(x) also for multiple selection

> filter_state <- teal.slice:::ChoicesFilterState$new(
+   x = c(LETTERS, NA),
+   slice = teal_slice(varname = "var", dataname = "data", selected = c("A", "B"), keep_na = FALSE)
+ )
> isolate(filter_state$get_call())
!is.na(var) & var %in% c("A", "B")

https://github.com/insightsengineering/teal.slice/pull/402#issuecomment-1659847958

chlebowa commented 8 months ago

Awesome, thank you :+1:

m7pr commented 8 months ago

no worries, thanks for double checking and flaggin!