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

Adds decorate functionality to module output #1357

Open
wants to merge 183 commits into
base: main
Choose a base branch
from

Conversation

gogonzo
Copy link
Contributor

@gogonzo gogonzo commented Sep 30, 2024

closes #1383 #1384

Companion PRs:

example tmg app
pkgload::load_all("teal")
pkgload::load_all("teal.modules.general")
library(teal.widgets)

data <- teal_data()
data <- within(data, {
  require(nestcolor)
  ADSL <- rADSL
})
join_keys(data) <- default_cdisc_join_keys[c("ADSL")]

footnote_regression <- teal_transform_module(
  server = make_teal_transform_server(expression(
    plot <- plot + labs(caption = deparse(summary(fit)[[1]]))
  ))
)

fact_vars_adsl <- names(Filter(isTRUE, sapply(data[["ADSL"]], is.factor)))
vars <- choices_selected(variable_choices(data[["ADSL"]], fact_vars_adsl))

app <- init(
  data = data,
  modules = modules(
    tm_a_regression(
      label = "Regression",
      response = data_extract_spec(
        dataname = "ADSL",
        select = select_spec(
          label = "Select variable:",
          choices = "BMRKR1",
          selected = "BMRKR1",
          multiple = FALSE,
          fixed = TRUE
        )
      ),
      regressor = data_extract_spec(
        dataname = "ADSL",
        select = select_spec(
          label = "Select variables:",
          choices = variable_choices(data[["ADSL"]], c("AGE", "SEX", "RACE")),
          selected = "AGE",
          multiple = TRUE,
          fixed = FALSE
        )
      ),
      ggplot2_args = ggplot2_args(
        labs = list(subtitle = "Plot generated by Regression Module")
      ),
      decorators = list(footnote_regression)
    )
  )
)

shinyApp(app$ui, app$server)

@gogonzo gogonzo marked this pull request as draft September 30, 2024 08:22
R/modules.R Outdated Show resolved Hide resolved
R/module_nested_tabs.R Outdated Show resolved Hide resolved
R/modules.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/teal_transform_module.R Outdated Show resolved Hide resolved
R/dummy_functions.R Outdated Show resolved Hide resolved
#' @rdname module_transform_data
ui_transform_data <- function(id, transformers = list(), class = "well") {
ui_teal_transform_data <- function(id, transformators, class = "well") {
Copy link
Contributor

@pawelru pawelru Nov 21, 2024

Choose a reason for hiding this comment

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

how about ui/srv_transform_teal_data

This is because of:

Module to transform reactive teal_data

Copy link
Contributor

@averissimo averissimo Nov 22, 2024

Choose a reason for hiding this comment

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

I agree and the team as well, on it

Although this requires a bunch of changes, so no single commit to show. I'll make the change in teal and then apply in tmg

R/module_transform_data.R Outdated Show resolved Hide resolved
R/module_transform_data.R Outdated Show resolved Hide resolved
R/module_nested_tabs.R Outdated Show resolved Hide resolved
#' @name teal_transform_module
#'
#' @export
teal_transform_module <- function(ui = NULL,
Copy link
Contributor

Choose a reason for hiding this comment

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

what is your opinion about putting package name in the function name? Can it be just transform_module()?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little bit confused what the convention is - also looking into other functions

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it can, but we may want to do this in a different issue and apply the same convention package-wide.

If my memory serves me, weeks/months ago we were all discussing with you @pawelru that module and modules should have the teal_ prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's exactly the part that I'm currently struggle with :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Naming convention could be improved, we can discuss it on another issue for sure

Comment on lines +182 to +183
Decorator failures are managed by an internal `teal` mechanism called **trigger on success**, which ensures that the `data`
object within the module remains intact. If a decorator fails, it will be ignored, and an appropriate error message will be displayed.
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it too technical?

Copy link
Contributor

Choose a reason for hiding this comment

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

The decorator is a technical feature of the framework. We need to go into some details.

I'm not too sure about what it means to "remain intact". @m7pr can you clarify as you were the last one to change this line 😅

Suggested change
Decorator failures are managed by an internal `teal` mechanism called **trigger on success**, which ensures that the `data`
object within the module remains intact. If a decorator fails, it will be ignored, and an appropriate error message will be displayed.
Decorator failures are managed by an internal `teal` mechanism called **trigger on success**, which ensures that the `data`
object within the module remains intact.
If a decorator fails, the outputs will not be shown, and an appropriate error message will be displayed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Like the object will not be changed inside module/teal_data id the decorator produces an error : P

Copy link
Contributor

@llrs-roche llrs-roche left a comment

Choose a reason for hiding this comment

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

Just a small suggestion for the vignette as I was reading & running it locally to understand this feature better.


Preserving the object's class during decoration is essential for compatibility. It ensures that the subsequent "display" logic can seamlessly handle both decorated and non-decorated objects.

The decoration process can vary in complexity:
Copy link
Contributor

Choose a reason for hiding this comment

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

This list doesn't show up nicely on the browser on my computer:

screenshot of this paragraph

I think it needs a new line before it:

Suggested change
The decoration process can vary in complexity:
The decoration process can vary in complexity:

Copy link
Contributor

@pawelru pawelru Nov 22, 2024

Choose a reason for hiding this comment

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

Good catch!

My assumption was that this is automatically detected - it seems that it's not

markdownlint results
❯ markdownlint vignettes/decorate-module-output.Rmd
vignettes/decorate-module-output.Rmd:15:98 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:15:81 MD013/line-length Line length [Expected: 80; Actual: 98]
vignettes/decorate-module-output.Rmd:16:103 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:16:81 MD013/line-length Line length [Expected: 80; Actual: 103]
vignettes/decorate-module-output.Rmd:17:208 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:17:81 MD013/line-length Line length [Expected: 80; Actual: 208]
vignettes/decorate-module-output.Rmd:18:81 MD013/line-length Line length [Expected: 80; Actual: 108]
vignettes/decorate-module-output.Rmd:22:81 MD013/line-length Line length [Expected: 80; Actual: 587]
vignettes/decorate-module-output.Rmd:24:81 MD013/line-length Line length [Expected: 80; Actual: 597]
vignettes/decorate-module-output.Rmd:26:81 MD013/line-length Line length [Expected: 80; Actual: 190]
vignettes/decorate-module-output.Rmd:29:81 MD013/line-length Line length [Expected: 80; Actual: 120]
vignettes/decorate-module-output.Rmd:29 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- **Simple Decorations**: Sing..."]
vignettes/decorate-module-output.Rmd:30:81 MD013/line-length Line length [Expected: 80; Actual: 161]
vignettes/decorate-module-output.Rmd:32:81 MD013/line-length Line length [Expected: 80; Actual: 290]
vignettes/decorate-module-output.Rmd:38:81 MD013/line-length Line length [Expected: 80; Actual: 222]
vignettes/decorate-module-output.Rmd:39:81 MD013/line-length Line length [Expected: 80; Actual: 272]
vignettes/decorate-module-output.Rmd:41:81 MD013/line-length Line length [Expected: 80; Actual: 136]
vignettes/decorate-module-output.Rmd:45:109 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:45:81 MD013/line-length Line length [Expected: 80; Actual: 109]
vignettes/decorate-module-output.Rmd:48:81 MD013/line-length Line length [Expected: 80; Actual: 116]
vignettes/decorate-module-output.Rmd:49:110 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:49:81 MD013/line-length Line length [Expected: 80; Actual: 110]
vignettes/decorate-module-output.Rmd:50:213 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:50:81 MD013/line-length Line length [Expected: 80; Actual: 213]
vignettes/decorate-module-output.Rmd:51:81 MD013/line-length Line length [Expected: 80; Actual: 98]
vignettes/decorate-module-output.Rmd:55:81 MD013/line-length Line length [Expected: 80; Actual: 227]
vignettes/decorate-module-output.Rmd:56:74 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:57:106 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:57:81 MD013/line-length Line length [Expected: 80; Actual: 106]
vignettes/decorate-module-output.Rmd:78:62 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:79:73 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:80:81 MD013/line-length Line length [Expected: 80; Actual: 202]
vignettes/decorate-module-output.Rmd:96:144 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:96:81 MD013/line-length Line length [Expected: 80; Actual: 144]
vignettes/decorate-module-output.Rmd:97:126 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:97:81 MD013/line-length Line length [Expected: 80; Actual: 126]
vignettes/decorate-module-output.Rmd:98:81 MD013/line-length Line length [Expected: 80; Actual: 180]
vignettes/decorate-module-output.Rmd:126:209 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:126:81 MD013/line-length Line length [Expected: 80; Actual: 209]
vignettes/decorate-module-output.Rmd:127:81 MD013/line-length Line length [Expected: 80; Actual: 193]
vignettes/decorate-module-output.Rmd:149:80 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:150:81 MD013/line-length Line length [Expected: 80; Actual: 123]
vignettes/decorate-module-output.Rmd:151:81 MD013/line-length Line length [Expected: 80; Actual: 133]
vignettes/decorate-module-output.Rmd:152:81 MD013/line-length Line length [Expected: 80; Actual: 119]
vignettes/decorate-module-output.Rmd:182:81 MD013/line-length Line length [Expected: 80; Actual: 123]
vignettes/decorate-module-output.Rmd:183:81 MD013/line-length Line length [Expected: 80; Actual: 134]
vignettes/decorate-module-output.Rmd:205:120 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:205:81 MD013/line-length Line length [Expected: 80; Actual: 120]
vignettes/decorate-module-output.Rmd:206:80 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:210 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
vignettes/decorate-module-output.Rmd:212:81 MD013/line-length Line length [Expected: 80; Actual: 93]
vignettes/decorate-module-output.Rmd:312 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Example Module"]
vignettes/decorate-module-output.Rmd:314:81 MD013/line-length Line length [Expected: 80; Actual: 308]
vignettes/decorate-module-output.Rmd:406:81 MD013/line-length Line length [Expected: 80; Actual: 87]
vignettes/decorate-module-output.Rmd:421 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
vignettes/decorate-module-output.Rmd:493 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Application"]
markdownlint results without MD13 (line lenght) rule
❯ markdownlint vignettes/decorate-module-output.Rmd --disable MD013
vignettes/decorate-module-output.Rmd:15:98 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:16:103 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:17:208 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:29 MD032/blanks-around-lists Lists should be surrounded by blank lines [Context: "- **Simple Decorations**: Sing..."]
vignettes/decorate-module-output.Rmd:45:109 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:49:110 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:50:213 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:56:74 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:57:106 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:78:62 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:79:73 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:96:144 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:97:126 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:126:209 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:149:80 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:205:120 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:206:80 MD009/no-trailing-spaces Trailing spaces [Expected: 0 or 2; Actual: 1]
vignettes/decorate-module-output.Rmd:210 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
vignettes/decorate-module-output.Rmd:312 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Example Module"]
vignettes/decorate-module-output.Rmd:421 MD012/no-multiple-blanks Multiple consecutive blank lines [Expected: 1; Actual: 2]
vignettes/decorate-module-output.Rmd:493 MD024/no-duplicate-heading Multiple headings with the same content [Context: "Application"]

Your observation is there - among tons of other things :)
markdownlint is designed for .md files. I believe that .Rmd should have a similar structure (if not identical) - worst case we would have to disable some of the rules. I'm thinking that we should configure our superlinter CI workflow to process .Rmd with markdownlint. Let me create an issue for this. For the time being - I would recommend to test it manually.

Copy link
Contributor

@pawelru pawelru Nov 22, 2024

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, the heading checks might help to structure better the vignettes or simplify the sections.
I think you meant to link to https://github.com/insightsengineering/idr-tasks/issues/823

tm_decorated_plot("no-ui", decorators = list(static_decorator)),
tm_decorated_plot("lang", decorators = list(static_decorator_lang)),
tm_decorated_plot("interactive", decorators = list(interactive_decorator)),
tm_decorated_plot("interactive-from lang", decorators = list(interactive_decorator_lang)),
Copy link
Contributor

Choose a reason for hiding this comment

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

The interactive-from lang example doesn't change the axis labels (on current branch status):

decorator_interactive_from_lang.mp4

The interactive_decorator_lang decorator uses make_teal_transform_server which is also used by static_decorator ( used in no-ui tab example) but on that example there is no possible change on the axis by the app user.

Other examples showing how to use decorators to change the axis interactively via function or with an interactive decorator work though.

averissimo and others added 2 commits November 22, 2024 13:43
Co-authored-by: Marcin <[email protected]>
Fixes comments on vignette example on #1357
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.

[Feature Request]: Finalize decorate POC
7 participants