The mapping in `teal_slice` is lost #966

vedhav · 2023-11-20T13:59:57Z

Only the global_filters are retained when filter-module mapping is provided.

 filter = teal_slices(
    # filters created with id
    teal_slice(dataname = "mtcars", varname = "mpg", id = "filter 1"),
    teal_slice(dataname = "mtcars", varname = "cyl", id = "filter 2"),
    teal_slice(dataname = "mtcars", varname = "disp", id = "filter 3"),
    teal_slice(dataname = "mtcars", varname = "hp", id = "filter 4"),
    teal_slice(dataname = "mtcars", varname = "drat", id = "filter 5"),
    teal_slice(dataname = "mtcars", varname = "wt", id = "filter 6"),
    
    # filters mapped to modules
    mapping = list(
      "module 1" = c("filter 2", "filter 4"),
      "module 2" = "filter 3",
      "module 3" = "filter 2",
      global_filters = "filter 1"
    )
   # module_specific = FALSE
  )
filter

{
  "slices": [
    {
      "dataname"       : "mtcars",
      "varname"        : "mpg",
      "id"             : "filter 1",
      "fixed"          : false,
      "anchored"       : false,
      "multiple"       : true
    },
    {
      "dataname"       : "mtcars",
      "varname"        : "cyl",
      "id"             : "filter 2",
      "fixed"          : false,
      "anchored"       : false,
      "multiple"       : true
    },
    {
      "dataname"       : "mtcars",
      "varname"        : "disp",
      "id"             : "filter 3",
      "fixed"          : false,
      "anchored"       : false,
      "multiple"       : true
    },
    {
      "dataname"       : "mtcars",
      "varname"        : "hp",
      "id"             : "filter 4",
      "fixed"          : false,
      "anchored"       : false,
      "multiple"       : true
    },
    {
      "dataname"       : "mtcars",
      "varname"        : "drat",
      "id"             : "filter 5",
      "fixed"          : false,
      "anchored"       : false,
      "multiple"       : true
    },
    {
      "dataname"       : "mtcars",
      "varname"        : "wt",
      "id"             : "filter 6",
      "fixed"          : false,
      "anchored"       : false,
      "multiple"       : true
    }
  ],
  "attributes": {
    "allow_add"        : true,
    "mapping"          : {
      "global_filters" : "filter 1"
    },
    "module_specific"  : false
  }
}

The text was updated successfully, but these errors were encountered:

donyunardi · 2023-11-20T20:35:43Z

Thanks for catching this and I was able to reproduce this.
This is also a good opportunity to improve our unit tests.

gogonzo · 2023-11-21T05:03:53Z

Everything works if one specifies module_specific = TRUE. Right now module_specific = FALSE by default so when one specifies mapping only it is ignored. I think it makes sense to change module_specific = length(mapping) > 0L to automatically change to module specific when mapping is specified.

working example app with `module_specific = TRUE`

app <- teal::init(
  data = teal.data::teal_data(iris = iris, mtcars = mtcars),
  modules = modules(
    example_module("iris", datanames = "iris"),
    example_module("mtcars", datanames = "mtcars"),
    example_module("all", datanames = "all"),
    example_module("none", datanames = NULL)
  ),
  filter = teal_slices(
    # filters created with id
    teal_slice(dataname = "mtcars", varname = "mpg", id = "f-1"),
    teal_slice(dataname = "mtcars", varname = "cyl", id = "f-2"),
    teal_slice(dataname = "mtcars", varname = "disp", id = "f-3"),
    teal_slice(dataname = "mtcars", varname = "hp", id = "f-4"),
    teal_slice(dataname = "mtcars", varname = "drat", id = "f-5"),
    teal_slice(dataname = "mtcars", varname = "wt", id = "f-6"),
    teal_slice(dataname = "iris", varname = "Species", id = "f-7"),
    # filters mapped to modules
    module_specific = TRUE,
    mapping = list(
      "iris" = "f-7",
      "mtcars" = c("f-1", "f-2"),
      "all" = c("f-2", "f-3", "f-4", "f-5", "f-6", "f-7"),
      global_filters = "f-1"
    )
  )
)

shinyApp(app$ui, app$server)

donyunardi · 2023-11-21T05:19:00Z

Ah that's right! Forgot about the module_specific argument.

I think it makes sense to change module_specific = length(mapping) > 0L

Sounds good to me!

vedhav · 2023-11-21T06:04:10Z

While we're on this topic. Do you think having module-specific filters should be independent from the mapping?

I mean that the module_specific = TRUE will isolate the filters done inside each module so that the filters done in one module do not affect the other modules, while the mapping arguments provides an ability to add different module-specific filters. So, one can have different filters for different modules and having module_specific = FALSE will filter other modules with the same filter, else it will isolate the filter behavior to this module. So, one can still have global_filters that are present in all the modules but declaring module_specific = TRUE will make sure that the filter is done module-specific.

vedhav · 2023-11-21T06:42:28Z

Okay, I see that we always want to maintain the filter state of one dataset common across all the modules. So ignore my previous comment.
But, in this case I don't see the point of having module_specific argument at all. What are your thoughts on this?

chlebowa · 2023-11-21T08:59:19Z

Everything works if one specifies module_specific = TRUE. Right now module_specific = FALSE by default so when one specifies mapping only it is ignored. I think it makes sense to change module_specific = length(mapping) > 0L to automatically change to module specific when mapping is specified.

I disagree. I find computations in default values convoluted and needlessly confusing. My preferred version would be leave module_specific without default, accept only logical(1L) and confront with mapping internally. Describe defaulting behavior in docs.

chlebowa · 2023-11-21T12:55:20Z

@insightsengineering/nest-core-dev
We jumped the gun, gentlemen.

Upon closely reading of the documentation I notice that

There is no error in processing global/specific filters. What is missing is a module_specific = TRUE in the example app from the vignette that drew @vedhav's attention.

has only global filters with `"filter 1"` active - in accordance with `mapping`

library(teal)

app <- init(
  data = teal_data(mtcars = mtcars),
  modules = modules(
    example_module(label = "module 1"),
    example_module(label = "module 2"),
    example_module(label = "module 3"),
    example_module(label = "module 4")
  ),
  filter = teal_slices(
    # filters created with id
    teal_slice(dataname = "mtcars", varname = "mpg", id = "filter 1"),
    teal_slice(dataname = "mtcars", varname = "cyl", id = "filter 2"),
    teal_slice(dataname = "mtcars", varname = "disp", id = "filter 3"),
    teal_slice(dataname = "mtcars", varname = "hp", id = "filter 4"),
    teal_slice(dataname = "mtcars", varname = "drat", id = "filter 5"),
    teal_slice(dataname = "mtcars", varname = "wt", id = "filter 6"),
    
    module_specific = FALSE,
    
    # filters mapped to modules
    mapping = list(
      "module 1" = c("filter 2", "filter 4"),
      "module 2" = "filter 3",
      "module 3" = "filter 2",
      global_filters = "filter 1"
    )
  )
)

shinyApp(app$ui, app$server)

has module-specific filters in accordance with mapping

library(teal)

app <- init(
    data = teal_data(mtcars = mtcars),
    modules = modules(
        example_module(label = "module 1"),
        example_module(label = "module 2"),
        example_module(label = "module 3"),
        example_module(label = "module 4")
    ),
    filter = teal_slices(
        # filters created with id
        teal_slice(dataname = "mtcars", varname = "mpg", id = "filter 1"),
        teal_slice(dataname = "mtcars", varname = "cyl", id = "filter 2"),
        teal_slice(dataname = "mtcars", varname = "disp", id = "filter 3"),
        teal_slice(dataname = "mtcars", varname = "hp", id = "filter 4"),
        teal_slice(dataname = "mtcars", varname = "drat", id = "filter 5"),
        teal_slice(dataname = "mtcars", varname = "wt", id = "filter 6"),
        
        module_specific = TRUE,
        
        # filters mapped to modules
        mapping = list(
            "module 1" = c("filter 2", "filter 4"),
            "module 2" = "filter 3",
            "module 3" = "filter 2",
            global_filters = "filter 1"
        )
    )
)

shinyApp(app$ui, app$server)

And now we know why the module_specific and mapping defaults are what they are. Everything is nice and explicit.

So all we have to do is add module_specific = TRUE in the vignette.

@vedhav, will you do the honors?

chlebowa · 2023-11-21T12:59:17Z

Perhaps we should make the documentation clearer still. I suggest reversing the order of possible values of module_specific and adding "(default)" to the description of FALSE.

kartikeyakirar · 2023-11-21T13:15:19Z

oh I completely overlooked documentation the details of the mapping. so proposed changes in https://github.com/insightsengineering/teal/pull/968/files are no longer required ?

chlebowa · 2023-11-21T13:17:06Z

I don't believe so.

kartikeyakirar · 2023-11-21T14:05:56Z

So all we have to do is add module_specific = TRUE in the vignette.

I have added another PR for document change #970.

lcd2yyz · 2023-11-21T22:54:39Z

Given the example @chlebowa has shown, does it make sense to set default module_specific = length(mapping[names(mapping) != "global_filters"]) > 0. Afterall, if an user goes through the trouble of specifying all the module-specific mappings, he/she most likely have the intention of applying it.

chlebowa · 2023-11-22T08:04:48Z

@lcd2yyz
As I recall (after my investigation yesterday) we wanted to make module_specific the senior argument, a gatekeeper to module-specificity, if you will. It's default should therefore be a simple flag.
I would consider whether we should disregard mapping altogether when module_specific = FALSE.
The thing is I remember it took a lot of thinking to arrive at the current state so It should be rather solid. I wish we had left a comment.
I suggest we rethink it when time permits.

lcd2yyz · 2023-11-23T23:57:24Z

Fair point @chlebowa
In the meantime, I would suggest to add a warning/message to notify user that configuration in mapping is not applied and ignored, when module_specific = FALSE but mapping is not empty (if this is not implemented already).

chlebowa · 2023-11-24T10:03:41Z

The thing is that warning will multiply:

Warning messages:
1: In teal_slices(teal_slice(dataname = "mtcars", varname = "mpg", ... :
 "module_specific" set to FALSE. Disregarding "mapping", only global filters will be applied. 
2: In (function (..., exclude_varnames = NULL, include_varnames = NULL, ... :
 "module_specific" set to FALSE. Disregarding "mapping", only global filters will be applied.

This is because of how mapping is handled internally. mapping$global_filters is used always, even in non-module-specific applications. Therefore, even if module_specific = FALSE, the mapping attribute is created and all teal_slices are placed there. Now, as the teal_slices object is handled by the filter managed, every time a slice is added with c, teal_slices is called and it receives a module_specific = FALSE and mapping with only global filters. Then the warning is fired again. Every time.

I say we leave it as is. The documentation is good enough, though requires careful reading. There are also "experimental" flags in there.

Closes insightsengineering#966. Here as per discussion I have added `module_specific` parameter in vignettes example and updated documentation for `module_specific `paramter --------- Signed-off-by: kartikeya kirar <[email protected]> Co-authored-by: Aleksander Chlebowski <[email protected]> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: m7pr <[email protected]>

vedhav added bug Something isn't working core labels Nov 20, 2023

donyunardi removed the bug Something isn't working label Nov 21, 2023

kartikeyakirar self-assigned this Nov 21, 2023

kartikeyakirar mentioned this issue Nov 21, 2023

fixes the relationship between module_specific and mapping. #968

Closed

kartikeyakirar mentioned this issue Nov 21, 2023

update documentation for teal_slices #970

Merged

kartikeyakirar closed this as completed in #970 Nov 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The mapping in `teal_slice` is lost #966

The mapping in `teal_slice` is lost #966

vedhav commented Nov 20, 2023 •

edited by gogonzo

Loading

donyunardi commented Nov 20, 2023

gogonzo commented Nov 21, 2023

donyunardi commented Nov 21, 2023 •

edited

Loading

vedhav commented Nov 21, 2023

vedhav commented Nov 21, 2023

chlebowa commented Nov 21, 2023

chlebowa commented Nov 21, 2023 •

edited

Loading

chlebowa commented Nov 21, 2023

kartikeyakirar commented Nov 21, 2023

chlebowa commented Nov 21, 2023

kartikeyakirar commented Nov 21, 2023

lcd2yyz commented Nov 21, 2023

chlebowa commented Nov 22, 2023

lcd2yyz commented Nov 23, 2023

chlebowa commented Nov 24, 2023

The mapping in teal_slice is lost #966

The mapping in teal_slice is lost #966

Comments

vedhav commented Nov 20, 2023 • edited by gogonzo Loading

donyunardi commented Nov 20, 2023

gogonzo commented Nov 21, 2023

donyunardi commented Nov 21, 2023 • edited Loading

vedhav commented Nov 21, 2023

vedhav commented Nov 21, 2023

chlebowa commented Nov 21, 2023

chlebowa commented Nov 21, 2023 • edited Loading

has only global filters with "filter 1" active - in accordance with mapping

has module-specific filters in accordance with mapping

chlebowa commented Nov 21, 2023

kartikeyakirar commented Nov 21, 2023

chlebowa commented Nov 21, 2023

kartikeyakirar commented Nov 21, 2023

lcd2yyz commented Nov 21, 2023

chlebowa commented Nov 22, 2023

lcd2yyz commented Nov 23, 2023

chlebowa commented Nov 24, 2023

The mapping in `teal_slice` is lost #966

The mapping in `teal_slice` is lost #966

vedhav commented Nov 20, 2023 •

edited by gogonzo

Loading

donyunardi commented Nov 21, 2023 •

edited

Loading

chlebowa commented Nov 21, 2023 •

edited

Loading

has only global filters with `"filter 1"` active - in accordance with `mapping`