Biogen-Inc / IDEAFilter

An R Shiny Filtering module. Demo the shiny module at: https://bit.ly/demo_IDEAFilter
https://biogen-inc.github.io/IDEAFilter/
Other
11 stars 2 forks source link

Allow selection of vectors and filters upon initialization #20

Closed Jeff-Thompson12 closed 11 months ago

Jeff-Thompson12 commented 11 months ago

Addresses #13

Jeff-Thompson12 commented 11 months ago

@AARON-CLARK what do you think of the approach outlined in this PR?

Things I like about it:

Things I don't like:

Here's a snippet to see the preselection in action:

ui <- fluidPage(
  titlePanel("Filter Data Example"),
  fluidRow(
    column(8,
      verbatimTextOutput("data_summary"),
      verbatimTextOutput("data_filter_code")),
    column(4, IDEAFilter::shiny_data_filter_ui("data_filter"))))

srv <- function(input, output, session) {
  filtered_data <- callModule(
    IDEAFilter::shiny_data_filter,
    "data_filter",
    data = airquality,
    preselection = list(Ozone = list(filter_na = TRUE, filter_fn = ~.x <= 100),
                        Solar.R = list()),
    verbose = FALSE)

  output$data_filter_code <- renderPrint({
    cat(gsub("%>%", "%>% \n ",
      gsub("\\s{2,}", " ",
        paste0(
          capture.output(attr(filtered_data(), "code")),
          collapse = " "))
    ))
  })

  output$data_summary <- renderPrint({
    if (nrow(filtered_data())) show(filtered_data())
    else "No data available"
  })
}

shinyApp(ui, srv)
Jeff-Thompson12 commented 11 months ago

Why can't it be last? so it doesn't introduce breaking changes to users who may not be specifying the arg name with the module call?

It can be last. In my mind the verbose argument makes sense as the last value, but at this point new arguments should be added to the end of functions already being exported.

Jeff-Thompson12 commented 11 months ago

I was playing around with the starwars example app in our inst/ folder by adding the following code:

filtered_data <- callModule(
   shiny_data_filter, 
   "data_filter", 
   data = starwars2,
   preselection = list(name = list(filter_na = TRUE, filter_fn = ~stringr::str_detect(.x, '[0-9]')),
                       eye_color = list(filter_na = TRUE, filter_fn = ~ c("yellow","red") %in% .x),
                       Solar.R = list()),
   verbose = FALSE)

I did notice that my filter on eye_color didn't really work. For some reason, it's still including "black" eye colors.

image

So I noticed that your filter is backwards (i.e. you have c("yellow", "red") %in% .x instead of .x %in% c("yellow", "red")). This is causing Filter() to apply a logical vector of length 12 instead of 6 giving the bizarre results. I can't decide if this is a problem for the PR thought because R is behaving in the expected way.

Jeff-Thompson12 commented 11 months ago

Yes, it is slightly confusing if your a developer and you think your filtering to < 100. It's a shame the generated code doesn't reflect the filter_fn. Do you think it could some how?

image

Not really. I was going down that route at the start, but then you have to account for the different methods. We could allow for other arguments (e.g. if a user provides a max or range value it supersedes filter_fn), but that type of processing is not very intuitive.

Plus since the filter has a waterfall type affect, you couldn't force the values anyways. I think the main thing is that the filtering code is meant to be reproducible on the same dataset, not across datasets.