BIMSBbioinfo / genomation

R package for genomic feature analysis and visualization
http://bioinformatics.mdc-berlin.de/genomation/
73 stars 22 forks source link

Allow `[` on ScoreMatrixList by name #182

Open balwierz opened 5 years ago

balwierz commented 5 years ago

The hack introduced in https://github.com/BIMSBbioinfo/genomation/issues/141 breaks the possibility of subsetting elements of ScoreMatrixList by names.

> foo = ScoreMatrixList(list(first=as(matrix(1), "ScoreMatrix"),
                           second=as(matrix(2,nrow=2,ncol=2), "ScoreMatrix")))
> foo["second"]
Error in ScoreMatrixList(tmp) : windows of class GRanges must be defined

If I undo that commit i.e. remove names slot from ScoreMatrixList class and delete the following function:

setMethod("[",signature(x="ScoreMatrixList", i = "ANY"), 
          function(x,i){
            tmp=sml@.Data[i]
            names(tmp)=names(x)[i]
            ScoreMatrixList( tmp)
          }
)

from Ops.R I unbreak the correct behaviour.

> foo = ScoreMatrixList(list(first=as(matrix(1), "ScoreMatrix"),
                           second=as(matrix(2,nrow=2,ncol=2), "ScoreMatrix")))
> foo["second"]
$second
  scoreMatrix with dims: 2 2

Not only this. I don't see the original problem mentioned in #141 at all. Subsetting a ScoreMatrixList preserves names.

>  names(foo[2])
[1] "second"
  sessionInfo()
R version 3.5.2 (2018-12-20)
Platform: x86_64-pc-linux-gnu (64-bit)
Running under: Debian GNU/Linux buster/sid

Matrix products: default
BLAS: /usr/lib/x86_64-linux-gnu/openblas/libblas.so.3
LAPACK: /usr/lib/x86_64-linux-gnu/libopenblasp-r0.3.5.so

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

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

other attached packages:
[1] genomation_1.11.3   BiocParallel_1.16.6

loaded via a namespace (and not attached):
[1] compiler_3.5.2 Rcpp_1.0.0
katwre commented 5 years ago

I agree with @balwierz, I don't see a problem with #141 and after removing mentioned by @balwierz function (https://github.com/BIMSBbioinfo/genomation/blob/master/R/Ops.R#L112) everything works well. @al2na @frenkiboy are u ok with removing:

setMethod("[",signature(x="ScoreMatrixList", i = "ANY"), 
          function(x,i){
            tmp=sml@.Data[i]
            names(tmp)=names(x)[i]
            ScoreMatrixList( tmp)
          }
)

?

al2na commented 5 years ago

underlying issue might have been fixed with newer R versions, if it doesn't break anything downstream I'm ok with removing. It looks like it shouldn't but maybe we run check first ?

katwre commented 5 years ago

check on it looks as before, no errors, no warnings if there are no arguments to keep it, then I'll remove it (@frenkiboy, if it would cause any problems in your code, I'll recheck it and try to make it work for everyone)

balwierz commented 5 years ago

Thanks. I have also removed the "names" slot in ScoreMatrixList definition as it was introduced in the fix of #141

katwre commented 5 years ago

I just reverted the commit that supposed to fix this issue until a better solution will be found, because it fails BioC check.