drieslab / Giotto

Spatial omics analysis toolbox
https://drieslab.github.io/Giotto_website/
Other
258 stars 98 forks source link

do not [] subset objects that are maybe not data tables #469

Closed andrewrech closed 1 year ago

andrewrech commented 1 year ago

In several places you copy an object, e.g.

  if(isTRUE(copy_obj)) featMeta[] = data.table::copy(featMeta[])

but in the code path it is possible the object is not a data table, but an S4 class. This errors.

Think you need to remove all the [] (they are superfluous)

Let me know if that is not clear

I think aside from this, these classes all work

Thank you

Andrew

jiajic commented 1 year ago

Hi @andrewrech ,

Could you provide some more details on error that you are getting? This is working for in our hands at the moment.

The empty [] ([ and [<- accessor generics) are defined for Giotto's S4 subobjects in v3.0+ as shorthand for extracting from or setting to the main data slot of an S4 subobject see code.

So when you see for example data.table::copy(featMeta[]), that is actually shorthand for data.table::copy(featMeta@metaDT).

This admittedly loses points for readability, but allows the S4s to be conveniently used in pretty much all situations where the wrapped S3 object can be used.

Best, Jiaji

$~$

We are a bit behind on writing this all up on our website, but to include some examples:

Getting S4 with copied data.table from gobject and setting values

> showGiottoFeatMetadata(gobj)
└──Spatial unit "cell"
   └──Feature type "rna"
         An object of class featMetaObj 
         Provenance: cell 
            feat_ID
         1:    Xkr4
         2:  Gm1992
         3:     Rp1
         4:   Sox17

> fm = get_feature_metadata(gobj, copy_obj = TRUE)
> class(fm)
[1] "featMetaObj"
attr(,"package")
[1] "Giotto"

> fm[][, new := 5]
> fm
An object of class featMetaObj 
Provenance: cell 
              feat_ID new
    1:           Xkr4   5
    2:         Gm1992   5
    3:            Rp1   5
    4:          Sox17   5
    5:         Mrpl15   5
   ---                   
21139:        Gm10931   5
21140:     AC125149.3   5
21141:     AC149090.1   5
21142: CAAA01118383.1   5
21143: CAAA01147332.1   5

# no change because copy_obj = TRUE and data.table was copied
> showGiottoFeatMetadata(gobj)
└──Spatial unit "cell"
   └──Feature type "rna"
         An object of class featMetaObj 
         Provenance: cell 
            feat_ID
         1:    Xkr4
         2:  Gm1992
         3:     Rp1
         4:   Sox17

Getting S4 from gobject without copying data.table and setting values

> fm = get_feature_metadata(gobj, copy_obj = FALSE)
> fm[][, newer := 10]
> fm
An object of class featMetaObj 
Provenance: cell 
              feat_ID newer
    1:           Xkr4    10
    2:         Gm1992    10
    3:            Rp1    10
    4:          Sox17    10
    5:         Mrpl15    10
   ---                     
21139:        Gm10931    10
21140:     AC125149.3    10
21141:     AC149090.1    10
21142: CAAA01118383.1    10
21143: CAAA01147332.1    10

# Modified by reference because copy_obj = FALSE and data.table was not copied
> showGiottoFeatMetadata(gobj)
└──Spatial unit "cell"
   └──Feature type "rna"
         An object of class featMetaObj 
         Provenance: cell 
            feat_ID newer
         1:    Xkr4    10
         2:  Gm1992    10
         3:     Rp1    10
         4:   Sox17    10

Example of returning []

> fm[]
              feat_ID newer
    1:           Xkr4    10
    2:         Gm1992    10
    3:            Rp1    10
    4:          Sox17    10
    5:         Mrpl15    10
   ---                     
21139:        Gm10931    10
21140:     AC125149.3    10
21141:     AC149090.1    10
21142: CAAA01118383.1    10
21143: CAAA01147332.1    10
> class(fm[])
[1] "data.table" "data.frame"
andrewrech commented 1 year ago

Hi

I see, I missed this. I think it is fine then. My Giotto objects belong to .GlobalEnv because I make frequent edits to your src here and there, and it seems like this accessor is not registered properly for my franken-objects. I was not able to reproduce this with a vanilla object. Sorry!

jiajic commented 1 year ago

No worries! My first guess for fixing that would be to set the basic classes in classes.R as exported. The access generics are defined based on those and that behavior is inherited by the S4 objects that contain those basic classes.

It would be good to fix that on Suite if it ends up fixing the issue.

Best Jiaji

andrewrech commented 1 year ago

Hi Jiaji,

Thanks. I fixed this but making sure gobject was built with all the classes belonging to Giotto. I realize now that changing the class of gobject doesn't affect anything inside, as is expected in R. Then, using anything in .GlobalEnv is not an issue.

jiajic commented 1 year ago

Okay glad to hear it!