Enhancement: setup system for TLG (plots)

[m-kolomanski]

Issue

Closes #53

Description

• Introduces new module and specification system allowing for converting simple functions that generate plots into dynamic plot display that allows the user to customize plot attributes.
• tlg_plot module responsible for displaying plots and creating widgets for plot option customizations, which are then dynamically passed to rendering functions.
• tlg.yaml file which allows specification of TLG metadata (descriptions, links etc) for compact table display, as well as rendering function and TLG options which are then displayed as shiny widgets and editable.
• Introduces pkcg01 plot as an example of system implementation.
• Documentation of the system is provided in .github/contributing` folder.

Definition of Done

• System for defining TLGs (plots):
  • Allowing the user for quick definition of new plots
  • Easy implementation of variables editable by the user
  • In editable widgets, allow for referencing columns and labels using special key characters ($COLNAME, !LABEL)
  • Plots rendered using plotly for interactivity
  • Documentation available
• First template plot:
  • pkcg01 function implemented and adjusted to fit definition system
  • plots with linear and log scales implemented

How to test

For basic testing, run the app and Submit mapping in the Mapping and filters tab. Then you can go to TLG tab and Submit order two default plots. When the plots are rendered, you can test changing the values in the widgets.\

For more advanced testing, please do try to add a new plot. The details are described in .github/contributing/add-tlg.md. Please check if the documentation is clear and complete. Adding a new plot will require writing a definition in the tlg.yaml file and providing a generating function, which could render a very basic plot.

Contributor checklist

• Code passes lintr checks
• Code passes all unit tests
• New logic covered by unit tests
• New logic is documented

Notes to reviewer

Please do review not only the code and functionalities, but also the documentation closely. If anything is confusing, missing or could be explained better - please do let me know. We want the system to be usable by contributors without the need of an introduction on a call.

[Gero1999]

Hello @m-kolomanski the idea seems amazing, pretty much what we talked and the yaml provides more flexibility. Only thing I am wondering is that the yaml file should also specify for each option if the input should be text, numeric, choice... And in case it is for example a selectInput multiple-choice we will need to connect it to the dataset and specify multiple=TRUE. Are you thinking in:

1. Adding explicitely in the yaml for each input in opts the type of input
2. Guessing the type of input is needed by some conventional naming

On the other hand, I am not sure if you should/could already have in mind how the app will make the UI distribution of the inputs. We would like that all inputs regarding for example labs (title, xlab, ylab) are located together at a different section to other type of inputs. Although I guess that this will make more sense to predefine it in the app instead of the yaml, as it should be static

[Gero1999]

Hello @m-kolomanski , I see that this is starting to look really cool! I will also ask for feedback from Valentin on this, but I will give you some points regarding my perspective that I noticed:

• I love that now we have subtabs instead of radio buttons for selecting the plots. I think it is definitely more aesthetic! Another aesthetic detail could be that when you submit the order details you directly are driven to a populated tab (Tables, Listings or Graphs).

• The general idea seems to be fitted well! Although I would think about the possibility of displaying more than 1 plot more like a scroll down of plots so the user can check multiple at the same time.

• Subtitle seems to be the same in all plots (with same Subject ID, which should change in each by default)

• Page navigation control problem. When pressed fast more than two times "Next page" will start acting on its own

• Labels are displaying literal column names instead of their values. I would also think if there are possibilities that when changing the labels one could not only introduce literal text but also column values like $AFRLT so that is not lost, if that makes sense! I would personally put it as default populated like that so everything keeps in "one place", but of course this is just a developer/personal idea. We can discuss if this could also be a separated issue instead so the task is not overloaded!

Let us know if you agree with everything stated and with the feature developed so far @Shaakon35

[Shaakon35]

Hello @m-kolomanski @Gero1999
Thanks for the progress on this. Looks great, I will just add a few points and confirm the one of Gerardo:

• Bug when you click more than 1 time on the Next page button
• Footnote is not working. We would have a check box next to the footnote: Apply to all.
  -> This way you can add a footnote for all plots, or only one plot (one subject). Therefore, we would also need a button Save to save the few things that we changed. (so that when we export all the TLGs in pdf, all the things changed are kept)
• By default, the selectInput on the right for xlab and ylab should be populated.
• Indeed, as Gerardo said, the xlab and ylab should be replaced by the labels of the column AVAL or AFRLT, and in parenthesis ( the unique value of RRLTU or AVALU - it needs to be unique otherwise it does not work)
• Can we make the left sidebar smaller? Also, can we make the plot ggplotly?
• The option to sho multiple plots on one page would be needed.

I think we are close to what we need now already, thanks a lot for all the work!

Best
Valentin

[m-kolomanski]

@Gero1999 @Shaakon35
Thank you for the feedback, the issues you have mentioned will be addressed (or were fixed today).

I have one question regarding the number of plots per page, do we want to keep the pagination at all? Or do we just want to display all plots on single page and scroll through them? Do we want similar pagination abilities to the slope selector? What would be the best solution?

[Gero1999]

Good point. I am actually thinking that maybe all plots at once would be better.

An alternative idea could be to have some selection controls like in slope plots so the user can make edition on the set of plots they choose by selectInputs or similar (i.e, change the footnote of all plots with the X analyte). But maybe the idea is too crazy, let me know what you both think!

@m-kolomanski @Shaakon35

[Gotfrid]

Overall idea with YAML configuration is a great direction in my opinion, @m-kolomanski. Good work on that. I left a few comments here and there.

However, I think that the current state of the app is crashing - see the recording.

CleanShot.2024-11-29.at.09.56.41.mp4

[Shaakon35]

@m-kolomanski

Regarding the pagination, my idea is first to wrap up this first graph. I don't think we need any pagination in the TLG tab, we can just click next, and that should be enough.

Then, we will think about pagination in a PDF for the plots:
#54

The idea is to have 1 plot per page on the pdf.

I have seen that you created two new buttons for X-axis and Y-axis, I think we should not let the user configure the plot too much as it will complicate the bug fixing of errors. I think we should make it simple for now.

[m-kolomanski]

@Gero1999 @Shaakon35
My apologies that implementing this feature is dragging on so long.

I aggree with Valentin, wrapping this up should be the priority. I am gonna make some finishing touches to implement a working, smooth basic system, and then we can expand on it as we add new plots and the need arises. I will let you know when I am satisfied and call for a review - mostly to know if the amount of capabilities is acceptable.

Could you please provide me with comprehensive list of what arguments should be customizable for the first plot, along with what type of input it should be and what kind of choices ideally the user should be able to make?

[Shaakon35]

Hello @m-kolomanski ,
Very good work on the TLGs, thanks a lot!!

I just noticed one bug for the log10 scale:

• the plot is cut in the lower part
• there are some numbers between 10 ^0 and 10 ^1

We used to have the following code.
This code was created to have nice ticks only at 10¨x, and it also makes sure that when there are only y values very close to each other (example AVAL = 10 to 15) it does show the scale from 10^1 to 10^2

log10_scale <- function(p, aval = "AVAL") {
  min_val <- min(log10(p$data[[aval]]),na.rm=TRUE)
  max_val <- max(log10(p$data[[aval]]),na.rm=TRUE)
  intervals = log10(c(10^-10, 10^-9, 10^-8, 10^-7, 10^-6, 10^-5, 10^-4, 10^-3, 10^-2, 10^-1,
                      10^0,10^1, 10^2, 10^3, 10^4, 10^5, 10^6, 10^7, 10^8, 10^9, 10^10) )
  
  # Define the intervals
  interval_bounds <- cbind(intervals[-length(intervals)], intervals[-1])
  
  # Find which intervals contain the min and max values
  interval_min <- interval_bounds[which(interval_bounds[, 1] <= min_val &
                                          interval_bounds[, 2] > min_val), 1]
  interval_max <- interval_bounds[which(interval_bounds[, 1] <= max_val &
                                          interval_bounds[, 2] > max_val), 1]
  
  # Check if min and max values are in the same interval
  if (interval_min == interval_max) {
    res = p + ggplot2::scale_y_log10(
      breaks = 10 ^ intervals, label = 10 ^ intervals,
      limits = 10 ^ c(interval_min, interval_bounds[which(interval_bounds[, 1] == interval_min), 2])
    ) +
      annotation_logticks(sides = "l")
  } else{
    res = p + ggplot2::scale_y_log10(breaks = 10 ^ intervals, label = 10 ^ intervals) +
      annotation_logticks(sides = "l")
  }
  
  return(res)
}

adpc1 = data.frame(AVAL = runif(10, 10, 15), ARELTM1 = runif(10, 0, 10))
p = ggplot(adpc1, aes(y=AVAL, x=ARELTM1)) + geom_point() + geom_line()
p
log10_scale(p)

# Why using a custom function? because this code does not work well:
# it does not make appear 10^0, 10^1, 10^2, etc.
p + ggplot2::scale_y_log10() + annotation_logticks(sides = "l")

  

Can we reuse part of the function? I have tried to use it on the object plotly_plot, but it's not a ggplot object and I struggled.
This codes allows to have nice ticks on the y axis :

[m-kolomanski]

Hi @Shaakon35,

1. In terms of lines cutting out, that is because the data consist of points that are equal to 0, which causes log10 to produce -Inf, so the points fall out of the range of the plot. What is the expected behavior what we wish to achieve?
2. In terms of ticks, sadly plotly completely ignores the ticks and annotations set by ggplot2 (I have tried hard to make this work previously, to no avail) and does not offer much customization options. I am afraid if we wish to keep the same level of interactivity that plotly offers, we will have to deal with the drawbacks.

[Gero1999]

I would also reconsider the need for plotly here to save speed @Shaakon35. In the end, this part is not exploratory but has only reporting intentions and once printed in the pdf-report plotly will not be reactable!

[Shaakon35]

Hi @m-kolomanski @Gero1999
I agree with your comments, I checked an the ticks don't appear anymore on the second plot when I switch to ggplotly...
There might be a workaround, but let's focus on it later as it's a display issue.

Thanks a lot, all good for me.

[m-kolomanski]

@Shaakon35
I want to confirm, in relation to comment from @Gero1999, we are committing to plotly, correct? This decision is kinda important, since both the module and the plot-generating functions will be suited to plotly and changing this in the future will be hassle, since it will involve updating all the graph functions.

I do not mind either way, both approaches have pros and cons, I just wanna confirm we are reasonably happy with the choice.

[Gotfrid]

Hi @m-kolomanski , I've looked into the busy indication. Unfortunately, shiny's busyIndicator simply doesn't work with the uiOutput, and it's not clear if it will work in the near future (I went through the discussions, and apparently only a change for tableOutput is in the making).

I'm afraid that the only way to solve this would be to use shinycssloaders - that way seems to work fine. I will push my commit, so that I can move on. If you disagree, feel free to revert it.

Also, while testing, I noticed that the plots are rendered twice, thus doubling the overall loading time. And also, if I "submit order details" second time, there are no plots visible on the screen.

[m-kolomanski]

@Gotfrid If inbuild functionalities fail, I see no issue with using shinycssloaders, it is a good package. Thank you for taking the time to look into this!

[Gotfrid]

This work is so big that it probably deserves its own package. I was able to follow the instructions and create my own plot without any problems.

I left a few comments, but I wouldn't consider any of them blocking.

There is one problem that I found: following the test instructions, i was able to see all the plots and then i started configuring them. as a user, I select x-axis to be studyid and the app crashes.

Also, I'm not sure how x and y axes limits are supposed to work. I entered some values, but the don't seem to have any effect. Probably another ggplot -> plotly problem.

[Gero1999]

Indeed @m-kolomanski great work! I can imagine this has taken a lot, but the results are very very impressive. Just to go part by part:

1. I talked with @Shaakon35 and he is keen in keeping the ggplotly functionality for the App visualization. However, we should keep in mind that at some point the App will allow the users to download a PDF document that includes all TLGs.

2. Regarding my review, I only noticed two things:

• x and y limits not working
• non-blocking suggestion: default selections for numeric inputs (i.e, x axis)

3. Regarding @Gotfrid error with STUDYID in x axis is probably because this is a discrete variable with only 1 unique value. Maybe another future solution non-blocking for this PR could be:

• non-blocking suggestion: filter choices with only 1 unique value in the column