MarioniLab / miloDE

Framework for sensitive DE testing (using neighbourhoods)
Other
57 stars 2 forks source link

Remove buildGraph step from assign_nhoods #3

Closed emdann closed 2 years ago

emdann commented 2 years ago

At the moment assign_neighbourhood function takes as input a SingleCellExperiment object and builds the Milo object and runs buildGraph internally

https://github.com/MarioniLab/miloDE/blob/4f370cc4d20c8750a9fc06cd99b820b4c74e9f07/R/assign_neighbourhoods.R#L24-L29

but this is inefficient if you've already build a Milo obj and graph once.

Suggested change

Take as input a Milo object and add initial check for the graph slot (this also serves as a check that the object is a Milo object and not a simple SCE). If the graph slot is not there throw an error with suggestion "Please run miloR::buildGraph(Milo) before assing_neighbourhood"

amissarova commented 2 years ago

Counter-suggestion: Take as input a Milo object and add initial check for the graph slot (this also serves as a check that the object is a Milo object and not a simple SCE). If the graph slot is not there - build graph ourselves within the function using buildGraph(Milo)

emdann commented 2 years ago

Sounds good, with message saying "Running buildGraph" or smt like this

MikeDMorgan commented 2 years ago

Just butting in here - but you can also check if the input objects class is Milo, then it will have a graph slot, but will have an empty list() entry. This is because the graph slot will always be there in a Milo object, but it might be empty.