Skip to content

RS-18683: Keep span attributes and some fixes for multiple stats #81

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

Merged
chschan merged 7 commits into from
May 20, 2025
Merged

RS-18683: Keep span attributes and some fixes for multiple stats #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ac6ff2a
Keep spans after manipulations
chschan Jun 11, 2024
3de769f
Fix tests
chschan May 18, 2025
b348da3
Fix transpose of multi-stat 2d QTable
chschan May 18, 2025
cc196ed
Add another fix for multiple stats
chschan May 18, 2025
edde0da
Remove span if final output is not QTable
chschan May 18, 2025
a5d4241
Keep QTable class in 1d case as well
chschan May 20, 2025
4295e67
Bump version
chschan May 20, 2025
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
4 changes: 2 additions & 2 deletions DESCRIPTION
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
Package: flipChart
Type: Package
Title: Single function for calling charts - CChart
Version: 1.12.5
Version: 1.12.6
Author: Displayr <opensource@displayr.com>
Maintainer: Displayr <opensource@displayr.com>
Description: Wrapper for other chart functions, such that they can be access via a
Expand All @@ -25,7 +25,7 @@ Imports:
flipFormat,
flipStandardCharts (>= 1.30.2),
flipStatistics,
flipTables (>= 2.7.0),
flipTables (>= 2.8.8),
flipTime,
flipTransformations (>= 1.10.0),
flipU (>= 1.6.2),
Expand Down
45 changes: 36 additions & 9 deletions R/preparedata.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,8 @@ PrepareData <- function(chart.type,
###########################################################################


if (!inherits(data, "QTable") && !is.null(attr(data, "span")))
attr(data, "span") <- NULL
Copy link
Copy Markdown
Contributor Author

@chschan chschan May 18, 2025

Choose a reason for hiding this comment

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

Add additional check here because it is fairly easy to generate a hardship error with a span attribute if it is not subscripted properly (which usually does occur because NETs are removed by default)

if (tidy.labels)
data <- tidyLabels(data, chart.type)
if (isScatter(chart.type)) # to remove span NETS
Expand Down Expand Up @@ -553,9 +555,10 @@ PrepareData <- function(chart.type,
if (chart.type == "Table" && !is.null(attr(data, "statistic")) &&
(is.null(dim(data)) || length(dim(data)) == 1))
{
tmp <- attr(data, "statistic")
data <- as.matrix(data)
attr(data, "statistic") <- tmp
is.qtable <- inherits(data, "QTable")
data <- CopyAttributes(as.matrix(data), data)
Comment thread
jrwishart marked this conversation as resolved.
if (is.qtable)
class(data) <- c(class(data), "QTable")
}

# Modify multi-stat QTables so they are 3 dimensional arrays
Expand Down Expand Up @@ -1076,7 +1079,7 @@ processInputData <- function(x, subset, weights)
# Try to use S3 method to extract data
x <- ExtractChartData(x)
n.dim <- length(dim(x)) - isQTableWithMultStatistic(x)
if (n.dim >= 2)
if (n.dim > 2)
x <- FlattenQTable(x)

if (hasUserSuppliedRownames(x))
Expand Down Expand Up @@ -1395,12 +1398,36 @@ transformTable <- function(data,
# hide.rows.threshold and row.names.to.remove refer to rows AFTER tranposing
if (isTRUE(transpose))
{
if (length(dim(data)) > 2)
if (!is.null(attr(data, "questions")) && length(dim(data)) == 2 && is.null(attr(data, "statistic")))
{
# 1-dimensional table with multiple statistics
is.qtable <- inherits(data, "QTable")
old.span <- attr(data, "span", exact = TRUE)
new.data <- array(data, dim = c(1, nrow(data), ncol(data)), dimnames = list("", rownames(data), colnames(data)))
data <- CopyAttributes(new.data, data)
if (is.qtable)
class(data) <- c(class(data), "QTable")
Comment thread
jrwishart marked this conversation as resolved.
if (!is.null(old.span))
attr(data, "span") <- list(rows = old.span$columns, columns = old.span$rows)
Comment thread
jrwishart marked this conversation as resolved.

} else if (length(dim(data)) > 2)
{
# 2-dimensional table with multiple statistics
is.qtable <- inherits(data, "QTable")
old.span <- attr(data, "span", exact = TRUE)
new.data <- aperm(data, c(2, 1, 3))
else
new.data <- t(data)
data <- CopyAttributes(new.data, data)
attr(data, "questions") <- rev(attr(data, "questions"))
data <- CopyAttributes(new.data, data)
if (is.qtable)
class(data) <- c(class(data), "QTable")
attr(data, "questions") <- rev(attr(data, "questions"))
if (!is.null(old.span))
attr(data, "span") <- list(rows = old.span$columns, columns = old.span$rows)
} else {
# Attributes handled by verbs (for QTables)
data <- t(data)
if (!inherits(data, "QTable"))
attr(data, "questions") <- rev(attr(data, "questions"))
Comment on lines +1404 to +1429
Copy link
Copy Markdown
Contributor

@jrwishart jrwishart May 19, 2025

Choose a reason for hiding this comment

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

This operation makes me a bit nervious. In most of this code block if input is a QTable, the data is restructured however the QTable class is retained. So subscripting will still call the QTable subscript? Or is this safe to do here? I fear this will break subscripting?

Copy link
Copy Markdown
Contributor

@jrwishart jrwishart May 20, 2025

Choose a reason for hiding this comment

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

I do notice on a second viewing that data <- CopyAttributes(new.data, data) was already there. So the QTable attributes would've been copied. This might be safe to do here? I can't recall where in the workflow this operation occurs. Does this occur after the statistics testing information has been extracted? And also after the subscripting (removal of rows/columns?)

Copy link
Copy Markdown
Contributor Author

@chschan chschan May 20, 2025

Choose a reason for hiding this comment

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

Yes, I tried this in Displayr with my ad hoc R server. In all 3 cases the output is still an array. The operation is a transpose, so its actually safer than subscripting because it never drops any dimensions.

Copy link
Copy Markdown
Contributor

@jrwishart jrwishart May 20, 2025

Choose a reason for hiding this comment

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

I was more concerned with reshaping the data but keeping the attributes. The subscripting relies on attributes heavily and might do the wrong operation on the reshaped data. e.g.

new.data <- array(data, dim = c(1, nrow(data), ncol(data)), dimnames = list("", rownames(data), colnames(data)))
            data <- CopyAttributes(new.data, data)

Copy link
Copy Markdown
Contributor Author

@chschan chschan May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not entirely sure what your example is trying to show. But CopyAttributes has by default attr.to.not.copy = c("dimnames", "names", "row.names", "dim", "class", "levels")

}
}

# Checking sample sizes (if available)
Expand Down
Loading
Loading