Filter mapping by module ID instead of module label

[pawelru]

Conceptually, I find it quite odd to map filter to module labels. It should be a module identifier instead. A label should be a free text but we tend to use it as identifier which is probably not the right thing to do.

@donyunardi comment from #972

Currently, this is how users define module specific mapping

app <- init(
  data = cdisc_data(...),
  modules = teal::modules(
    teal.modules.general::tm_data_table(
      label = "ADS Data Table",
      variables_selected = list(ADSL = c("STUDYID", "USUBJID", "SUBJID", "SITEID", "AGE", "SEX", "COUNTRY")),
      dt_args = list(caption = "ADSL Table Caption")
    ),
  filter = teal_slices(
    teal_slice("ADSL", "COUNTRY", "country", selected = "USA", fixed = TRUE),
    teal_slice("ADSL", "ETHNIC", "ethnic", anchored = TRUE),
    module_specific = TRUE,
    mapping = list(
      "ADS Data Table" = c("country", "ethnic") # using the module's label to assign filters
    )
  )
)

We have an assertion to check for duplicate labels, which is good.

However, can we explore the possibility of using a module's id for this assignment? This could involve altering the module's structure and introducing the id in the module's server.

Here's what the code could look like:

app <- init(
  data = cdisc_data(...),
  modules = teal::modules(
    teal.modules.general::tm_data_table(
      id = "module1",
      label = "ADS Data Table",
      variables_selected = list(ADSL = c("STUDYID", "USUBJID", "SUBJID", "SITEID", "AGE", "SEX", "COUNTRY")),
      dt_args = list(caption = "ADSL Table Caption")
    ),
  filter = teal_slices(
    teal_slice("ADSL", "COUNTRY", "country", selected = "USA", fixed = TRUE),
    teal_slice("ADSL", "ETHNIC", "ethnic", anchored = TRUE),
    module_specific = TRUE,
    mapping = list(
      "module1" = c("country", "ethnic") # using the module's id to assign filters
    )
  )
)

[chlebowa]

Aren't module ids created from the labels behind the scenes and never made available to the user?

[pawelru]

Yes and I don't feel it's correct. Like I said earlier, a label should be a free text, in particular: an empty character or non-unique values between modules. These all should be allowed but it's not because we treat labels as identifier.

[chlebowa]

That is a fair point, however, juggling labels AND ids is additional burden on the user.

[pawelru]

Yes. We can probably came up with some convenience features but I agree that managing two attributes is more or less twice as difficult as managing just one.

This doesn't change the point that IMHO right now labels are misused as ids. Recent rollout of mapping feature just exposed such flaw. Until this, the concept of module id was just useless for the user. I think we need to rethink the concept of module ID. Until a new not-yet-anticipated feature / extension that requires an ID.

[lcd2yyz]

I remember we discussed this briefly during the design of the module-specific mechanism, but decided to stick with the labels for the sake of moving on, because there weren't clear/better alternatives at the time. I concur @pawelru's thoughts, thanks for raising the topic again.

[gogonzo]

We are in dead end with this one.

1. Issue with mapping argument - even if we decide to do following

modules(
  tm_xy("mod1", ,..., filters = teal_slices(teal_slice_age, teal_slice_sex),
  modules(
    label = "tab",
    tm_xy("mod1", ...., filters = teal_slices(teal_slice_age)
  )
)

We would be able to map modules properly together even if they are nested.

2. Problem with displaying mapping in the app - here situation is different because we need to have a visual depresentation of these modules. For above example we would display (currently it is not possible because we don't allow duplicated labels when module-specific):

filter	mod1	mod 1
AGE	x	x
SEX	x	o

And we rather should display this information in following way:

filter	mod1	tab > mod 1
AGE	x	x
SEX	x	o

[gogonzo]

Another solutions are:

1. teal to force label change when duplicated.
2. Forbid duplicated modules names even if nested modules

Just a reminder of early findings: https://github.com/insightsengineering/teal/issues/534

[pawelru]

That's exactly what I was referring to. Use ID instead of label which should remain as a free text that we can further combine or anything.

---

How about the following: add optional (!) id argument to the module. We can make a reasonable default value (e.g. sprintf("%s_%s", short_hash(Sys.time()), label_to_id(label)) OR auto-create it inside the init(). Regardless of this, we would also need to add uniqueness check somewhere inside init().

This would address @chlebowa concern about throwing yet another technical requirement to the app developer which is totally valid point.

Problem with displaying mapping in the app

I believe this would be sorted out as well. We will be having the following structure:

[
  {
    id: some_id1, 
    label: "mod1", 
    filters: [...]
  },
  {
    id: some_id2,
    label: "tab > mod2",
    filters: [...]
  }
]

From this, you can easily visualise this as a table and skip ID at all.