Robustify utility check

[stemangiola]

No description provided.

[Copilot]

Pull Request Overview

This PR enhances the check_if_assays_are_NOT_consistently_ordered utility function by fixing a bug where unnamed assays would cause failures, and adds comprehensive test coverage with 25 test cases covering edge cases and various permutation scenarios.

Key changes:

• Fixed handling of unnamed assays by forcing default numeric names when assay names are missing
• Added logic to properly separate the assay existence check from the consistency check
• Added extensive test suite covering empty assays, missing colnames, and various ordering scenarios

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
R/utilities.R	Fixed unnamed assay handling and refactored logic to separate assay existence check from consistency validation
NAMESPACE	Added import for rlang::set_names function
tests/testthat/test-utilities.R	Added comprehensive test suite with 25 test cases for the consistency check function

Comments suppressed due to low confidence (1)

R/utilities.R:1

• Line 1104-1105: map(colnames) is called on a boolean result from the keep predicate. The keep filter should check if colnames exist, but map(colnames) should be applied to the matrices before filtering, not within the predicate.

#' Drop attribute to abject

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The if condition evaluates names() which returns a character vector. In R, only the first element of a vector is used in if statements with a warning. This should check if names are NULL or missing using is.null() instead.

Suggested change

	se |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	names()
	)
	assays(se) =
	se |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	set_names(se |> assays() |> as.list() |> length() |> seq_len())
	is.null(
	se |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	names()
	) || length(
	se |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	names()
	) == 0
	) {
	assays(se) =
	se |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	set_names(se |> assays() |> as.list() |> length() |> seq_len())
	}

[Copilot]

The assignment assays(se) = modifies the input parameter, which creates side effects. Consider creating a local copy of se at the function start or restructuring to avoid mutating the input object.

Suggested change

	)
	assays(se) =
	se |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	set_names(se |> assays() |> as.list() |> length() |> seq_len())
	lack_of_consistency =
	se |>
	) {
	se_copy <- se
	assays(se_copy) =
	se_copy |>
	assays(withDimnames = FALSE) |>
	as.list() |>
	set_names(se_copy |> assays() |> as.list() |> length() |> seq_len())
	}
	lack_of_consistency =
	(if (exists("se_copy")) se_copy else se) |>

[Copilot]

Tests 11-25 are highly repetitive with identical structure testing permutations. Consider using a parameterized test approach with a data structure containing test cases (permutations and expected results) to reduce duplication and improve maintainability.

Suggested change

	# Test 11: SE with assays, all with same colnames but different order (all different) - should return TRUE
	se_all_different <- SummarizedExperiment::SummarizedExperiment(
	assays = list(
	mat1 = matrix(seq_len(9), nrow = 3),
	mat2 = matrix(seq(10, 18), nrow = 3),
	mat3 = matrix(seq(19, 27), nrow = 3)
	)
	)
	colnames(assay(se_all_different, "mat1", withDimnames = FALSE)) <- paste0("S", seq_len(3))
	colnames(assay(se_all_different, "mat2", withDimnames = FALSE)) <- paste0("S", c(3, 1, 2))
	colnames(assay(se_all_different, "mat3", withDimnames = FALSE)) <- paste0("S", c(2, 3, 1))
	expect_true(tidySummarizedExperiment:::check_if_assays_are_NOT_consistently_ordered(se_all_different))
	# Parameterized tests for permutations of colnames (Tests 11-25)
	permutation_tests <- list(
	# Each test case: list of colnames for each assay, expected result
	list(
	colnames = list(
	mat1 = c("S1", "S2", "S3"),
	mat2 = c("S3", "S1", "S2"),
	mat3 = c("S2", "S3", "S1")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("A", "B", "C"),
	mat2 = c("B", "C", "A"),
	mat3 = c("C", "A", "B")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("X", "Y", "Z"),
	mat2 = c("X", "Y", "Z"),
	mat3 = c("X", "Y", "Z")
	),
	expected = FALSE
	),
	list(
	colnames = list(
	mat1 = c("S1", "S2", "S3"),
	mat2 = c("S2", "S3", "S1"),
	mat3 = c("S3", "S1", "S2")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("A", "B", "C"),
	mat2 = c("A", "B", "C"),
	mat3 = c("C", "B", "A")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("S1", "S2", "S3"),
	mat2 = c("S1", "S2", "S3"),
	mat3 = c("S1", "S2", "S3")
	),
	expected = FALSE
	),
	list(
	colnames = list(
	mat1 = c("S1", "S2", "S3"),
	mat2 = c("S2", "S1", "S3"),
	mat3 = c("S3", "S2", "S1")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("A", "B", "C"),
	mat2 = c("C", "A", "B"),
	mat3 = c("B", "C", "A")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("X", "Y", "Z"),
	mat2 = c("Z", "Y", "X"),
	mat3 = c("Y", "X", "Z")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("S1", "S2", "S3"),
	mat2 = c("S1", "S2", "S3"),
	mat3 = c("S3", "S2", "S1")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("A", "B", "C"),
	mat2 = c("A", "B", "C"),
	mat3 = c("A", "B", "C")
	),
	expected = FALSE
	),
	list(
	colnames = list(
	mat1 = c("S1", "S2", "S3"),
	mat2 = c("S2", "S3", "S1"),
	mat3 = c("S1", "S3", "S2")
	),
	expected = TRUE
	),
	list(
	colnames = list(
	mat1 = c("A", "B", "C"),
	mat2 = c("B", "A", "C"),
	mat3 = c("C", "B", "A")
	),
	expected = TRUE
	)
	# Add more test cases as needed
	)
	for (i in seq_along(permutation_tests)) {
	test_case <- permutation_tests[[i]]
	se <- SummarizedExperiment::SummarizedExperiment(
	assays = list(
	mat1 = matrix(seq_len(9), nrow = 3),
	mat2 = matrix(seq(10, 18), nrow = 3),
	mat3 = matrix(seq(19, 27), nrow = 3)
	)
	)
	colnames(assay(se, "mat1", withDimnames = FALSE)) <- test_case$colnames$mat1
	colnames(assay(se, "mat2", withDimnames = FALSE)) <- test_case$colnames$mat2
	colnames(assay(se, "mat3", withDimnames = FALSE)) <- test_case$colnames$mat3
	if (isTRUE(test_case$expected)) {
	expect_true(tidySummarizedExperiment:::check_if_assays_are_NOT_consistently_ordered(se))
	} else {
	expect_false(tidySummarizedExperiment:::check_if_assays_are_NOT_consistently_ordered(se))
	}
	}