tmc shinyvalidate

[nikolas-burkoff]

Closes #688

This uses the teal.transform PR insightsengineering/teal.transform#112 to validate the d-e-s

Modules will now behave like this:

[asbates]

In the tm_g_ci example there's a couple of filter_specs (select lab, screening). I think these should have validation as well b/c otherwise the plot is empty. You mentioned in insightsengineering/teal.transform/pull/113 that filter_spec validation updates weren't needed for tmc. But I think filter_spec should have the same validation for consistency.

[nikolas-burkoff]

In the tm_g_ci example there's a couple of filter_specs (select lab, screening). I think these should have validation as well b/c otherwise the plot is empty. You mentioned in insightsengineering/teal.transform#113 that filter_spec validation updates weren't needed for tmc. But I think filter_spec should have the same validation for consistency.

So it's not that they weren't needed it's that they might not be feasible or the api for adding them is just too clunky - in principle all data-extract-specs can have filter and select specs - most tmc modules require only choices_selected (but some also allow the full d-e-s to be inputted) which are generally converted to select spec's - hence the filter spec not being too important.

So feel free to take a look in teal.transform to get filter spec validation added (and cover the cases where there is also select validation/and or dataset validation :) - though having said that I wonder if for the CI case we should add some validation with a shiny::validate which checks once you've got the data that you have enough data points for the plot -> CI doesn't use validate_standard_inputs like other modules - maybe it should?

[asbates]

I'm wondering if the messages in the main panel should be highlighted more. I know the inputs themselves have red. I'm not entirely sure it would be obvious to all users that all the errors are in the main panel. Except maybe if they are already familiar with the modules.

[donyunardi]

Great work, Nik!
I do have a scenario for you and want to hear your thought.

Let's take tm_t_events for example where I purposely remove all factors/levels from the data frame.

library(dplyr)
library(scda)
library(teal.modules.clinical)

adsl <- synthetic_cdisc_dataset("latest", "adsl")
# change all columns to character, wiping out factors from columns.
adsl <- as.data.frame(lapply(adsl, as.character), stringsAsFactors = F)
adae <- synthetic_cdisc_dataset("latest", "adae")

app <- teal::init(
  data = cdisc_data(
    cdisc_dataset("ADSL", adsl, code = 'ADSL <- synthetic_cdisc_dataset("latest", "adsl")'),
    cdisc_dataset("ADAE", adae, code = 'ADAE <- synthetic_cdisc_dataset("latest", "adae")')
  ),
  modules = modules(
    tm_t_events(
      label = "Adverse Event Table",
      dataname = "ADAE",
      arm_var = choices_selected(c("ARM", "ARMCD"), "ARM"),
      llt = choices_selected(
        choices = variable_choices(adae, c("AETERM", "AEDECOD")),
        selected = c("AEDECOD")
      ),
      hlt = choices_selected(
        choices = variable_choices(adae, c("AEBODSYS", "AESOC")),
        selected = "AEBODSYS"
      ),
      add_total = TRUE,
      event_type = "adverse event"
    )
  )
)
if (interactive()) {
  shinyApp(app$ui, app$server)
}

When the app is loaded, you see the message from data input validation (shiny::validate) because there's something wrong with the data, not the inputs.

However, if I trigger the input validator, I no longer see the message from message from data input validation even though the problem is there. It's replaced with the message from input validator.

This makes sense because we run the input validator first before the the data input validation in validate_checks reactive.

teal.modules.clinical/R/tm_t_events.R

Lines 697 to 699 in d80b6b8

	validate_checks <- shiny::reactive({
	teal::validate_inputs(iv_r())

I don't think we can combine both validation messages since they're using different concept, but do you think we should change the order of validation execution? I feel that it could make more sense to always run the data input validation first before we run the input validators. WDYT?

---

Also, you already brought this up before (insightsengineering/teal.widgets#102) but I thought I'd mention it again:

Perhaps we also need to push this together with teal.widgets/issues/102?

[chlebowa]

When the app is loaded, you see the message from data input validation (shiny::validate) because there's something wrong with the data, not the inputs.

However, if I trigger the input validator, I no longer see the message from message from data input validation even though the problem is there. It's replaced with the message from input validator.

I don't think we can combine both validation messages since they're using different concept, but do you think we should change the order of validation execution? I feel that it could make more sense to always run the data input validation first before we run the input validators. WDYT?

I would place data validations should be done before input validations since "not a factor" prevents one from using the app altogether. Btw, I think this should be covered by assertions and not by validate because it's not in the user's power to change it, rather the developer's responsibility to ensure it, see here

[nikolas-burkoff]

Btw, I think this should be covered by assertions and not by validate because it's not in the user's power to change it, rather the developer's responsibility to ensure it, see insightsengineering/teal.osprey#198

In principle yes definitely but because we don't have the data (or at a least the schema which could be validated in teal when the data is loaded before coming into a module) until we have the app loading in DDL case we can't do this.. :(

[nikolas-burkoff]

@asbates

In the tm_g_ci example there's a couple of filter_specs (select lab, screening). I think these should have validation as well b/c otherwise the plot is empty. You mentioned in insightsengineering/teal.transform#113 that filter_spec validation updates weren't needed for tmc. But I think filter_spec should have the same validation for consistency.

Yup so I can see this is going to be a problem occasionally - usually for filtering for paramcd in a dataset - so we may need to update teal.transform to handle this somehow...