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.

Sign up for GitHub

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

feat: added formatting function for filter panel classes #28

Merged
merged 12 commits into from
May 17, 2022
Merged

feat: added formatting function for filter panel classes #28

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3020d22
feat: added a method to convert filter state to string
kpagacz May 5, 2022
e37e73c
wip
kpagacz May 5, 2022
a0ab58b
wip
kpagacz May 5, 2022
5d2986f
wip
kpagacz May 10, 2022
49ab696
wip
kpagacz May 10, 2022
0ede485
feat: added human-readable string represtations of filtering classes
kpagacz May 12, 2022
ee4f770
Apply suggestions from code review
kpagacz May 13, 2022
7e62953
review no 1
kpagacz May 13, 2022
8205487
fixes
kpagacz May 13, 2022
967a751
added timezone to posixct constructors
kpagacz May 13, 2022
02d3b9c
Apply suggestions from code review
kpagacz May 17, 2022
99999c0
Merge branch 'main' into 8_teal.reporter_filter_list_print2@main
kpagacz May 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions R/FilterState.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -354,6 +354,23 @@ FilterState <- R6::R6Class( # nolint
return(invisible(NULL))
},

#' @description
#' Returns a formatted string representing this `FilterState`.
#'
#' @param indent (`numeric(1)`) the number of spaces before after each new line character of the formatted string.
#' Default: 0
#' @return `character(1)` the formatted string
#'
format = function(indent = 0) {
checkmate::assert_number(indent, finite = TRUE)

whitespace_indent <- paste0(rep(" ", indent), collapse = "")
formatted <- c(paste0(whitespace_indent, "Filtering on: ", self$get_varname()))
selected <- paste0(format(self$get_selected(), nsmall = 3), collapse = " ")
formatted <- c(formatted, paste0(whitespace_indent, " ", "Selected: ", selected))
paste(formatted, collapse = "\n")
kpagacz marked this conversation as resolved.
Show resolved Hide resolved
},

#' @description
#' Returns reproducible condition call for current selection relevant
#' for selected variable type.
Expand All @@ -370,7 +387,7 @@ FilterState <- R6::R6Class( # nolint
#' @return (`name` or `character(1)`)
get_dataname = function(deparse = TRUE) {
if (isTRUE(deparse)) {
deparse(private$input_dataname)
deparse1(private$input_dataname)
} else {
private$input_dataname
}
Expand All @@ -397,7 +414,7 @@ FilterState <- R6::R6Class( # nolint
#' @return (`name` or `character(1)`)
get_varname = function(deparse = FALSE) {
if (isTRUE(deparse)) {
deparse(private$varname)
deparse1(private$varname)
} else {
private$varname
}
Expand Down
102 changes: 102 additions & 0 deletions R/FilterStates.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,24 @@ FilterStates <- R6::R6Class( # nolint
logger::log_trace("Instantiated { class(self)[1] }, dataname: { deparse1(private$input_dataname) }")
invisible(self)
},

#' @description
#' Returns the input dataname
#' @return (`character(1)`) the input dataname
kpagacz marked this conversation as resolved.
Show resolved Hide resolved
get_datalabel = function() {
private$datalabel
},

#' @description
#' Returns the formatted string representing this `FilterStates` object.
#'
#' @param indent (`numeric(1)`) the number of spaces before each line of the representation
#' @return `character(1)` the formatted string
#'
format = function(indent) {
stop("Pure virtual method")
},

#' @description
#' Filter call
#'
Expand Down Expand Up @@ -689,6 +707,22 @@ DFFilterStates <- R6::R6Class( # nolint
)
},

#' @description
#' Returns the formatted string representing this `FilterStates` object.
#'
#' @param indent (`numeric(1)`) the number of spaces before each line of the representation
#' @return `character(1)` the formatted string
#' @examples
#'
format = function(indent = 0) {
formatted_states <- c()

for (state in self$queue_get(1L)) {
formatted_states <- c(formatted_states, paste0(state$format(indent = indent)))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

these 5 lines can be simplified to one vapply

Suggested change
formatted_states <- c()
for (state in self$queue_get(1L)) {
formatted_states <- c(formatted_states, paste0(state$format(indent = indent)))
}
formatted_states <- vapply(self$queue_get(1L), state$format, character(1), indent = indent)
for (state in self$queue_get(1L)) {
formatted_states <- c(formatted_states, paste0(state$format(indent = indent)))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we care about lines, I can do it in 2 with the previous approach 🤷🏻

paste(formatted_states, collapse = "\n")
},

#' Get function name
#'
#' Get function name used to create filter call.
Expand Down Expand Up @@ -1082,6 +1116,24 @@ MAEFilterStates <- R6::R6Class( # nolint
return(invisible(self))
},

#' @description
#' Returns the formatted string representing this `MAEFilterStates` object.
#'
#' @param indent (`numeric(1)`) the number of spaces before each line of the representation
#' @return `character(1)` the formatted string
#' @examples
#'
format = function(indent = 0) {
checkmate::assert_number(indent, finite = TRUE)

if (length(self$queue_get(1L)) > 0) {
whitespace_indent <- paste0(rep(" ", indent), collapse = "")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
whitespace_indent <- paste0(rep(" ", indent), collapse = "")
whitespace_indent <- format("", width = indent)

formatted_states <- c(paste0(whitespace_indent, "Subject filters:"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
formatted_states <- c(paste0(whitespace_indent, "Subject filters:"))
formatted_states <- paste0(whitespace_indent, "Subject filters:")

for (state in self$queue_get(1L)) formatted_states <- c(formatted_states, state$format(indent = indent + 2))
paste(formatted_states, collapse = "\n")
}
},
Copy link
Contributor

@gogonzo gogonzo May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's little too much for the simple task. Handling indention etc can be done in much simpler way:

cat(yaml::as.yaml(fs))

Screenshot 2022-05-10 at 14 25 11

If we need nice naming you can return the list similar to get_filter_states but with nicer names. And at the end use yaml::as.yaml to pretty-print this list.

For example output list could be

cat(yaml::as.yaml(
  list(
    `iris filters ` = list(
      `filtered by Sepal.Length` = list(`selected values` = c(5.1, 6.4), `NA included` = TRUE, `Inf included` = FALSE),
      `filtered by Species` = list(`selected values` = c("setosa", "versicolor"), `NA included` = FALSE)
    )
  )
))

Screenshot 2022-05-10 at 14 40 13

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a neat idea and I was toying with something similar as well, although...

I feel like creating compound list names is as annoying to do as making a string. If I want to create a name for these lists I need to declare a string anyway and at this point, does it really make sense to make a list of it if I can just return the string?

Also - if I want to do it nicely, then each of these classes will need to create a list (with custom list names which is annoying to do), then cast it to yaml, then I need to capture output that yaml and then return the captured string from a method? Seems like a lot of steps can be skipped there.

Alternatively, you could just return yaml, but it sort of beats the idea of returning a human-readable (so, something clear that does not need a full API reference to understand) string from this method, which was the motivation for this task. I agree it's not there yet, but as outlined - it's wip.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, I agree it's a hard choice. But when I see number of function calls, concatenating, formatting, appending vectors and paste-collapse then making a list might be worth a shot - especially that PR is still missing keep_na and keep_inf informations which are going to add more code. This code is too long for relatively simple task - there are some places where we can reduce number of calls (if we use sprintf).

Altenratively, what about yaml::as.yaml(datasets$get_filter_state(pretty = TRUE))?

I see the point of making format functions as we can fully control the output. I'm leaving the decision between list-to-yaml and recursive-format to you. I'm happy also with the class$format but with simpler formatting code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to avoid sprintf because its support for NULL and zero length arrays is awful and using paste makes it so much easier. Replacing paste with sprintf won't really make a difference in terms of complexity, it will just move it to dealing with zero-length arrays and NULL values which is more annoying for me. As it is now, the string is built in a somewhat logical manner which is a plus for me.

I am actually debating adding NA or Inf. I actually think the Inf part is inconsequential and keep NA needs to be transformed somehow to be more meaningful.

This looks like an easy task but it's annoying to do if we care about human readability and the latter is the crux of the issue.


#' Get function name
#'
#' Get function name used to create filter call.
Expand Down Expand Up @@ -1435,6 +1487,38 @@ SEFilterStates <- R6::R6Class( # nolint
)
},

#' @description
#' Returns the formatted string representing this `MAEFilterStates` object.
#'
#' @param indent (`numeric(1)`) the number of spaces before each line of the representation
#' @return `character(1)` the formatted string
#' @examples
#'
format = function(indent = 0) {
checkmate::assert_number(indent, finite = TRUE)

whitespace_indent <- paste0(rep(" ", indent), collapse = "")
formatted_states <- c()
if (!is.null(self$queue_get(queue_index = "subset"))) {
formatted_states <- c(formatted_states, paste0(whitespace_indent, " Subsetting:"))
for (state in self$queue_get(queue_index = "subset")) {
formatted_states <- c(formatted_states, state$format(indent = indent + 4))
}
}

if (!is.null(self$queue_get(queue_index = "select"))) {
formatted_states <- c(formatted_states, paste0(whitespace_indent, "Selecting:"))
for (state in self$queue_get(queue_index = "select")) {
formatted_states <- c(formatted_states, state$format(indent = indent + 4))
}
}

if (length(formatted_states) > 0) {
formatted_states <- c(paste0(whitespace_indent, "Assay ", self$get_datalabel(), " filters:"), formatted_states)
paste(formatted_states, collapse = "\n")
}
},

#' @description
#' Server module
#' @param id (`character(1)`)\cr
Expand Down Expand Up @@ -1970,6 +2054,24 @@ MatrixFilterStates <- R6::R6Class( # nolint
)
},

#' @description
#' Returns the formatted string representing this `MatrixFilterStates` object.
#'
#' @param indent (`numeric(1)`) the number of spaces before each line of the representation
#' @return `character(1)` the formatted string
#' @examples
#'
format = function(indent = 0) {
checkmate::assert_number(indent, finite = TRUE)

formatted_states <- c()
whitespace_indent <- paste0(rep(" ", indent), collapse = "")
kpagacz marked this conversation as resolved.
Show resolved Hide resolved
for (state in self$queue_get(queue_index = "subset")) {
formatted_states <- c(formatted_states, state$format(indent = indent + 2))
}
paste(formatted_states, collapse = "\n")
},

#' @description
#' Server module
#' @param id (`character(1)`)\cr
Expand Down
36 changes: 34 additions & 2 deletions R/FilteredData.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ FilteredData <- R6::R6Class( # nolint
names(private$filtered_datasets)
},


#' Gets data label for the dataset
#'
#' Useful to display in `Show R Code`.
Expand Down Expand Up @@ -315,7 +314,6 @@ FilteredData <- R6::R6Class( # nolint
intersect(self$datanames(), datanames)
},


#' @description
#' Adds a `TealDataset` object to this `FilteredData`
#'
Expand Down Expand Up @@ -377,6 +375,40 @@ FilteredData <- R6::R6Class( # nolint
Filter(function(x) length(x) > 0, states)
},

#' @description
#' Returns the filter state formatted for printing to an `IO` device.
#'
#' @return `character` the pre-formatted filter state
#' @examples
#' datasets <- teal.slice:::FilteredData$new()
#' datasets$set_dataset(teal.data::dataset("iris", iris))
#' utils::data(miniACC, package = "MultiAssayExperiment")
#' datasets$set_dataset(teal.data::dataset("mae", miniACC))
#' fs <- list(
#' iris = list(
#' Sepal.Length = list(selected = c(5.1, 6.4), keep_na = TRUE, keep_inf = FALSE),
#' Species = list(selected = c("setosa", "versicolor"), keep_na = FALSE)
#' ),
#' mae = list(
#' subjects = list(
#' years_to_birth = list(selected = c(30, 50), keep_na = TRUE, keep_inf = FALSE),
#' vital_status = list(selected = "1", keep_na = FALSE),
#' gender = list(selected = "female", keep_na = TRUE)
#' ),
#' RPPAArray = list(
#' subset = list(ARRAY_TYPE = list(selected = "", keep_na = TRUE))
#' )
#' )
#' )
#' datasets$set_filter_state(state = fs)
#' cat(shiny::isolate(datasets$get_formatted_filter_state()))
#'
get_formatted_filter_state = function() {
out <- c()
for (filtered_dataset in self$get_filtered_dataset()) out <- c(out, filtered_dataset$get_formatted_filter_state())
Copy link
Contributor

@nikolas-burkoff nikolas-burkoff May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

wouldn't

out <- vapply(self$get_filtered_dataset(), 
  function(x) x$get_formatted_filter_state(), FUN.VALUE = character(1)
)
paste(out, collapse = "\n")

work as well? And in other places? Or is it a deliberate decision to use for loops

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They don't return a character(1) always.

Copy link
Contributor Author

@kpagacz kpagacz May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Plus, I usually append things to the out object (not always) before looping and apply stuff doesn't capture by reference from their lexical scopes, so I opted to use for loops everywhere. Plus, for loops in R stopped being less performant than apply stuff ages ago, so tbh there's no particularly good reason to use them except for 'list or dict comprehension'-style programming.

Copy link
Contributor

Choose a reason for hiding this comment

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

fair enough just askin' ;)

paste(out, collapse = "\n")
},

#' @description
#' Sets active filter states.
#' @param state (`named list`)\cr
Expand Down
14 changes: 13 additions & 1 deletion R/FilteredDataset.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,18 @@ FilteredDataset <- R6::R6Class( # nolint
invisible(self)
},


#' @description
#' Returns a string representation of the filter state in this `FilteredDataset`.
#'
#' @return `character(1)` the formatted string representing the filter state
#'
get_formatted_filter_state = function() {
out <- c(paste0("Filters for dataset: ", self$get_dataname()))
for (states in self$get_filter_states()) out <- c(out, states$format(indent = 2))
paste(out, collapse = "\n")
},

#' @description
#' Adds objects to the filter call evaluation environment
#' @param name (`character`) object name
Expand Down Expand Up @@ -554,7 +566,6 @@ DefaultFilteredDataset <- R6::R6Class( # nolint
#' @param dataset (`TealDataset`)\cr
#' single dataset for which filters are rendered
initialize = function(dataset) {
stopifnot(is(dataset, "TealDataset"))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The super constructor already checks it.

super$initialize(dataset)
dataname <- dataset$get_dataname()

Expand Down Expand Up @@ -591,6 +602,7 @@ DefaultFilteredDataset <- R6::R6Class( # nolint
)
},

#' @description
#' Gets the reactive values from the active `FilterState` objects.
#'
#' Get all active filters from this dataset in form of the nested list.
Expand Down
1 change: 1 addition & 0 deletions man/CDISCFilteredData.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions man/CDISCFilteredDataset.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions man/ChoicesFilterState.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 42 additions & 2 deletions man/DFFilterStates.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions man/DateFilterState.Rd
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading