Bioconductor / HDF5Array

HDF5 backend for DelayedArray objects
https://bioconductor.org/packages/HDF5Array
9 stars 13 forks source link

Use DelayedArray:::set_dimnames() #7

Closed PeteHaitch closed 6 years ago

PeteHaitch commented 6 years ago

Avoids unnecessarily degrading a HDF5Array with NULL dimnames to a DelayedArray in the coercion.

hpages commented 6 years ago

Hi Pete,

I don't think this change would make any difference. Whether you use dimnames<- or DelayedArray:::set_dimnames to do it, setting dimnames on an HDF5Array object always degrades it to a DelayedArray instance (unless you are setting NULL dimnames).

The underlying issue here is that HDF5 realization doesn't write the dimnames to the HDF5 file so, unfortunately, an HDF5Array object can never carry dimnames. This is why the coercion method from HDF5RealizationSink to DelayedArray needs to propagate them explicitly, which has the side effect to degrade the HDF5Array object to DelayedArray (the dimnames are set on the object as a delayed operation).

Storing the dimnames in the HDF5 file has been on my TODO list for a while. @vjcitn reminded me recently about this. Will try to get to this soon.

H.

hpages commented 6 years ago

Oh I see that you just closed this. Thanks!

PeteHaitch commented 6 years ago

Closing was accidental, I meant to rebase it (instead this opened a pull request #8, woops). This is the type of thing I was trying to address:

suppressPackageStartupMessages(library(HDF5Array))

x <- realize(matrix(1:10, ncol = 2), "HDF5Array")
class(x)
#> [1] "HDF5Matrix"
#> attr(,"package")
#> [1] "HDF5Array"
# as() should be a no-op, right?
class(as(x, "DelayedArray"))
#> [1] "DelayedArray"
#> attr(,"package")
#> [1] "DelayedArray"

Created on 2018-06-20 by the reprex package (v0.2.0).

hpages commented 6 years ago

By default as() is expected to perform strict coercion (i.e. to return an object of the specified class) so as(x, "DelayedArray") behaves as expected. What's expected is that as(x, "DelayedArray", strict=FALSE) is a no-op, which it is.

PeteHaitch commented 6 years ago

Ah, that makes sense! Thanks for the explanation.