Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Could path,DelayedArray-method just defer to the path() of its @seed? #120

Closed
LTLA opened this issue Nov 30, 2024 · 3 comments
Closed

Could path,DelayedArray-method just defer to the path() of its @seed? #120

LTLA opened this issue Nov 30, 2024 · 3 comments

Comments

@LTLA
Copy link
Contributor

LTLA commented Nov 30, 2024

Curently path(<DelayedArray>) just calls path(<DelayedOp>), which effectively tries to call path(seed(object)). Could a dedicated path,DelayedArray-method be added that calls path(object@seed)?

As per the conclusion of #119, my ReloadedArraySeed class now inherits from DelayedUnaryIsoOp, but its path() method is not respected when calling path(<ReloadedArray>). This is because the seed() call follows the new inheritance path for the DelayedUnaryIsoOp, retrieving the ReloadedArraySeed's seed instead of the ReloadedArraySeed itself. Subsequently, the path() call uses the seed's method instead of my custom ReloadedArraySeed method, which is not intended.

The proposed change to path(object@seed) should not affect any existing behavior. If @seed is a DelayedOp, path() will still call seed() to get the underlying seed; and if @seed is something else, directly using its path() method is arguably less surprising than whatever we might get from following seed() (if it even runs at all, e.g., error from multiple seeds).

@hpages
Copy link
Contributor

hpages commented Dec 2, 2024

Hmm.. even with the dedicated path,DelayedArray-method, my current path,DelayedOp-method method is still in the way:

library(DelayedArray)
y <- DelayedArray(rsparsematrix(1000, 100, 0.05))
y <- log1p(abs(y) / 1:100)

library(alabaster.matrix)
z <- ReloadedArray(path=tempdir(), y)

path(z)
# [1] "/tmp/RtmpXXEFKs"

path(z * 10)
# Error: unable to find an inherited method for function ‘path’ for signature ‘object = "dgCMatrix"’

Adding the dedicated path,DelayedArray-method:

setMethod(path, "DelayedArray", function(object, ...) path(object@seed, ...))

path(z * 10)
# Error: unable to find an inherited method for function ‘path’ for signature ‘object = "dgCMatrix"’

Need to implement path() in a recursive manner, like I did for seed(). Then no need for dedicated methods for DelayedArray or ReloadedArray objects. You'll only need the path,ReloadedArraySeed-method.

Working on that change now...

sessionInfo()
R Under development (unstable) (2024-10-22 r87265)
Platform: x86_64-pc-linux-gnu
Running under: Ubuntu 23.10

Matrix products: default
BLAS:   /home/hpages/R/R-4.5.r87265/lib/libRblas.so 
LAPACK: /usr/lib/x86_64-linux-gnu/lapack/liblapack.so.3.11.0

locale:
 [1] LC_CTYPE=en_US.UTF-8       LC_NUMERIC=C              
 [3] LC_TIME=en_US.UTF-8        LC_COLLATE=en_US.UTF-8    
 [5] LC_MONETARY=en_US.UTF-8    LC_MESSAGES=en_US.UTF-8   
 [7] LC_PAPER=en_US.UTF-8       LC_NAME=C                 
 [9] LC_ADDRESS=C               LC_TELEPHONE=C            
[11] LC_MEASUREMENT=en_US.UTF-8 LC_IDENTIFICATION=C       

time zone: America/Los_Angeles
tzcode source: system (glibc)

attached base packages:
[1] stats4    stats     graphics  grDevices utils     datasets  methods  
[8] base     

other attached packages:
 [1] alabaster.matrix_1.7.3 alabaster.base_1.7.2   DelayedArray_0.33.2   
 [4] SparseArray_1.7.2      S4Arrays_1.7.1         abind_1.4-8           
 [7] IRanges_2.41.1         S4Vectors_0.45.2       MatrixGenerics_1.19.0 
[10] matrixStats_1.4.1      BiocGenerics_0.53.3    generics_0.1.3        
[13] Matrix_1.7-1          

loaded via a namespace (and not attached):
 [1] zlibbioc_1.53.0         lattice_0.22-6          rhdf5filters_1.19.0    
 [4] XVector_0.47.0          Rhdf5lib_1.29.0         HDF5Array_1.35.1       
 [7] grid_4.5.0              rhdf5_2.51.0            compiler_4.5.0         
[10] tools_4.5.0             alabaster.schemas_1.7.0 Rcpp_1.0.13-1          
[13] jsonlite_1.8.9          crayon_1.5.3           

hpages added a commit that referenced this issue Dec 2, 2024
Revisit implementation of the path() getter (and setter) for DelayedOp.

The new implementation is recursive so will acknowledge path() methods
defined for custom DelayedOp derivatives like the path() getter defined
for ReloadedArraySeed objects in the alabaster.matrix package.

Should address #120
@hpages
Copy link
Contributor

hpages commented Dec 2, 2024

Done in DelayedArray 0.33.3 (see commit a782607).

With this version:

library(DelayedArray)
y <- DelayedArray(rsparsematrix(1000, 100, 0.05))
y <- log1p(abs(y) / 1:100)

library(alabaster.matrix)
z <- ReloadedArray(path=tempdir(), y)

path(z)
# [1] "/tmp/RtmpoVqLhJ"

path(z * 10)
# [1] "/tmp/RtmpoVqLhJ"

You should no longer need the path() method for ReloadedArray objects.

@hpages hpages closed this as completed Dec 2, 2024
@LTLA
Copy link
Contributor Author

LTLA commented Dec 3, 2024

Thanks, looks good on my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants