drighelli / SpatialExperiment

55 stars 20 forks source link

`dim,StoredSpatialImage-method` should use `magick::image_info(img)` if raster is not loaded #112

Closed LTLA closed 2 years ago

LTLA commented 2 years ago

This primarily stems from StoredSpatialImages having a very slow dim method (and thus show method) if the image has not already been loaded into the cache, because it currently needs to obtain a raster to query the dim.

This is most noticeable if you're trying to inspect the imgData and it contains multiple StoredSpatialImages objects, at which point all of them are inadvertently loaded into memory as part of the DataFrame's show method. This defeats the purpose of having StoredSpatialImages, which is to only load specific images into memory on an as-needed basis.

I believe it should be possible to specialize the dim,StoredSpatialImage-method to do something like:

HelenaLC commented 2 years ago

Huh, smart. Like this (see dim,StoredSpatialImage-method branch)? ...Not sure the unit test is the best (basically just testing that dim() is nearly instant on the SPI, which was not the case before actually, even with the tiny example image).

#' @export
#' @importFrom magick image_read image_info
setMethod("dim",
    "StoredSpatialImage",
    function(x) {
        src <- normalizePath(imgPath(x))
        src <- paste0("file://", src)
        img <- .get_from_cache(src, NULL)
        if (!is.null(img)) return(dim(img))
        img <- image_read(src)
        tib <- image_info(img)
        c(tib$height, tib$width)
    })

...Also noticed a bug in the imgSource methods that popped up as an error when running show() on an SPI; namely, no default value for path (set to FALSE now). But this is very curious as it's never been an issue. Maybe something got messed up with recent merges? No clue.

LTLA commented 2 years ago

That looks right. If you want to test it, I would do the following:

HelenaLC commented 2 years ago

Should be addressed by #113; thanks (!!) Aaron for noting & the very clear instructions for resolving this.