per-layer validity

[HelenaLC]

No description provided.

[Copilot]

Pull request overview

This PR introduces stricter, per-resolution validity checks for ImageArray/LabelArray objects (and adds/updates tests) to ensure each stored scale has the expected dimensionality and storage type.

Changes:

• Add new S4 validity methods for ImageArray, LabelArray, and PointFrame, and extend SpatialData validity to validate LabelArray content per resolution.
• Add a new test-validity.R test file and adjust LabelArray tests to reflect 2D label data and integer storage.
• Import ZarrArray::type() in NAMESPACE to support type validation.

Reviewed changes

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

File	Description
R/validity.R	Adds/rewrites validity functions to validate per-resolution dimensions and data types; updates zattrs validation behavior.
tests/testthat/test-validity.R	New tests covering validity failures for ImageArray, LabelArray, and PointFrame.
tests/testthat/test-labelarray.R	Updates tests to use 2D label arrays and integer values consistent with new validity rules.
NAMESPACE	Imports ZarrArray::type() used by the new validators.

---

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

[Copilot]

.validatePointFrame() returns msg directly; since msg <- c() yields NULL, valid PointFrames (and the early-return empty case) can make validObject() fail because the validity method returns neither TRUE nor a character vector. Return TRUE when there are no messages (including empty PointFrames), and only return a character vector when issues are found.

Suggested change

	if (!length(object)) return(msg)
	if (!"x" %in% names(object)) msg <- c(msg, "'PointFrame' missing 'x'.")
	if (!"y" %in% names(object)) msg <- c(msg, "'PointFrame' missing 'y'.")
	return(msg)
	if (!length(object)) return(TRUE)
	if (!"x" %in% names(object)) msg <- c(msg, "'PointFrame' missing 'x'.")
	if (!"y" %in% names(object)) msg <- c(msg, "'PointFrame' missing 'y'.")
	if (length(msg)) return(msg) else return(TRUE)

[Copilot]

.validateSpatialData() still calls .validatePointFrame(x) where x is a SpatialData, but .validatePointFrame() is now registered as the PointFrame validity function and expects a PointFrame (column names x/y). This call will either skip point validation or validate the wrong object. Iterate over points(x) (and validate each PointFrame) or remove this call and rely on per-PointFrame validObject() checks elsewhere.

Suggested change

	msg <- c(msg, .validatePointFrame(x))
	for (y in points(x)) {
	ok <- .validatePointFrame(y)
	if (!isTRUE(ok)) msg <- c(msg, ok)
	}

[Copilot]

The comment says “there should be two dimensions” for ImageArray, but the validator and the rest of the test treat images as 3D (channel + 2D). Update the comment to avoid confusion.

Suggested change

	# there should be two dimensions
	# there should be three dimensions (channel + 2D spatial dimensions)

[Copilot]

This test file unconditionally calls library(SpatialData.plot), which is only in Suggests. That can make R CMD check fail in environments where suggested packages aren’t installed. Either remove this dependency (it doesn’t appear needed for these validity tests) or guard it with testthat::skip_if_not_installed("SpatialData.plot") / requireNamespace().

Suggested change

library(SpatialData.plot)

[Copilot]

select() here relies on the dplyr generic being on the search path. Since this file doesn’t attach dplyr (and SpatialData only Imports it), select() may be unavailable unless some other suggested package happens to attach it. Prefer dplyr::select(...) (or require(dplyr, quietly=TRUE) + skip if not installed) to make the test self-contained.

Suggested change

	expect_error(validObject(select(x, -x)))
	expect_error(validObject(select(x, -y)))
	expect_silent(validObject(select(x, -c(x, y))[0,]))
	expect_error(validObject(dplyr::select(x, -x)))
	expect_error(validObject(dplyr::select(x, -y)))
	expect_silent(validObject(dplyr::select(x, -c(x, y))[0,]))

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

R/validity.R:104

• .validateZattrs_multiscales() now indexes x$multiscales[[1]] directly. If multiscales is missing/NULL or not a non-empty list, [[1]] can throw and make validObject() error instead of returning a validity message. Add a guard that checks x$multiscales is a non-empty list before indexing, and only then validate the first multiscale entry.

.validateZattrs_multiscales <- \(x, msg) {
    if (is.null(ms <- x$multiscales[[1]]))
        msg <- c(msg, "missing 'multiscales'")
    # MUST contain
    for (. in c("axes", "datasets"))
        if (is.null(ms[[.]]))
            msg <- c(msg, sprintf("missing 'multiscales$%s'", .))
    return(msg)

---

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

[Copilot]

In .validateZattrs_coordTrans(), when coordinateTransformations is missing or not a list you append an error message but still iterate over ct and index ct[[i]][[j]], which can throw for non-list/atomic values. Return early (or wrap the loop in an else) when coordinateTransformations is not a valid list.

[Copilot]

.validateZattrsLabelArray() assigns ms <- za$multiscales[[1]] without confirming za$multiscales exists and has at least one element. For malformed or empty .zattrs, this can throw and break validObject(); guard for NULL/non-list/empty before indexing and skip downstream checks when missing.