Accept functions #1393
Accept functions #1393
Conversation
- add/modify tests - r cmd check
Unit Tests Summary 1 files 25 suites 8m 55s ⏱️ Results for commit 8630957. ♻️ This comment has been updated with latest results. |
Unit Test Performance Difference
Additional test case details
Results for commit 17da5e7 ♻️ This comment has been updated with latest results. |
- don't export get_filter_overview
|
Code Coverage SummaryDiff against mainResults for commit: 8630957 Minimum allowed coverage is ♻️ This comment has been updated with latest results |
|
this topic is very close to insightsengineering/teal.slice#621 issue. Would it make sense to include it or you prefer to do this separately? |
Nice, I guess this solves the problem, is it? |
I haven't tried it myself but based on below it seems that yes!
|
| @@ -3,15 +3,17 @@ | |||
| #' Module and its utils to display the number of rows and subjects in the filtered and unfiltered data. | |||
| #' | |||
| #' @details Handling different data classes: | |||
| #' `get_object_filter_overview()` is a pseudo S3 method which has variants for: | |||
| #' `get_filter_overview()` is a pseudo S3 method which has variants for: | |||
There was a problem hiding this comment.
In the future it can be easily changed into S3 - so that app developers/teal-developers can make own methods to display summary for other datasets. There is a vision for teal.slice that it could be extended in the future by S3 methods.
There was a problem hiding this comment.
Can we make it S3 without exporting the method?
There was a problem hiding this comment.
We can but I wouldn't do this now. This won't even change the code dramatically because we can't export <method>.data.frame or <method>.MultiAssayExperiment because it will complicate overriding in the future by the users. When trying to refactor teal.slice (you were also involved i remember), we noticed that overriding a S3 method is possible only through .GlobalEnv - if somebody creates <method>.data.frame in the package then teal will always prioritize teal method. This is why I'd avoid creating S3 for now, because the only method will be get_filter_overview.default with this "ugly-if--pseudo-dispatch"
| dataname = dataname, | ||
| subject_keys = subject_keys | ||
| ) | ||
| } | ||
| ) | ||
|
|
||
| unssuported_idx <- vapply(rows, function(x) all(is.na(x[-1])), logical(1)) # this is mainly for vectors | ||
| do.call(rbind, c(rows[!unssuported_idx], rows[unssuported_idx])) | ||
| do.call(.smart_rbind, out) |
There was a problem hiding this comment.
.smart_rbind because get_filter_overview output is not restricted. Previously every method have returned:
dataname,obsandobs_filtered: number of filtered and unfiltered rows which were formatted into single column inrenderUItoObs = <filtered>/<unfiltered>subjectsandsubjects_filtered
Now methods can return any columns but formatting occurs in the method and not in the renderUI
datanameobs: formatted counts"<filtered>/<unfiltered>"subjects
I've modified the way what function returns to allow easy extension in the future. .rbind_smart is used only to bind overviews which might be different for different object types.
- better name for toggle unsupported button
There was a problem hiding this comment.
I really like the idea and the implementation makes sense, however, 🔴 the functionality is not working on my machine as the datanames are always showing (by default) and JS link does nothing
Screencast.from.2024-10-30.08-28-12.webm
Can you confirm it works to see if I need to research locally why my laptop is acting up 😅
Note: there's a conflict with main, but I suppose it's a simple fix to include the .smart_rbind function definition
I've added a few comments and found a bug in data summary (not this functionality, but I believe we can sneak the fix here).
I have 1 suggestion about the feature:
- Group together unsupported data types on bottom
My sample app to test a bunch of unsupported types.
Example app
pkgload::load_all("teal")
options(
teal.log_level = "TRACE",
teal.show_js_log = TRUE,
# teal.bs_theme = bslib::bs_theme(version = 5),
shiny.bookmarkStore = "server"
)
init(
data = teal_data() |> within({
my_fun <- function(x) x
`%cbind%` <- iris
my_expression <- expression(1+1)
my_primitive <- c
my_env <- new.env()
my_env$a <- 1
my_env$b <- 2
my_lang <- quote(1+1)
my_expr <- expression(1+1)
my_quo <- rlang::quo(1+1)
my_numeric1 <- 2
my_order0 <- 1
my_order1 <- iris
my_order2 <- 1
iris <- iris
mae <- withr::with_package("MultiAssayExperiment", {
data("miniACC", package = "MultiAssayExperiment", envir = environment())
miniACC
})
}),
modules = modules(
example_module("Module (mae unsupported)", datanames = c("mae")),
example_module("Module (small)", datanames = c("iris", "my_order0", "my_order1", "my_order2")),
example_module("Module (all)"),
example_module(
"With Transformers",
transformers = list(
teal_transform_module(
server = function(id, data) {
reactive({
data() |> within({
t_small_iris <- head(iris)
t_character <- "transformed"
t_mae <- withr::with_package("MultiAssayExperiment", {
data("miniACC", package = "MultiAssayExperiment", envir = environment())
miniACC
})
})
})
}
)
)
)
)
) |> shiny::runApp()| @@ -3,15 +3,17 @@ | |||
| #' Module and its utils to display the number of rows and subjects in the filtered and unfiltered data. | |||
| #' | |||
| #' @details Handling different data classes: | |||
| #' `get_object_filter_overview()` is a pseudo S3 method which has variants for: | |||
| #' `get_filter_overview()` is a pseudo S3 method which has variants for: | |||
There was a problem hiding this comment.
Can we make it S3 without exporting the method?
DESCRIPTION
Outdated
There was a problem hiding this comment.
Thanks Dawid. This is very nicely implemented now and heading into being merged.
That always makes me step back and try again to look from a bigger perspective. I do understand the need of presenting the information which objects do not end up being filterable. Is this the right way to communicate this? I have some doubts.
I'm having this issue in my mind which is actually coming from our examples (most likely in teal.gallery). Have a look how big is the empty space. And space on the filter panel is really precious. You have added a show/hide for this. Is this the best solution? Me, as a user, I just want to know that a, b, c objects are not supported. For me it's fine to put that as a table footer (or text below) and enumerate names by comma. Adding this to the table unnecessarily (!) consumes a lot (!) of space.
I also have concerns about show/hide. This is a summary table and that suppose to be a simple information provided in a simple way. To me, It feels like an unnecessary feature that distracts from the main focus (that should be putted elsewhere - filtering, encoding, main panel).
I'm sorry to put my comment that late but I want you to have a second thought about this before it got merged.
(even though it's a general comment I'm putting this a code-comment to better organise the discussion)
There was a problem hiding this comment.
No worries. Makes sense to just enumerate unsupported datanames. What form are you proposing? Like this:
| Dataset | Obs |
|---|---|
| iris | 150/150 |
| mtcars | 27/27 |
Unsupported: a, b, potentially long, elo, metoo
I'm cool with reducing complexity and no interactivity with show/hide unsupported names 👍
There was a problem hiding this comment.
Yes. That was my first thought. We just need to think about HTML formatting - proper font size. Also, what to do when the string would be super wide. Maybe horizontal scrolling?
Yet another idea (probably worse) could be to enumerate in the tooltip - something like "hoover on me to see unsupported obj names)
Speaking about tooltip - maybe an additional info why on the "?" icon.
"Unsupported: . (?)" and in the tooltip on "(?)" -> "These objects are of class that is currently not supported in the Filter Panel"
There was a problem hiding this comment.
Me personally like the tooltip idea more. Same tooltip which is now for the unsupported in the summary table - warning sign, when moving cursor over extra info is shown
There was a problem hiding this comment.
Alright. Let's try with this then
There was a problem hiding this comment.
Do the names really matter that much to the user? If the list is long enough then it could take 2 or more lines
Alternatively, we we just have those in the tooltip
There was a problem hiding this comment.
Unfortunately circle without filling is in font-awesome Pro. I've added following:
There was a problem hiding this comment.
I'm open for suggestions here. I just wanted to highlight that unsupported could be too strong as these datasets just can't be filtered. They still can be transformed and used in the module as normal.
My suggestion for future is to have these counts as S3 method which can be extended quickly by us or app developer.
There was a problem hiding this comment.
I just wanted to highlight that unsupported could be too strong as these datasets just can't be filtered.
I agree. Good move
From my side:
- "dataset(s)" -> "object(s)"
- icon to the right after the text
- "+ X objs" -> "And X more objs"
- smaller font / spacing?
| name = "fas fa-circle-info", | ||
| title = paste( | ||
| sep = "", | ||
| collapse = ", ", |
There was a problem hiding this comment.
| collapse = ", ", | |
| collapse = "\n", |
I'd prefer having a line break instead of comma separation.
| ")" | ||
| ) | ||
| ), | ||
| sprintf("+%s unfilterable dataset(s)", sum(is_unsupported)) |
There was a problem hiding this comment.
| sprintf("+%s unfilterable dataset(s)", sum(is_unsupported)) | |
| sprintf("+%s unfilterable object(s)", sum(is_unsupported)) |
Since we include non-dataset objects here I liked the previous naming where we called it just object.
Closes #1352
This PR enables including any data type in the
data(teal_data) object.dataApp example