Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions NAMESPACE
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,10 @@ importClassesFrom(S4Vectors,DFrame)
importFrom(BiocGenerics,as.data.frame)
importFrom(BiocGenerics,colnames)
importFrom(BiocGenerics,rownames)
importFrom(DelayedArray,ConstantArray)
importFrom(DelayedArray,DelayedArray)
importFrom(DelayedArray,cbind)
importFrom(DelayedArray,rbind)
Comment on lines +94 to +95
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

Importing cbind/rbind from DelayedArray into the package namespace can mask base cbind/rbind and affect other code paths in this package that call cbind() (e.g., data.frame/DFrame binding). Consider removing these importFrom() entries and instead calling DelayedArray::cbind() / DelayedArray::rbind() (or DelayedArray::bindCOLS/bindROWS) explicitly at the call sites that need DelayedArray support.

Suggested change
importFrom(DelayedArray,cbind)
importFrom(DelayedArray,rbind)

Copilot uses AI. Check for mistakes.
importFrom(DelayedArray,realize)
importFrom(Matrix,rowSums)
importFrom(Matrix,sparseVector)
Expand Down
5 changes: 2 additions & 3 deletions R/ImageArray.R
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,8 @@
#'
#' @examples
#' library(SpatialData.data)
#' dir.create(td <- tempfile())
#' pa <- SpatialData.data:::.unzip_merfish_demo(td)
#' pa <- file.path(pa, "images", "rasterized")
#' zs <- get_demo_SDdata("merfish")
#' pa <- file.path(zs, "images", "rasterized")
#' (ia <- readImage(pa))
#'
#' @importFrom S4Vectors metadata<-
Expand Down
5 changes: 2 additions & 3 deletions R/PointFrame.R
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,8 @@
#'
#' @examples
#' library(SpatialData.data)
#' dir.create(tf <- tempfile())
#' base <- SpatialData.data:::.unzip_merfish_demo(tf)
#' x <- file.path(base, "points", "single_molecule")
#' zs <- get_demo_SDdata("merfish")
#' x <- file.path(zs, "points", "single_molecule")
#' (p <- readPoint(x))
#'
#' head(as.data.frame(data(p)))
Expand Down
8 changes: 4 additions & 4 deletions R/ShapeFrame.R
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,13 @@
#'
#' @examples
#' library(SpatialData.data)
#' dir.create(tf <- tempfile())
#' base <- SpatialData.data:::.unzip_merfish_demo(tf)
#' y <- file.path(base, "shapes", "cells")
#' zs <- get_demo_SDdata("merfish")
#'
#' y <- file.path(zs, "shapes", "cells")
#' (s <- readShape(y))
#' plot(sf::st_as_sf(data(s)), cex=0.2)
#'
#' y <- file.path(base, "shapes", "anatomical")
#' y <- file.path(zs, "shapes", "anatomical")
#' (s <- readShape(y))
#' plot(sf::st_as_sf(data(s)), cex=0.2)
#'
Expand Down
3 changes: 1 addition & 2 deletions R/read.R
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,7 @@ allp = c("zarr==3.1.5", "spatialdata==0.7.0", "spatialdata_io==0.6.0",
#'
#' @examples
#' library(SpatialData.data)
#' dir.create(tf <- tempfile())
#' zs <- SpatialData.data:::.unzip_merfish_demo(tf)
#' zs <- get_demo_SDdata("merfish")
#'
#' # read complete Zarr store
#' (sd <- readSpatialData(zs, anndataR=TRUE))
Expand Down
3 changes: 1 addition & 2 deletions R/sdArray.R
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,7 @@
#'
#' @examples
#' library(SpatialData.data)
#' dir.create(tf <- tempfile())
#' zs <- SpatialData.data:::.unzip_merfish_demo(tf)
#' zs <- get_demo_SDdata("merfish")
#'
#' # helper that gets path to first element in layer 'l'
#' fn <- \(l) list.files(file.path(zs, l), full.names=TRUE)[1]
Expand Down
92 changes: 70 additions & 22 deletions R/trans.R
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,60 @@ setMethod("scale", c("sdArray", "numeric"), \(x, j, t, ...) {

# label ----

#TODO
#' @rdname trans
#' @importFrom DelayedArray cbind rbind ConstantArray
#' @importFrom methods as
#' @export
setMethod("translation", c("sdArray", "numeric"), \(x, t, ...) {
#x <- label(sd, 2); t <- c(64,0)
stopifnot(
length(t) == length(dim(x)),
t == round(t), all(is.finite(t)))
Comment on lines +65 to +66
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

stopifnot(t == round(t)) will always fail when t has length > 1 because stopifnot() expects each argument to be a single TRUE; a logical vector is not isTRUE(). This makes translation(sdArray, numeric) unusable for the intended 2D/3D cases. Use a scalar check like all(t == round(t)) (and keep the finiteness check scalar as well).

Suggested change
length(t) == length(dim(x)),
t == round(t), all(is.finite(t)))
length(t) == length(dim(x)),
all(t == round(t)),
all(is.finite(t)))

Copilot uses AI. Check for mistakes.
if (all(t == 0)) return(x)
ys <- data(x, NULL)
if (length(ys) == 1) {
ts <- list(t)
} else {
ds <- vapply(ys, ncol, integer(1))
sf <- c(1, ds[-1]/ds[-length(ds)])
ts <- lapply(cumprod(sf), `*`, t)
Comment on lines +69 to +74
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

When x is multiscale, ts is computed by multiplying t by scale factors derived from ncol(). This can yield non-integer translations due to non-integer scale ratios and/or floating-point rounding (e.g., 0.5 * t becoming 31.999999), which will later be used as array dimensions via abs(t[1/2]) and can error. Consider validating/rounding each scaled translation (and failing with a clear error if it’s not exactly integer) before using it to construct d.

Suggested change
if (length(ys) == 1) {
ts <- list(t)
} else {
ds <- vapply(ys, ncol, integer(1))
sf <- c(1, ds[-1]/ds[-length(ds)])
ts <- lapply(cumprod(sf), `*`, t)
validate_translation <- \(tt, scale) {
rr <- round(tt)
tol <- sqrt(.Machine$double.eps)
if (!all(is.finite(tt)) || any(abs(tt - rr) > tol)) {
stop(
sprintf(
"translation becomes non-integer at multiscale level %d after scaling; got %s",
scale, paste(tt, collapse=", ")),
call.=FALSE)
}
as.integer(rr)
}
if (length(ys) == 1) {
ts <- list(as.integer(round(t)))
} else {
ds <- vapply(ys, ncol, integer(1))
sf <- c(1, ds[-1]/ds[-length(ds)])
ts <- lapply(seq_along(sf), \(k) {
validate_translation(cumprod(sf)[k] * t, k)
})

Copilot uses AI. Check for mistakes.
}
x@data <- lapply(seq_along(ys), \(k) {
t <- ts[[k]]
y <- as(ys[[k]], "DelayedArray")
# TODO: no 'abind' support so that we
# permute, 'c/rbind', and back-permute;
# surely there has to be a better way?
if (length(dim(y)) == 2) {
n <- NULL
} else {
t <- t[-1]
n <- dim(x)[1]
y <- aperm(y, c(2,3,1))
}
Comment on lines +82 to +88
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

For 3D arrays you drop the first component of t (t <- t[-1]) and never use it. If a caller passes a non-zero translation for the first dimension, it will be silently ignored (and dim(y) will not match the requested translation). Either enforce t[1] == 0 when length(dim(y)) > 2, or implement translation along that dimension as well.

Copilot uses AI. Check for mistakes.
if (t[2] != 0) {
d <- c(nrow(y), abs(t[2]), n)
z <- ConstantArray(0, dim=d)
y <- if (t[2] > 0) cbind(z, y) else cbind(y, z)
}
if (t[1] != 0) {
d <- c(abs(t[1]), ncol(y), n)
z <- ConstantArray(0, dim=d)
y <- if (t[1] > 0) rbind(z, y) else rbind(y, z)
}
if (!is.null(n)) aperm(y, c(3,1,2)) else y
})
return(x)
})

# point ----

#' @rdname trans
#' @importFrom dplyr mutate
#' @export
setMethod("scale", c("PointFrame", "numeric"), \(x, t, ...) {
stopifnot(is.numeric(t), length(t) == 2, t > 0, is.finite(t))
if (all(t == 1)) return(x)
y <- NULL # R CMD check
x@data <- x@data |>
mutate(x=x*t[1]) |>
Expand All @@ -74,21 +120,24 @@ setMethod("scale", c("PointFrame", "numeric"), \(x, t, ...) {
#' @importFrom dplyr mutate select
#' @export
setMethod("rotate", c("PointFrame", "numeric"), \(x, t, ...) {
y <- .y <- .x <- NULL # R CMD check
stopifnot(is.numeric(t), length(t) == 1, is.finite(t))
if (t %% 360 == 0) return(x)
y <- a <- b <- c <- d <- NULL # R CMD check
R <- .R(t*pi/180)
x@data <- x@data |>
mutate(.x=x*R[1,1], .y=y*R[1,2]) |>
mutate(x=.x+.y) |>
mutate(.x=x*R[2,1], .y=y*R[2,2]) |>
mutate(y=.x+.y) |>
select(-.x, -.y)
mutate(a=x*R[1,1], b=y*R[1,2]) |>
mutate(c=x*R[2,1], d=y*R[2,2]) |>
Comment on lines +128 to +129
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

.R() now returns the transpose of the usual CCW rotation matrix (suitable for right-multiplying row vectors, as used in ShapeFrame rotation via xy %*% .R(...)). However, PointFrame rotation computes x' = x*R[1,1] + y*R[1,2] / y' = x*R[2,1] + y*R[2,2] (left-multiplying column vectors), which makes the rotation direction opposite to ShapeFrame and the “counter-clockwise” comment. Please make the convention consistent (e.g., transpose R in the PointFrame method or adjust .R()/call sites) so the same angle rotates points in the same direction across element types.

Suggested change
mutate(a=x*R[1,1], b=y*R[1,2]) |>
mutate(c=x*R[2,1], d=y*R[2,2]) |>
mutate(a=x*R[1,1], b=y*R[2,1]) |>
mutate(c=x*R[1,2], d=y*R[2,2]) |>

Copilot uses AI. Check for mistakes.
mutate(x=a+b, y=c+d) |>
select(-c(a,b, c,d))
return(x)
})

#' @rdname trans
#' @importFrom dplyr mutate select
#' @export
setMethod("translation", c("PointFrame", "numeric"), \(x, t, ...) {
stopifnot(is.numeric(t), length(t) == 2, is.finite(t))
if (all(t == 0)) return(x)
y <- NULL # R CMD check
x@data <- x@data |>
mutate(x=x+t[1]) |>
Expand All @@ -98,11 +147,23 @@ setMethod("translation", c("PointFrame", "numeric"), \(x, t, ...) {

# shape ----

# TODO: do this w/o realizing
#' @importFrom sf st_as_sf st_geometry st_geometry<-
.trans_s <- \(x, f) {
y <- st_as_sf(data(x))
xy <- st_coordinates(y)
xy <- data.frame(f(xy))
xy <- st_as_sf(xy, coords=names(xy))
st_geometry(y) <- st_geometry(xy)
x@data <- y
return(x)
}

#' @rdname trans
#' @importFrom sf st_as_sf st_coordinates
#' @export
setMethod("scale", c("ShapeFrame", "numeric"), \(x, t, ...) {
stopifnot(is.numeric(t), length(t) == 2, t > 0, all(is.finite(t)))
stopifnot(is.numeric(t), length(t) == 2, t > 0, is.finite(t))
.trans_s(x, \(xy) sweep(xy, 2, t, `*`))
})

Expand All @@ -125,7 +186,7 @@ setMethod("translation", c("ShapeFrame", "numeric"), \(x, t, ...) {
# utils ----

# rotation matrix to rotate points counter-clockwise through an angle 't'
.R <- function(t) matrix(c(cos(t), -sin(t), sin(t), cos(t)), 2, 2)
.R <- \(t) matrix(c(cos(t), sin(t), -sin(t), cos(t)), 2, 2)

# count occurrences of each coordinate space;
# return most frequent (in order of appearance)
Expand Down Expand Up @@ -164,16 +225,3 @@ setMethod("translation", c("ShapeFrame", "numeric"), \(x, t, ...) {
}
return(xy)
}

# transform 'ShapeFrame' by realizing,
# and updating 'sf' geometry coordinates
#' @importFrom sf st_as_sf st_geometry st_geometry<-
.trans_s <- \(x, f) {
y <- st_as_sf(data(x))
xy <- st_coordinates(y)
xy <- data.frame(f(xy))
xy <- st_as_sf(xy, coords=names(xy))
st_geometry(y) <- st_geometry(xy)
x@data <- y
return(x)
}
3 changes: 1 addition & 2 deletions man/Array-methods.Rd

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

5 changes: 2 additions & 3 deletions man/ImageArray.Rd

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

5 changes: 2 additions & 3 deletions man/PointFrame.Rd

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

8 changes: 4 additions & 4 deletions man/ShapeFrame.Rd

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

8 changes: 4 additions & 4 deletions man/SpatialData.Rd

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

3 changes: 1 addition & 2 deletions man/readSpatialData.Rd

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

3 changes: 3 additions & 0 deletions man/trans.Rd

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

Loading
Loading