drieslab / Giotto

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

data.table::setalloccol behavior on some methods appears to break #467

Closed andrewrech closed 1 year ago

andrewrech commented 1 year ago

@jiajic

I don't have the bandwidth to fully debug this, but I want to give you a heads up that the use of data.table::setalloccol breaks these classes for me on my fork that I brought up to upstream suite

https://github.com/andrewrech/Giotto/commit/92daedd78dee6038f57d5cb493aaef6cbd71b337

Breaks with error: cannot set on NULL dt. It is not NULL, so unclear what is going on.

RubD commented 1 year ago

@andrewrech thanks for the heads up. Any chance you could give us a small reproducible example? Is this potentially a difference in data.table version or something more complex.

jiajic commented 1 year ago

Hi @andrewrech, thanks for bringing this to our attention. I did notice this happening with objects that sometimes have NULL in the a data.table-based slot instead of a data.table. One example is a kNN spatialNetworkObj where it tries to run setalloccol() on both slot @networkDT and @networkDT_before_filter even though the latter should be NULL.

library(data.table)

# Bad
> setalloccol(NULL)
Error in setalloccol(NULL) : alloccol has been passed a NULL dt

# Okay
> setalloccol(data.table())
Null data.table (0 rows and 0 cols)
# Okay
> setalloccol(data.table(NULL))
Null data.table (0 rows and 0 cols)

I added a check to the initialize() function for spatialNetworkObj #463 that seemed to resolve this for me, but it might be a good idea to add it to all the others if you are seeing it in other objects.

jiajic commented 1 year ago

Just added the same check to the all the other objects as well (#468). Should hopefully fix this issue.

andrewrech commented 1 year ago

Hi all,

Thanks for your attention to this, apologize that I did not see these message earlier. I just confirmed that #468 fixes this issue.

Thanks