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

slider snaps from edge in RangedFilterState #189

Merged
merged 35 commits into from Feb 13, 2023
Merged

Conversation

chlebowa
Copy link
Contributor

@chlebowa chlebowa commented Feb 3, 2023

Solves #184.

Range of numeric variable is prettified first, before anything is created.
Slider is created based on prettified range (as previously).
Initial selection is set based on prettified range (previously on actual range).
Condition call is based on prettified range (previously on actual range).
Formatted output is based on actual range and values (as previously).

@chlebowa
Copy link
Contributor Author

chlebowa commented Feb 3, 2023

Use this app to test (thanks @nikolas-burkoff):
(edited)

library(shiny)
library(shinyjs)

repeat {
  x <- c(0.0091, 0.01 + runif(50, max= 0.99))
  if (identical(range(pretty(x, 100)), c(0, 1))) break
}

my_filter_state <- teal.slice:::init_filter_state(
  x = x,
  varname = "VARNAME",
  varlabel = "A label"
)

#####

ui <- fluidPage(
  useShinyjs(),
  column(4,
         div(  # only need a div so that we can hide the ui when the close button is pressed
           id = "x",
           my_filter_state$ui("fs"),
           h4("Actual data range"),
           textOutput("selection_range"),
           h4("Condition (i.e. call)"), # show the condition generated by this FilterState
           textOutput("condition"),
           h4("Formatted state"), # show the human readable state
           textOutput("formatted"),
           h4("Unformatted state"), # show the raw state
           textOutput("unformatted"),
         )
  ),
  column(3,  actionButton("set_filter", "Set filter state to [0.0091, 1]"))
)

server <- function(input, output, session) {
  
  output$condition <- renderPrint(my_filter_state$get_call())
  output$formatted <- renderText(my_filter_state$format())
  output$unformatted <- renderPrint(my_filter_state$get_state()) # also get_selected(), get_keep_na
  
  output$selection_range <- renderPrint(range(x))
  
  signal_to_remove <- my_filter_state$server("fs")  # filterstate$server returns a trigger when the close button is pressed
  observeEvent(signal_to_remove(), shinyjs::hide("x")) # and shinyjs removes the ui
  
  observeEvent(input$set_filter, my_filter_state$set_selected(c(0.0091, 1)))
}

shinyApp(ui, server)

Copy link
Contributor

@nikolas-burkoff nikolas-burkoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As we're going into main we should have NEWS? We may need a test or two?
Also merge into refactor branch

Could there be a problem in teal that app developers have specified a specific filter i.e. (list(ADSL = list(x = selected(0.0009, 1)) and that gets automatically rounded for users?

Add actionButton("set_filter", "Set filter state to [0.0091, 1]") into the test app UI and observeEvent(input$set_filter, my_filter_state$set_selected(c(0.0091, 1))) into its server function

I wouldn't spend too much time on this though as we're moving to allow users to enter a range and not just pick from a slider and at that point we can handle this fully. Please add your thoughts about this (and the complexities of jumping between slider and range here)

R/FilterState.R Outdated Show resolved Hide resolved
},
error = function(error) stop("The array of set values must contain values coercible to numeric.")
)
values <- as.numeric(values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need a suppresswarnings here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous version these warnings were never actually triggered.
They could be suppressed or they could be logged, depending on how we want to report the set range.

R/FilterStateRange.R Outdated Show resolved Hide resolved
R/FilterStateRange.R Outdated Show resolved Hide resolved
@chlebowa
Copy link
Contributor Author

chlebowa commented Feb 7, 2023

Could there be a problem in teal that app developers have specified a specific filter i.e. (list(ADSL = list(x = selected(0.0009, 1)) and that gets automatically rounded for users?

Add actionButton("set_filter", "Set filter state to [0.0091, 1]") into the test app UI and observeEvent(input$set_filter, my_filter_state$set_selected(c(0.0091, 1))) into its server function

Works just as well with these modifications, remove_out_of_bound_values does raise warnings. I guess you are worried that the lower limit is set to 0.0009 but the slider displays 0.01?

Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'd go further without a need to calculate pretty_choices twice in the initialize and ui_inputs.

          teal.widgets::optionalSliderInput(
            inputId = ns("selection"),
            label = NULL,
            min = private$choices[1],
            max = private$choices[2],
            value = private$choices,
            step = private$step or private$choices$step
            width = "100%"
          )
  • Reason why I requested selected and choices to be identical in initialize:
    Please be aware of the method is_any_filtered which checks if the choices and selected are the same. If they identical it means that fata doesn't have to be filtered so condition is not returned.
    is_any_filtered = function() {
      if (!setequal(self$get_selected(), private$choices)) {
        TRUE
      } else if (!isTRUE(self$get_keep_inf()) && private$inf_count > 0) {
        TRUE
      } else if (!isTRUE(self$get_keep_na()) && private$na_count > 0) {
        TRUE
      } else {
        FALSE
      }
    },

R/FilterStateRange.R Outdated Show resolved Hide resolved
R/FilterStateRange.R Outdated Show resolved Hide resolved
@chlebowa
Copy link
Contributor Author

chlebowa commented Feb 8, 2023

Unfortunately, the initialize method cannot be simplified that much because there is an edge case of the variable having range 0 (i.e. all values are the same).

@chlebowa
Copy link
Contributor Author

chlebowa commented Feb 9, 2023

See here for how Nik's button problem is "solved".

This means the subsetting call need not contain the exact values passed to the filter API. E.g. one may wish to filter on this:

list(
      mtcars = list(
        hp = list(selected = c(52, 65), keep_na = FALSE, keep_inf = FALSE)
      )
    )

but the variable in question is:

52 62 65 66 66 91 (...)

thus the prettified range is:

50 55 60 65 70 75 (...)

and so the cal will be >=52 rather than >=50.

R/FilterStateRange.R Outdated Show resolved Hide resolved
@chlebowa chlebowa marked this pull request as ready for review February 10, 2023 13:25
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2023

Unit Test Performance Difference

Test Suite $Status$ Time on main $±Time$ $±Tests$ $±Skipped$ $±Failures$ $±Errors$
FilterState-utils 👶 $+0.06$ $+12$ $0$ $0$ $0$
Additional test case details
Test Suite $Status$ Time on main $±Time$ Test Case
FilterState-utils 👶 $+0.05$ contain_interval_accepts_proper_arguments
FilterState-utils 👶 $+0.00$ contain_interval_returns_containing_range
FilterState-utils 👶 $+0.00$ contain_interval_returns_range_if_x_is_x_is_out_of_bounds
FilterState-utils 👶 $+0.00$ contain_interval_returns_x_if_interval_matches_ticks
FilterState 💀 $0.01$ $-0.01$ private_get_pretty_range_inputs_returns_nicely_rounded_values
FilterState 💀 $0.06$ $-0.06$ private_get_pretty_range_step_returns_pretty_step_size
RangeFilterState 👶 $+0.06$ private_get_pretty_range_step_returns_pretty_step_size
RangeFilterState 💀 $0.01$ $-0.01$ set_selected_defaults_to_the_lower_and_the_upper_bound_of_the_possible_range_if_the_passed_values_exceed_the_possible_range
RangeFilterState 👶 $+0.01$ set_selected_raises_error_when_the_passed_values_are_not_coercible_to_numeric
RangeFilterState 💀 $0.01$ $-0.01$ set_selected_throws_an_error_if_selecting_completely_outside_of_the_possible_range
RangeFilterState 💀 $0.01$ $-0.01$ set_selected_throws_when_the_passed_values_are_not_coercible_to_numeric
RangeFilterState 💀 $0.01$ $-0.01$ set_selected_warns_when_the_selected_range_intersects_the_possible_range_but_is_not_fully_included_in_it

Results for commit bb9a1ea

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2023

badge

Code Coverage Summary

Filename                      Stmts    Miss  Cover    Missing
--------------------------  -------  ------  -------  -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
R/call_utils.R                  156       4  97.44%   141-144
R/choices_labeled.R              49      14  71.43%   19, 30, 35, 45-50, 62, 66-70
R/filter_state_api.R             17      17  0.00%    137-172
R/FilteredData-utils.R           98      43  56.12%   126-131, 216-263
R/FilteredData.R                505     267  47.13%   139, 249-253, 302-316, 368-382, 717-849, 877-879, 883-886, 890-894, 898-900, 929-935, 942-944, 946-947, 955, 961-966, 984-989, 1012-1020, 1023-1048, 1063-1078, 1130-1151, 1175
R/FilteredDataCDISC.R           152      28  81.58%   92, 115, 124, 128, 142-151, 255-262, 280-283, 289, 354-357
R/FilteredDataset.R             188      77  59.04%   103, 199, 298, 362-419, 461-463, 502-508, 545-555
R/FilteredDatasetDefault.R      106       8  92.45%   166-173
R/FilteredDatasetMAE.R          208      59  71.63%   28, 119, 251-327
R/FilterPanelAPI.R               23       8  65.22%   88, 119-125
R/FilterState-utils.R           131      14  89.31%   154, 253-259, 271-276
R/FilterState.R                 230      88  61.74%   114-115, 236, 362-408, 521-593
R/FilterStateChoices.R          153      96  37.25%   149-155, 168, 194-295
R/FilterStateDate.R             163     108  33.74%   138-144, 157, 161, 192-308
R/FilterStateDatettime.R        223     164  26.46%   61-63, 156-162, 176, 180, 211-384
R/FilterStateEmpty.R             41      16  60.98%   136-162
R/FilterStateLogical.R          137      86  37.23%   136-142, 156, 172-264
R/FilterStateRange.R            229     136  40.61%   259-265, 275-276, 280-286, 303-460
R/FilterStates-utils.R           68       5  92.65%   96, 113, 160, 206, 227
R/FilterStates.R                226      93  58.85%   94-100, 170, 229, 319-513, 530-531, 540
R/FilterStatesDF.R              230      29  87.39%   85-87, 91-99, 101, 105-107, 109, 275-284, 343, 412
R/FilterStatesMAE.R             221     106  52.04%   28, 88-93, 232-354, 371
R/FilterStatesMatrix.R          217     108  50.23%   37, 69-74, 82-83, 85, 133-134, 220-338
R/FilterStatesSE.R              409     201  50.86%   22, 88-93, 126-131, 138-139, 141, 302-311, 346-353, 361-368, 396-565
R/include_css_js.R                5       5  0.00%    12-16
R/resolve_state.R                23       0  100.00%
R/utils.R                        39       2  94.87%   101-102
R/variable_types.R               48      33  31.25%   42-47, 57, 70-105
R/zzz.R                           6       6  0.00%    3-11
TOTAL                          4301    1821  57.66%

Diff against main

Filename                    Stmts    Miss  Cover
------------------------  -------  ------  -------
R/FilterState-utils.R          +5       0  +0.42%
R/FilterStateChoices.R         -3      -3  +0.72%
R/FilterStateDate.R            +1       0  +0.41%
R/FilterStateDatettime.R       +1       0  +0.33%
R/FilterStateLogical.R         -3      -3  +0.80%
R/FilterStateRange.R          -27      -8  -3.14%
TOTAL                         -26     -14  +0.07%

Results for commit: bb9a1ea

Minimum allowed coverage is 80%

♻️ This comment has been updated with latest results

Aleksander Chlebowski added 2 commits February 10, 2023 14:41
@github-actions
Copy link
Contributor

github-actions bot commented Feb 10, 2023

Unit Tests Summary

    1 files    29 suites   23s ⏱️
423 tests 423 ✔️ 0 💤 0
747 runs  747 ✔️ 0 💤 0

Results for commit 02ac57a.

♻️ This comment has been updated with latest results.

@gogonzo gogonzo self-assigned this Feb 13, 2023
Copy link
Contributor

@gogonzo gogonzo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat 👍 please merge main to the feature branch afterwards

@chlebowa chlebowa merged commit 204fb5c into main Feb 13, 2023
@chlebowa chlebowa deleted the 184_slider_moves@main branch February 13, 2023 13:58
This was referenced Feb 13, 2023
gogonzo added a commit that referenced this pull request Feb 14, 2023
Merging `main` branch into `filter_panel_refactor`  following #189.

---------

Co-authored-by: chlebowa <chlebowa@users.noreply.github.com>
Co-authored-by: Dawid Kałędkowski <dawid.kaledkowski@gmail.com>
@mhallal1 mhallal1 mentioned this pull request Feb 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants