Allow to set datanames not identical with object names

[pawelru]

library(tern)
#> Loading required package: rtables
#> Loading required package: formatters
#> Loading required package: magrittr
#> 
#> Attaching package: 'rtables'
#> The following object is masked from 'package:utils':
#> 
#>     str
#> Registered S3 method overwritten by 'tern':
#>   method   from 
#>   tidy.glm broom
library(teal.data)
#> Loading required package: shiny
library(magrittr)

data <- cdisc_data() %>%
  within({
    adsl <- tern_ex_adsl %>%
      df_explicit_na()

    adtte <- tern_ex_adtte %>%
      df_explicit_na()
  })
datanames(data)
#> character(0)
datanames(data) <- c("ADSL", "ADTTE")
#> Error in `datanames<-`(`*tmp*`, value = c("ADSL", "ADTTE")): invalid name
data
#> <environment: 0x109b2c350> [L]
#> Parent: <environment: package:teal.data>
#> Bindings:
#> • adtte: <tibble[,28]> [L]
#> • adsl: <tibble[,21]> [L]

^{Created on 2023-12-04 with reprex v2.0.2}

We probably need to enable a dict / named character vector as an argument as a new datanames.

Also - please enhance error message. We should return not match found type of thing instead.

[chlebowa]

Also - please enhance error message. We should return not match found type of thing instead.

Way ahead of you, done on refactor branch.

We probably need to enable a dict / named character vector as an argument as a new datanames.

Wouldn't that defeat the purpose of datanames?

[pawelru]

We probably need to enable a dict / named character vector as an argument as a new datanames.

Wouldn't that defeat the purpose of datanames?

Could you please elaborate more?

In my example I want to do the following: datanames(data) <- c("adsl" = "ADSL", "adtte" = "ADTTE")
but it could be also like this: datanames(data) <- c("x" = "ADSL", "y" = "ADTTE")

I see nothing wrong right now in the above.

UPDATE: I made a mistake and updated snippets to use datanames(data)<-

[chlebowa]

What you should do is either

data <- cdisc_data() %>%
  within({
    adsl <- tern_ex_adsl %>%
      df_explicit_na()

    adtte <- tern_ex_adtte %>%
      df_explicit_na()
  })
datanames(data) <- c("adsl", "adtte")

or

data <- cdisc_data() %>%
  within({
    ADSL <- tern_ex_adsl %>%
      df_explicit_na()

    ADTTE <- tern_ex_adtte %>%
      df_explicit_na()
  })
datanames(data) <- c("ADSL", "ADTTE")

[pawelru]

I did exactly this not to be blocked but I feel it would be a good enhancements to avoid this and address this in datanames setter instead. It's much easier and more convenient for the app developer.

The use case I am facing is that I want to use teal.modules.clinical which has uppercase datanames hardcoded. Hence option A is not really feasible and I need to stick with B. Now I don't really want to alter my data loading code (everything what's in within) which usually is quite lenghty. I can imagine the scenario that such code is impossible to modify or users will not know how to do that. I would prefer to do this in one liner outside of data loading code.
(*) I can of course just append renaming code to the loading code but again - setter is simpler.

Other than that I also see the requirement of dataname to be identical with object name as probably too strict. I cannot find a good reason why x object cannot represent dataset with dataname "y". On the other hand, if this is correct (which I still don't think so) - why do we need datanames at all and cannot use obj names if those needs to be identical? This could be derived automatically.

[chlebowa]

The purpose of datanames is to flag variables in the teal_data object's environment as datasets as opposed to any arbitratry auxiliary variables. This information is necessary when the teal_data is confronted with the datanames property of modules as modules may only send datasets to the filter panel. It cannot be derived automatically because there is no way of knowing what non-dataset variables have been created in the environment.

It is not a label, it is a specification.

[pawelru]

Thank you for the explanation. Now I think I understand what is happening and I have a bad conclusion (please correct me if I made a mistake in my thinking).

To me we have made a non-backwards compatible change (luckily not released yet) with regard to the change of the meaning of a "datanames".
What you have described (what's currently implemented in the main/refactor branch) is an identifier - an user needs to specify which objects inside currently stored environemnt are datasets so s/he needs to give us environment object names. This is not a definition of "dataname" as we used to have. "dataname" was sort of a label hence it could have more business meaning (e.g. "ADSL" or "the_main_dataset" (not quite sure if even: "the main dataset")). Identifier is more technical entity tightly coupled with what's in the environment that code produces.
Previously it was ok to have the following: dataset(x, dataname = "ADSL") and right now it's not.

(I think now) I understand what's on the main branch and I do agree. But it seems that we have applied a new (more technical) responsibility on the business field / entity and I think it's wrong. It might happen that this is carried forward to the teal.slice and then to the modules. In particular: data[["<object_name>"]]() vs data[["<dataname>"]](). Here I don't yet an opinion which one is correct but it's clearly different to what's released.

Please tell me what's your view on that. Maybe everything is what it should be but then we still need to make it clear in the release notes.
CC @donyunardi @lcd2yyz @gogonzo for awareness / comments.

[chlebowa]

Note that the dataset function is deprecated so dataname in that sense is no longer relevant.

Also, data[["<object_name>"]]() is not valid anymore because data is not a list of reactives. We use data()[["<var_name>"]] in modules because modules receive a reactive that contains teal_data. As it happens, modules only deal with variables in teal_data that are datasets, so data()[["<dataname>"]] is correct but it is a special (if common) case.

[gogonzo]

I think datanames is very similar conceptually to the previous design. Previously objects were de facto renamed by specifying dataset(dataname, ...) argument. It is possible to do this now and table below proves that two approaches could be equivalent.

Previous	Now
x <- teal.data::rADSL teal_data( dataset("ADSL", x = x), dataset("ADTTE", x = teal.data::rADTTE) )	x <- teal.data::rADSL teal_data( ADSL = x, ADTTE = teal.data::rADTTE )

However, datanames concept means something else internally for the class. Previously "dataname" referred to the object in the data$get_datasets(<dataname>)$raw_data, and now it refers directly to the bindings name in @env[[<dataname>]].

I personally think that previous design was even more confusing. In some internal operations there datanames also referred to the objects in the temporary environments. I don't want to get to the depths of the previous implementation. IMO it is important how different it is for the users now. And as you saw on the table above - change is minimal but still reasonably simplifying.

---

I admit, that specifying datanames when using teal_data() (recommended solution) is something annoying. We will have a discussion about this later this week or next week.

data <- teal_data() |> within(adsl = teal.data::rADSL)
datanames(data)
# character(0)

[pawelru]

Thanks Dawid. I was aware that I could still do teal_data(ADSL = x, ...) but wanted to do this in a recommended way with teal_data() |> within(...) instead.

I think that with a (small?) change in datanames setter we can address this problem and be meaning backwards compatible and also be compatible with all the upstream functionalities.

How about the following (pseudo-code):

set_datanames <- function(data, datanames) {
  for ((x_name, x_val) in datanames) {
    if (!is.null(x_name)) {
      # push an additional assignment call
      data <<- within(data, parse_call({x_name} <- {x_val})
      x_dataname <- x_name
    } else {
      x_dataname <- x_val
    }
    
    (additional assertions on x_dataname)
    
    data@datanames <- append(data@datanames, x_dataname)
  }
}

Then we could support the following: teal_data() |> within({x = ...; y = ...}) |> set_datanames(ADSL = x, ADRS = y)

Please take that into consideration in the upcoming discussions.